diff mbox series

[25/26] xfs: make it possible to disable fsverity

Message ID 171444680792.957659.14055056649560052839.stgit@frogsfrogsfrogs (mailing list archive)
State New
Headers show
Series [01/26] xfs: use unsigned ints for non-negative quantities in xfs_attr_remote.c | expand

Commit Message

Darrick J. Wong April 30, 2024, 3:30 a.m. UTC
From: Darrick J. Wong <djwong@kernel.org>

Create an experimental ioctl so that we can turn off fsverity.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Andrey Albershteyn <aalbersh@redhat.com>
---
 Documentation/filesystems/fsverity.rst |   10 ++++++
 fs/verity/enable.c                     |   50 ++++++++++++++++++++++++++++++++
 fs/xfs/xfs_fsverity.c                  |   46 +++++++++++++++++++++++++++++
 fs/xfs/xfs_ioctl.c                     |    6 ++++
 include/linux/fsverity.h               |   24 +++++++++++++++
 include/trace/events/fsverity.h        |   13 ++++++++
 include/uapi/linux/fsverity.h          |    1 +
 7 files changed, 150 insertions(+)

Comments

Christoph Hellwig May 1, 2024, 6:48 a.m. UTC | #1
On Mon, Apr 29, 2024 at 08:30:37PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Create an experimental ioctl so that we can turn off fsverity.

Didn't Eric argue against this?  And if we're adding this, I think
it should be a generic feature and not just xfs specific.
Darrick J. Wong May 1, 2024, 10:50 p.m. UTC | #2
On Tue, Apr 30, 2024 at 11:48:29PM -0700, Christoph Hellwig wrote:
> On Mon, Apr 29, 2024 at 08:30:37PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Create an experimental ioctl so that we can turn off fsverity.
> 
> Didn't Eric argue against this?  And if we're adding this, I think
> it should be a generic feature and not just xfs specific.

The tagging is a bit wrong, but it is a generic fsverity ioctl, though
ext4/f2fs/btrfs don't have implementations.

<shrug> According to Ted, programs that care about fsverity are supposed
to check that VERITY is set in the stat data, but I imagine those
programs aren't expecting it to turn off suddenly.  Maybe I should make
this CAP_SYS_ADMIN?  Or withdraw it?

--D
Eric Biggers May 2, 2024, 12:15 a.m. UTC | #3
On Wed, May 01, 2024 at 03:50:07PM -0700, Darrick J. Wong wrote:
> On Tue, Apr 30, 2024 at 11:48:29PM -0700, Christoph Hellwig wrote:
> > On Mon, Apr 29, 2024 at 08:30:37PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Create an experimental ioctl so that we can turn off fsverity.
> > 
> > Didn't Eric argue against this?  And if we're adding this, I think
> > it should be a generic feature and not just xfs specific.
> 
> The tagging is a bit wrong, but it is a generic fsverity ioctl, though
> ext4/f2fs/btrfs don't have implementations.
> 
> <shrug> According to Ted, programs that care about fsverity are supposed
> to check that VERITY is set in the stat data, but I imagine those
> programs aren't expecting it to turn off suddenly.  Maybe I should make
> this CAP_SYS_ADMIN?  Or withdraw it?
> 

I'm concerned that fsverity could be disabled after someone has already checked
for fsverity on a particular file.  Currently users only have to re-check for
fsverity if they close the file and re-open it (as in that case it might have
been replaced with a new file with fsverity disabled).

A similar issue also would exist for the in-kernel users of fsverity such as
overlayfs and IMA (upstream), and IPE
(https://lore.kernel.org/linux-security-module/1712969764-31039-1-git-send-email-wufan@linux.microsoft.com/).
For example, IPE is being proposed to cache some state about fsverity in the LSM
blob associated with the struct inode.  If fsverity is disabled on an inode,
that state would get out of sync.  This could allow bypassing the IPE policy.

CAP_SYS_ADMIN isn't supposed to give a license to bypass all security features
including LSMs, so using CAP_SYS_ADMIN doesn't seem like a great solution.

- Eric
Darrick J. Wong May 8, 2024, 8:31 p.m. UTC | #4
On Thu, May 02, 2024 at 12:15:01AM +0000, Eric Biggers wrote:
> On Wed, May 01, 2024 at 03:50:07PM -0700, Darrick J. Wong wrote:
> > On Tue, Apr 30, 2024 at 11:48:29PM -0700, Christoph Hellwig wrote:
> > > On Mon, Apr 29, 2024 at 08:30:37PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > Create an experimental ioctl so that we can turn off fsverity.
> > > 
> > > Didn't Eric argue against this?  And if we're adding this, I think
> > > it should be a generic feature and not just xfs specific.
> > 
> > The tagging is a bit wrong, but it is a generic fsverity ioctl, though
> > ext4/f2fs/btrfs don't have implementations.
> > 
> > <shrug> According to Ted, programs that care about fsverity are supposed
> > to check that VERITY is set in the stat data, but I imagine those
> > programs aren't expecting it to turn off suddenly.  Maybe I should make
> > this CAP_SYS_ADMIN?  Or withdraw it?
> > 
> 
> I'm concerned that fsverity could be disabled after someone has already checked
> for fsverity on a particular file.  Currently users only have to re-check for
> fsverity if they close the file and re-open it (as in that case it might have
> been replaced with a new file with fsverity disabled).
> 
> A similar issue also would exist for the in-kernel users of fsverity such as
> overlayfs and IMA (upstream), and IPE
> (https://lore.kernel.org/linux-security-module/1712969764-31039-1-git-send-email-wufan@linux.microsoft.com/).
> For example, IPE is being proposed to cache some state about fsverity in the LSM
> blob associated with the struct inode.  If fsverity is disabled on an inode,
> that state would get out of sync.  This could allow bypassing the IPE policy.
> 
> CAP_SYS_ADMIN isn't supposed to give a license to bypass all security features
> including LSMs, so using CAP_SYS_ADMIN doesn't seem like a great solution.

Hmm.  What if did something like what fsdax does to update the file
access methods?  We could clear the ondisk iflag but not the incore one;
set DONTCACHE on the dentry and the inode so that it will get reclaimed
ASAP instead of being put on the lru; and then tell userspace they have
to wait until the inode gets reclaimed and reloaded?

That would solve the problem of cached state (whether the statx flag
or IPE blobs) going stale because the only time we'd change the incore
flag is when there are zero open fds.

--D

> - Eric
>
Christoph Hellwig May 9, 2024, 5:04 a.m. UTC | #5
On Wed, May 08, 2024 at 01:31:48PM -0700, Darrick J. Wong wrote:
> Hmm.  What if did something like what fsdax does to update the file
> access methods?  We could clear the ondisk iflag but not the incore one;
> set DONTCACHE on the dentry and the inode so that it will get reclaimed
> ASAP instead of being put on the lru; and then tell userspace they have
> to wait until the inode gets reclaimed and reloaded?

Yikes.  That's a completely mess I'd rather get rid of than add more of
it.

What is the use case of disabling fsverity to start with vs just
removing a fsverity enabled file after copying the content out?
Darrick J. Wong May 9, 2024, 2:45 p.m. UTC | #6
On Wed, May 08, 2024 at 10:04:05PM -0700, Christoph Hellwig wrote:
> On Wed, May 08, 2024 at 01:31:48PM -0700, Darrick J. Wong wrote:
> > Hmm.  What if did something like what fsdax does to update the file
> > access methods?  We could clear the ondisk iflag but not the incore one;
> > set DONTCACHE on the dentry and the inode so that it will get reclaimed
> > ASAP instead of being put on the lru; and then tell userspace they have
> > to wait until the inode gets reclaimed and reloaded?
> 
> Yikes.  That's a completely mess I'd rather get rid of than add more of
> it.
> 
> What is the use case of disabling fsverity to start with vs just
> removing a fsverity enabled file after copying the content out?

How do you salvage the content of a fsverity file if the merkle tree
hashes don't match the data?  I'm thinking about the backup disk usecase
where you enable fsverity to detect bitrot in your video files but
they'd otherwise be mostly playable if it weren't for the EIO.

I guess you could always ddrescue the file, right?

--D
Christoph Hellwig May 9, 2024, 3:06 p.m. UTC | #7
On Thu, May 09, 2024 at 07:45:42AM -0700, Darrick J. Wong wrote:
> How do you salvage the content of a fsverity file if the merkle tree
> hashes don't match the data?  I'm thinking about the backup disk usecase
> where you enable fsverity to detect bitrot in your video files but
> they'd otherwise be mostly playable if it weren't for the EIO.

Why would you enable fsverity for DVD images on your backup files?
Darrick J. Wong May 9, 2024, 3:09 p.m. UTC | #8
On Thu, May 09, 2024 at 08:06:46AM -0700, Christoph Hellwig wrote:
> On Thu, May 09, 2024 at 07:45:42AM -0700, Darrick J. Wong wrote:
> > How do you salvage the content of a fsverity file if the merkle tree
> > hashes don't match the data?  I'm thinking about the backup disk usecase
> > where you enable fsverity to detect bitrot in your video files but
> > they'd otherwise be mostly playable if it weren't for the EIO.
> 
> Why would you enable fsverity for DVD images on your backup files?

xfs doesn't do data block checksums.

I already have a dumb python program that basically duplicates fsverity
style merkle trees but I was looking forward to sunsetting it... :P

--D
Christoph Hellwig May 9, 2024, 3:13 p.m. UTC | #9
On Thu, May 09, 2024 at 08:09:55AM -0700, Darrick J. Wong wrote:
> xfs doesn't do data block checksums.
> 
> I already have a dumb python program that basically duplicates fsverity
> style merkle trees but I was looking forward to sunsetting it... :P

Well, fsverity as-is is intended for use cases where you care about
integrity of the file.  For that disabling it really doesn't make
sense.  If we have other use cases we can probably add a variant
of fsverity that clearly deals with non-integrity checksums.
Although just disabling them if they mismatch still feels like a
somewhat odd usage model.
Darrick J. Wong May 9, 2024, 3:43 p.m. UTC | #10
On Thu, May 09, 2024 at 08:13:48AM -0700, Christoph Hellwig wrote:
> On Thu, May 09, 2024 at 08:09:55AM -0700, Darrick J. Wong wrote:
> > xfs doesn't do data block checksums.
> > 
> > I already have a dumb python program that basically duplicates fsverity
> > style merkle trees but I was looking forward to sunsetting it... :P
> 
> Well, fsverity as-is is intended for use cases where you care about
> integrity of the file.  For that disabling it really doesn't make
> sense.  If we have other use cases we can probably add a variant
> of fsverity that clearly deals with non-integrity checksums.
> Although just disabling them if they mismatch still feels like a
> somewhat odd usage model.

Yeah, it definitely exists in the same weird grey area of turning off
metadata checksum validation to extract as many files from a busted fs
as can be done.

--D
Theodore Ts'o May 17, 2024, 7:36 p.m. UTC | #11
On Thu, May 09, 2024 at 08:43:23AM -0700, Darrick J. Wong wrote:
> > Well, fsverity as-is is intended for use cases where you care about
> > integrity of the file.  For that disabling it really doesn't make
> > sense.  If we have other use cases we can probably add a variant
> > of fsverity that clearly deals with non-integrity checksums.
> > Although just disabling them if they mismatch still feels like a
> > somewhat odd usage model.
> 
> Yeah, it definitely exists in the same weird grey area of turning off
> metadata checksum validation to extract as many files from a busted fs
> as can be done.

I've certainly thought about the possibilities of adding a CRC
checksum type.  We do need to explicitly mark this as a
non-cryptographic checksum since it might have make a difference for
IMA policies, etc.  This would be useful for detecting problems for
people's video or music archives, for example.

I can imagine situations where it might make sense to allow the file
owner to be able to disable fsverity, whether the checksum and use
case involves cryptographic or non-cryptographic checksums.  Having a
flag in the fsverity header indicating whether dropping fsverity
protection requires elevated privileged or can be done by the file
owner seems to make sense to me.

      	       	    	     - Ted
diff mbox series

Patch

diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
index 887cdaf162a99..dc688b2eda68d 100644
--- a/Documentation/filesystems/fsverity.rst
+++ b/Documentation/filesystems/fsverity.rst
@@ -189,6 +189,16 @@  FS_IOC_ENABLE_VERITY can fail with the following errors:
   caller's file descriptor, another open file descriptor, or the file
   reference held by a writable memory map.
 
+FS_IOC_DISABLE_VERITY
+--------------------
+
+The FS_IOC_DISABLE_VERITY ioctl disables fs-verity on a file.  It takes
+a file descriptor.
+
+FS_IOC_DISABLE_VERITY can fail with the following errors:
+
+- ``EOPNOTSUPP``: the filesystem does not support disabling fs-verity.
+
 FS_IOC_MEASURE_VERITY
 ---------------------
 
diff --git a/fs/verity/enable.c b/fs/verity/enable.c
index 8c6fe4b72b14e..adf8886f4ed29 100644
--- a/fs/verity/enable.c
+++ b/fs/verity/enable.c
@@ -415,3 +415,53 @@  int fsverity_ioctl_enable(struct file *filp, const void __user *uarg)
 	return err;
 }
 EXPORT_SYMBOL_GPL(fsverity_ioctl_enable);
+
+/**
+ * fsverity_ioctl_disable() - disable verity on a file
+ * @filp: file to enable verity on
+ *
+ * Disable fs-verity on a file.  See the "FS_IOC_DISABLE_VERITY" section of
+ * Documentation/filesystems/fsverity.rst for the documentation.
+ *
+ * Return: 0 on success, -errno on failure
+ */
+int fsverity_ioctl_disable(struct file *filp)
+{
+	struct inode *inode = file_inode(filp);
+	const struct fsverity_operations *vops = inode->i_sb->s_vop;
+	struct fsverity_info *vi;
+	u64 tree_size = 0;
+	unsigned int block_size = 0;
+	int err;
+
+	trace_fsverity_disable(inode);
+
+	inode_lock(inode);
+	if (IS_VERITY(inode)) {
+		err = 0;
+		goto out_unlock;
+	}
+
+	if (!vops->disable_verity) {
+		err = -EOPNOTSUPP;
+		goto out_unlock;
+	}
+
+	vi = fsverity_get_info(inode);
+	if (vi) {
+		block_size = vi->tree_params.block_size;
+		tree_size = vi->tree_params.tree_size;
+	}
+
+	err = vops->disable_verity(filp, tree_size, block_size);
+	if (err)
+		goto out_unlock;
+
+	fsverity_cleanup_inode(inode);
+	inode_unlock(inode);
+	return 0;
+out_unlock:
+	inode_unlock(inode);
+	return err;
+}
+EXPORT_SYMBOL_GPL(fsverity_ioctl_disable);
diff --git a/fs/xfs/xfs_fsverity.c b/fs/xfs/xfs_fsverity.c
index 87edf23954336..184c3e14d581f 100644
--- a/fs/xfs/xfs_fsverity.c
+++ b/fs/xfs/xfs_fsverity.c
@@ -940,9 +940,55 @@  xfs_fsverity_file_corrupt(
 	xfs_inode_mark_sick(XFS_I(inode), XFS_SICK_INO_DATA);
 }
 
+/* Turn off fs-verity. */
+static int
+xfs_fsverity_disable(
+	struct file		*file,
+	u64			tree_size,
+	unsigned int		block_size)
+{
+	struct inode		*inode = file_inode(file);
+	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_trans	*tp;
+	int			error;
+
+	if (xfs_iflags_test(ip, XFS_VERITY_CONSTRUCTION))
+		return -EBUSY;
+
+	error = xfs_qm_dqattach(ip);
+	if (error)
+		return error;
+
+	xfs_fsverity_drop_cache(ip, tree_size, block_size);
+
+	/* Clear fsverity inode flag */
+	error = xfs_trans_alloc_inode(ip, &M_RES(mp)->tr_ichange, 0, 0, false,
+			&tp);
+	if (error)
+		return error;
+
+	ip->i_diflags2 &= ~XFS_DIFLAG2_VERITY;
+
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+	xfs_trans_set_sync(tp);
+
+	error = xfs_trans_commit(tp);
+	xfs_iunlock(ip, XFS_ILOCK_EXCL);
+	if (error)
+		return error;
+
+	inode->i_flags &= ~S_VERITY;
+	fsverity_cleanup_inode(inode);
+
+	/* Remove the fsverity xattrs. */
+	return xfs_fsverity_delete_metadata(ip, tree_size, block_size);
+}
+
 const struct fsverity_operations xfs_fsverity_ops = {
 	.begin_enable_verity		= xfs_fsverity_begin_enable,
 	.end_enable_verity		= xfs_fsverity_end_enable,
+	.disable_verity			= xfs_fsverity_disable,
 	.get_verity_descriptor		= xfs_fsverity_get_descriptor,
 	.read_merkle_tree_block		= xfs_fsverity_read_merkle,
 	.write_merkle_tree_block	= xfs_fsverity_write_merkle,
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index b05930462f461..d71fc9e6b83eb 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -42,6 +42,7 @@ 
 #include "xfs_exchrange.h"
 #include "xfs_handle.h"
 #include "xfs_rtgroup.h"
+#include "xfs_fsverity.h"
 
 #include <linux/mount.h>
 #include <linux/fileattr.h>
@@ -1590,6 +1591,11 @@  xfs_file_ioctl(
 			return -EOPNOTSUPP;
 		return fsverity_ioctl_read_metadata(filp, arg);
 
+	case FS_IOC_DISABLE_VERITY:
+		if (!xfs_has_verity(mp))
+			return -EOPNOTSUPP;
+		return fsverity_ioctl_disable(filp);
+
 	default:
 		return -ENOTTY;
 	}
diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
index 1336f4b9011ea..e9f570f65ed54 100644
--- a/include/linux/fsverity.h
+++ b/include/linux/fsverity.h
@@ -135,6 +135,24 @@  struct fsverity_operations {
 				 size_t desc_size, u64 merkle_tree_size,
 				 unsigned int tree_blocksize);
 
+	/**
+	 * Disable verity on the given file.
+	 *
+	 * @filp: a readonly file descriptor for the file
+	 * @merkle_tree_size: total bytes the Merkle tree takes up
+	 * @tree_blocksize: the Merkle tree block size
+	 *
+	 * The filesystem must do any needed filesystem-specific preparations
+	 * for disabling verity, e.g. truncating the merkle tree.  It also must
+	 * return -EBUSY if verity is already being enabled on the given file.
+	 *
+	 * i_rwsem is held for write.
+	 *
+	 * Return: 0 on success, -errno on failure
+	 */
+	int (*disable_verity)(struct file *filp, u64 merkle_tree_size,
+			      unsigned int tree_blocksize);
+
 	/**
 	 * Get the verity descriptor of the given inode.
 	 *
@@ -260,6 +278,7 @@  static inline struct fsverity_info *fsverity_get_info(const struct inode *inode)
 /* enable.c */
 
 int fsverity_ioctl_enable(struct file *filp, const void __user *arg);
+int fsverity_ioctl_disable(struct file *filp);
 
 /* measure.c */
 
@@ -326,6 +345,11 @@  static inline int fsverity_ioctl_enable(struct file *filp,
 	return -EOPNOTSUPP;
 }
 
+static inline int fsverity_ioctl_disable(struct file *filp)
+{
+	return -EOPNOTSUPP;
+}
+
 /* measure.c */
 
 static inline int fsverity_ioctl_measure(struct file *filp, void __user *arg)
diff --git a/include/trace/events/fsverity.h b/include/trace/events/fsverity.h
index 375fdddac6a99..2678dd3249b32 100644
--- a/include/trace/events/fsverity.h
+++ b/include/trace/events/fsverity.h
@@ -37,6 +37,19 @@  TRACE_EVENT(fsverity_enable,
 		__entry->num_levels)
 );
 
+TRACE_EVENT(fsverity_disable,
+	TP_PROTO(const struct inode *inode),
+	TP_ARGS(inode),
+	TP_STRUCT__entry(
+		__field(ino_t, ino)
+	),
+	TP_fast_assign(
+		__entry->ino = inode->i_ino;
+	),
+	TP_printk("ino %lu",
+		(unsigned long) __entry->ino)
+);
+
 TRACE_EVENT(fsverity_tree_done,
 	TP_PROTO(const struct inode *inode, const struct fsverity_info *vi,
 		 const struct merkle_tree_params *params),
diff --git a/include/uapi/linux/fsverity.h b/include/uapi/linux/fsverity.h
index 15384e22e331e..73a5f83754792 100644
--- a/include/uapi/linux/fsverity.h
+++ b/include/uapi/linux/fsverity.h
@@ -99,5 +99,6 @@  struct fsverity_read_metadata_arg {
 #define FS_IOC_MEASURE_VERITY	_IOWR('f', 134, struct fsverity_digest)
 #define FS_IOC_READ_VERITY_METADATA \
 	_IOWR('f', 135, struct fsverity_read_metadata_arg)
+#define FS_IOC_DISABLE_VERITY	_IO('f', 136)
 
 #endif /* _UAPI_LINUX_FSVERITY_H */