[28/29] xfs: allow verity files to be opened even if the fsverity metadata is damaged

Commit Message

Darrick J. Wong March 30, 2024, 12:43 a.m. UTC

From: Darrick J. Wong <[e-mail]>

There are more things that one can do with an open file descriptor on
XFS -- query extended attributes, scan for metadata damage, repair
metadata, etc.  None of this is possible if the fsverity metadata are
damaged, because that prevents the file from being opened.

Ignore a selective set of error codes that we know fsverity_file_open to
return if the verity descriptor is nonsense.

Signed-off-by: Darrick J. Wong <[e-mail]>
---
 fs/iomap/buffered-io.c |    8 ++++++++
 fs/xfs/xfs_file.c      |   19 ++++++++++++++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)

Comments

Andrey Albershteyn April 2, 2024, 6:04 p.m. UTC | #1

On 2024-03-29 17:43:22, Darrick J. Wong wrote:
> From: Darrick J. Wong <[e-mail]>
> 
> There are more things that one can do with an open file descriptor on
> XFS -- query extended attributes, scan for metadata damage, repair
> metadata, etc.  None of this is possible if the fsverity metadata are
> damaged, because that prevents the file from being opened.
> 
> Ignore a selective set of error codes that we know fsverity_file_open to
> return if the verity descriptor is nonsense.
> 
> Signed-off-by: Darrick J. Wong <[e-mail]>
> ---
>  fs/iomap/buffered-io.c |    8 ++++++++
>  fs/xfs/xfs_file.c      |   19 ++++++++++++++++++-
>  2 files changed, 26 insertions(+), 1 deletion(-)
> 
> 

Looks good to me:
Reviewed-by: Andrey Albershteyn <[e-mail]>

Colin Walters April 2, 2024, 8 p.m. UTC | #2

On Fri, Mar 29, 2024, at 8:43 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <[e-mail]>
>
> There are more things that one can do with an open file descriptor on
> XFS -- query extended attributes, scan for metadata damage, repair
> metadata, etc.  None of this is possible if the fsverity metadata are
> damaged, because that prevents the file from being opened.
>
> Ignore a selective set of error codes that we know fsverity_file_open to
> return if the verity descriptor is nonsense.
>
> Signed-off-by: Darrick J. Wong <[e-mail]>
> ---
>  fs/iomap/buffered-io.c |    8 ++++++++
>  fs/xfs/xfs_file.c      |   19 ++++++++++++++++++-
>  2 files changed, 26 insertions(+), 1 deletion(-)
>
>
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index 9f9d929dfeebc..e68a15b72dbdd 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -487,6 +487,14 @@ static loff_t iomap_readpage_iter(const struct 
> iomap_iter *iter,
>  	size_t poff, plen;
>  	sector_t sector;
> 
> +	/*
> +	 * If this verity file hasn't been activated, fail read attempts.  This
> +	 * can happen if the calling filesystem allows files to be opened even
> +	 * with damaged verity metadata.
> +	 */
> +	if (IS_VERITY(iter->inode) && !fsverity_active(iter->inode))
> +		return -EIO;
> +
>  	if (iomap->type == IOMAP_INLINE)
>  		return iomap_read_inline_data(iter, folio);
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index c0b3e8146b753..36034eaefbf55 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1431,8 +1431,25 @@ xfs_file_open(
>  			FMODE_DIO_PARALLEL_WRITE | FMODE_CAN_ODIRECT;
> 
>  	error = fsverity_file_open(inode, file);
> -	if (error)
> +	switch (error) {
> +	case -EFBIG:
> +	case -EINVAL:
> +	case -EMSGSIZE:
> +	case -EFSCORRUPTED:
> +		/*
> +		 * Be selective about which fsverity errors we propagate to
> +		 * userspace; we still want to be able to open this file even
> +		 * if reads don't work.  Someone might want to perform an
> +		 * online repair.
> +		 */
> +		if (has_capability_noaudit(current, CAP_SYS_ADMIN))
> +			break;

As I understand it, fsverity (and dm-verity) are desirable in high-safety and integrity requirement cases where the goal is for the system to "fail closed" if errors in general are detected; anything that would have the system be in an ill-defined state.

A lot of ambient processes are going to have CAP_SYS_ADMIN and this will just swallow these errors for those (will things the EFSCORRUPTED path at least have been logged by a lower level function?)...whereas this is only needed just for a very few tools.

At least for composefs the quoted cases of "query extended attributes, scan for metadata damage, repair metadata" are all things that canonically live in the composefs metadata (EROFS) blob, so in theory there's a lot less of a need to query/inspect it for those use cases.  (Maybe for composefs we should force canonicalize all the underlying files to have mode 0400 and no xattrs or something and add that to its repair).

I hesitate to say it but maybe there should be some ioctl for online repair use cases only, or perhaps a new O_NOVERITY special flag to openat2()?

Darrick J. Wong April 2, 2024, 10:52 p.m. UTC | #3

On Tue, Apr 02, 2024 at 04:00:06PM -0400, Colin Walters wrote:
> 
> 
> On Fri, Mar 29, 2024, at 8:43 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <[e-mail]>
> >
> > There are more things that one can do with an open file descriptor on
> > XFS -- query extended attributes, scan for metadata damage, repair
> > metadata, etc.  None of this is possible if the fsverity metadata are
> > damaged, because that prevents the file from being opened.
> >
> > Ignore a selective set of error codes that we know fsverity_file_open to
> > return if the verity descriptor is nonsense.
> >
> > Signed-off-by: Darrick J. Wong <[e-mail]>
> > ---
> >  fs/iomap/buffered-io.c |    8 ++++++++
> >  fs/xfs/xfs_file.c      |   19 ++++++++++++++++++-
> >  2 files changed, 26 insertions(+), 1 deletion(-)
> >
> >
> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > index 9f9d929dfeebc..e68a15b72dbdd 100644
> > --- a/fs/iomap/buffered-io.c
> > +++ b/fs/iomap/buffered-io.c
> > @@ -487,6 +487,14 @@ static loff_t iomap_readpage_iter(const struct 
> > iomap_iter *iter,
> >  	size_t poff, plen;
> >  	sector_t sector;
> > 
> > +	/*
> > +	 * If this verity file hasn't been activated, fail read attempts.  This
> > +	 * can happen if the calling filesystem allows files to be opened even
> > +	 * with damaged verity metadata.
> > +	 */
> > +	if (IS_VERITY(iter->inode) && !fsverity_active(iter->inode))
> > +		return -EIO;
> > +
> >  	if (iomap->type == IOMAP_INLINE)
> >  		return iomap_read_inline_data(iter, folio);
> > 
> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > index c0b3e8146b753..36034eaefbf55 100644
> > --- a/fs/xfs/xfs_file.c
> > +++ b/fs/xfs/xfs_file.c
> > @@ -1431,8 +1431,25 @@ xfs_file_open(
> >  			FMODE_DIO_PARALLEL_WRITE | FMODE_CAN_ODIRECT;
> > 
> >  	error = fsverity_file_open(inode, file);
> > -	if (error)
> > +	switch (error) {
> > +	case -EFBIG:
> > +	case -EINVAL:
> > +	case -EMSGSIZE:
> > +	case -EFSCORRUPTED:
> > +		/*
> > +		 * Be selective about which fsverity errors we propagate to
> > +		 * userspace; we still want to be able to open this file even
> > +		 * if reads don't work.  Someone might want to perform an
> > +		 * online repair.
> > +		 */
> > +		if (has_capability_noaudit(current, CAP_SYS_ADMIN))
> > +			break;
> 
> As I understand it, fsverity (and dm-verity) are desirable in
> high-safety and integrity requirement cases where the goal is for the
> system to "fail closed" if errors in general are detected; anything
> that would have the system be in an ill-defined state.

Is "open() fails if verity metadata are trashed" a hard requirement?

Reads will still fail due to (iomap) readahead returning EIO for a file
that is IS_VERITY() && !fsverity_active().  This is (afaict) the state
you end up with when the fsverity open fails.  ext4/f2fs don't do that,
but they also don't have online fsck so once a file's dead it's dead.

> A lot of ambient processes are going to have CAP_SYS_ADMIN and this
> will just swallow these errors for those (will things the EFSCORRUPTED
> path at least have been logged by a lower level function?)...whereas
> this is only needed just for a very few tools.
> 
> At least for composefs the quoted cases of "query extended attributes,
> scan for metadata damage, repair metadata" are all things that
> canonically live in the composefs metadata (EROFS) blob, so in theory
> there's a lot less of a need to query/inspect it for those use cases.
> (Maybe for composefs we should force canonicalize all the underlying
> files to have mode 0400 and no xattrs or something and add that to its
> repair).

<shrug> I don't know if regular (i.e. non-verity) xattrs are one of the
things that get frozen by verity?  Storing fsverity metadata in private
namespace xattrs is unique to xfs.

> I hesitate to say it but maybe there should be some ioctl for online
> repair use cases only, or perhaps a new O_NOVERITY special flag to
> openat2()?

"openat2 but without meddling from the VFS"?  Tempting... ;)

--D

Eric Biggers April 2, 2024, 11:45 p.m. UTC | #4

On Tue, Apr 02, 2024 at 03:52:16PM -0700, Darrick J. Wong wrote:
> On Tue, Apr 02, 2024 at 04:00:06PM -0400, Colin Walters wrote:
> > 
> > 
> > On Fri, Mar 29, 2024, at 8:43 PM, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <[e-mail]>
> > >
> > > There are more things that one can do with an open file descriptor on
> > > XFS -- query extended attributes, scan for metadata damage, repair
> > > metadata, etc.  None of this is possible if the fsverity metadata are
> > > damaged, because that prevents the file from being opened.
> > >
> > > Ignore a selective set of error codes that we know fsverity_file_open to
> > > return if the verity descriptor is nonsense.
> > >
> > > Signed-off-by: Darrick J. Wong <[e-mail]>
> > > ---
> > >  fs/iomap/buffered-io.c |    8 ++++++++
> > >  fs/xfs/xfs_file.c      |   19 ++++++++++++++++++-
> > >  2 files changed, 26 insertions(+), 1 deletion(-)
> > >
> > >
> > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > > index 9f9d929dfeebc..e68a15b72dbdd 100644
> > > --- a/fs/iomap/buffered-io.c
> > > +++ b/fs/iomap/buffered-io.c
> > > @@ -487,6 +487,14 @@ static loff_t iomap_readpage_iter(const struct 
> > > iomap_iter *iter,
> > >  	size_t poff, plen;
> > >  	sector_t sector;
> > > 
> > > +	/*
> > > +	 * If this verity file hasn't been activated, fail read attempts.  This
> > > +	 * can happen if the calling filesystem allows files to be opened even
> > > +	 * with damaged verity metadata.
> > > +	 */
> > > +	if (IS_VERITY(iter->inode) && !fsverity_active(iter->inode))
> > > +		return -EIO;
> > > +
> > >  	if (iomap->type == IOMAP_INLINE)
> > >  		return iomap_read_inline_data(iter, folio);
> > > 
> > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > index c0b3e8146b753..36034eaefbf55 100644
> > > --- a/fs/xfs/xfs_file.c
> > > +++ b/fs/xfs/xfs_file.c
> > > @@ -1431,8 +1431,25 @@ xfs_file_open(
> > >  			FMODE_DIO_PARALLEL_WRITE | FMODE_CAN_ODIRECT;
> > > 
> > >  	error = fsverity_file_open(inode, file);
> > > -	if (error)
> > > +	switch (error) {
> > > +	case -EFBIG:
> > > +	case -EINVAL:
> > > +	case -EMSGSIZE:
> > > +	case -EFSCORRUPTED:
> > > +		/*
> > > +		 * Be selective about which fsverity errors we propagate to
> > > +		 * userspace; we still want to be able to open this file even
> > > +		 * if reads don't work.  Someone might want to perform an
> > > +		 * online repair.
> > > +		 */
> > > +		if (has_capability_noaudit(current, CAP_SYS_ADMIN))
> > > +			break;
> > 
> > As I understand it, fsverity (and dm-verity) are desirable in
> > high-safety and integrity requirement cases where the goal is for the
> > system to "fail closed" if errors in general are detected; anything
> > that would have the system be in an ill-defined state.
> 
> Is "open() fails if verity metadata are trashed" a hard requirement?
> 
> Reads will still fail due to (iomap) readahead returning EIO for a file
> that is IS_VERITY() && !fsverity_active().  This is (afaict) the state
> you end up with when the fsverity open fails.  ext4/f2fs don't do that,
> but they also don't have online fsck so once a file's dead it's dead.
> 

We really should have the same behavior on all filesystems, and that behavior
should be documented in Documentation/filesystems/fsverity.rst.  I guess you
want this for XFS_IOC_SCRUB_METADATA?  That takes in an inode number directly,
in xfs_scrub_metadata::sm_ino; does it even need to be executed on the same file
it's checking?  Anyway, allowing the open means that the case of IS_VERITY() &&
!fsverity_active() needs to be handled later in any case when I/O may be done to
the file.  We need to be super careful to ensure that all cases are handled.

Even just considering this patchset and XFS only, it looks like you got it wrong
in xfs_file_read_iter().  You're allowing direct I/O to files that have
IS_VERITY() && !fsverity_active().

This change also invalidates the documentation for fsverity_active() which is:

/**
 * fsverity_active() - do reads from the inode need to go through fs-verity?
 * @inode: inode to check
 *
 * This checks whether ->i_verity_info has been set.
 *
 * Filesystems call this from ->readahead() to check whether the pages need to
 * be verified or not.  Don't use IS_VERITY() for this purpose; it's subject to
 * a race condition where the file is being read concurrently with
 * FS_IOC_ENABLE_VERITY completing.  (S_VERITY is set before ->i_verity_info.)
 *
 * Return: true if reads need to go through fs-verity, otherwise false
 */

I think that if you'd like to move forward with this, it would take a patchset
that brings the behavior to all filesystems and considers all callers of
fsverity_active().

Another consideration will be whether the fsverity builtin signature not
matching the file, not being trusted, or being malformed counts as "the fsverity
metadata being damaged".

- Eric

Colin Walters April 3, 2024, 12:10 a.m. UTC | #5

[cc alexl@, retained quotes for context]

On Tue, Apr 2, 2024, at 6:52 PM, Darrick J. Wong wrote:
> On Tue, Apr 02, 2024 at 04:00:06PM -0400, Colin Walters wrote:
>> 
>> 
>> On Fri, Mar 29, 2024, at 8:43 PM, Darrick J. Wong wrote:
>> > From: Darrick J. Wong <[e-mail]>
>> >
>> > There are more things that one can do with an open file descriptor on
>> > XFS -- query extended attributes, scan for metadata damage, repair
>> > metadata, etc.  None of this is possible if the fsverity metadata are
>> > damaged, because that prevents the file from being opened.
>> >
>> > Ignore a selective set of error codes that we know fsverity_file_open to
>> > return if the verity descriptor is nonsense.
>> >
>> > Signed-off-by: Darrick J. Wong <[e-mail]>
>> > ---
>> >  fs/iomap/buffered-io.c |    8 ++++++++
>> >  fs/xfs/xfs_file.c      |   19 ++++++++++++++++++-
>> >  2 files changed, 26 insertions(+), 1 deletion(-)
>> >
>> >
>> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
>> > index 9f9d929dfeebc..e68a15b72dbdd 100644
>> > --- a/fs/iomap/buffered-io.c
>> > +++ b/fs/iomap/buffered-io.c
>> > @@ -487,6 +487,14 @@ static loff_t iomap_readpage_iter(const struct 
>> > iomap_iter *iter,
>> >  	size_t poff, plen;
>> >  	sector_t sector;
>> > 
>> > +	/*
>> > +	 * If this verity file hasn't been activated, fail read attempts.  This
>> > +	 * can happen if the calling filesystem allows files to be opened even
>> > +	 * with damaged verity metadata.
>> > +	 */
>> > +	if (IS_VERITY(iter->inode) && !fsverity_active(iter->inode))
>> > +		return -EIO;
>> > +
>> >  	if (iomap->type == IOMAP_INLINE)
>> >  		return iomap_read_inline_data(iter, folio);
>> > 
>> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
>> > index c0b3e8146b753..36034eaefbf55 100644
>> > --- a/fs/xfs/xfs_file.c
>> > +++ b/fs/xfs/xfs_file.c
>> > @@ -1431,8 +1431,25 @@ xfs_file_open(
>> >  			FMODE_DIO_PARALLEL_WRITE | FMODE_CAN_ODIRECT;
>> > 
>> >  	error = fsverity_file_open(inode, file);
>> > -	if (error)
>> > +	switch (error) {
>> > +	case -EFBIG:
>> > +	case -EINVAL:
>> > +	case -EMSGSIZE:
>> > +	case -EFSCORRUPTED:
>> > +		/*
>> > +		 * Be selective about which fsverity errors we propagate to
>> > +		 * userspace; we still want to be able to open this file even
>> > +		 * if reads don't work.  Someone might want to perform an
>> > +		 * online repair.
>> > +		 */
>> > +		if (has_capability_noaudit(current, CAP_SYS_ADMIN))
>> > +			break;
>> 
>> As I understand it, fsverity (and dm-verity) are desirable in
>> high-safety and integrity requirement cases where the goal is for the
>> system to "fail closed" if errors in general are detected; anything
>> that would have the system be in an ill-defined state.
>
> Is "open() fails if verity metadata are trashed" a hard requirement?

I can't say authoritatively, but I do want to ensure we've dug into the semantics here, and I agree with Eric that it would make the most sense to have this be consistent across filesystems.

> Reads will still fail due to (iomap) readahead returning EIO for a file
> that is IS_VERITY() && !fsverity_active().  This is (afaict) the state
> you end up with when the fsverity open fails.  ext4/f2fs don't do that,
> but they also don't have online fsck so once a file's dead it's dead.

OK, right.  Allowing an open() but having read() fail seems like it doesn't weaken things too much in reality.  I think what makes me uncomfortable is the error-swallowing; but yes, in theory we should get the same or similar error on a subsequent read().

> <shrug> I don't know if regular (i.e. non-verity) xattrs are one of the
> things that get frozen by verity?  Storing fsverity metadata in private
> namespace xattrs is unique to xfs.

No, verity only covers file contents, no other metadata.  This is one of the rationales for composefs (e.g. ensuring things like the suid bit, security.selinux xattr etc. are covered as well as in general complete filesystem trees).

>> I hesitate to say it but maybe there should be some ioctl for online
>> repair use cases only, or perhaps a new O_NOVERITY special flag to
>> openat2()?
>
> "openat2 but without meddling from the VFS"?  Tempting... ;)

Or really any lower level even filesystem-specific API for the online fsck case.  
Adding a blanket new special case for all CAP_SYS_ADMIN processes covers a lot of things that don't need that.

Darrick J. Wong April 3, 2024, 1:34 a.m. UTC | #6

On Tue, Apr 02, 2024 at 04:45:58PM -0700, Eric Biggers wrote:
> On Tue, Apr 02, 2024 at 03:52:16PM -0700, Darrick J. Wong wrote:
> > On Tue, Apr 02, 2024 at 04:00:06PM -0400, Colin Walters wrote:
> > > 
> > > 
> > > On Fri, Mar 29, 2024, at 8:43 PM, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <[e-mail]>
> > > >
> > > > There are more things that one can do with an open file descriptor on
> > > > XFS -- query extended attributes, scan for metadata damage, repair
> > > > metadata, etc.  None of this is possible if the fsverity metadata are
> > > > damaged, because that prevents the file from being opened.
> > > >
> > > > Ignore a selective set of error codes that we know fsverity_file_open to
> > > > return if the verity descriptor is nonsense.
> > > >
> > > > Signed-off-by: Darrick J. Wong <[e-mail]>
> > > > ---
> > > >  fs/iomap/buffered-io.c |    8 ++++++++
> > > >  fs/xfs/xfs_file.c      |   19 ++++++++++++++++++-
> > > >  2 files changed, 26 insertions(+), 1 deletion(-)
> > > >
> > > >
> > > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > > > index 9f9d929dfeebc..e68a15b72dbdd 100644
> > > > --- a/fs/iomap/buffered-io.c
> > > > +++ b/fs/iomap/buffered-io.c
> > > > @@ -487,6 +487,14 @@ static loff_t iomap_readpage_iter(const struct 
> > > > iomap_iter *iter,
> > > >  	size_t poff, plen;
> > > >  	sector_t sector;
> > > > 
> > > > +	/*
> > > > +	 * If this verity file hasn't been activated, fail read attempts.  This
> > > > +	 * can happen if the calling filesystem allows files to be opened even
> > > > +	 * with damaged verity metadata.
> > > > +	 */
> > > > +	if (IS_VERITY(iter->inode) && !fsverity_active(iter->inode))
> > > > +		return -EIO;
> > > > +
> > > >  	if (iomap->type == IOMAP_INLINE)
> > > >  		return iomap_read_inline_data(iter, folio);
> > > > 
> > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > > index c0b3e8146b753..36034eaefbf55 100644
> > > > --- a/fs/xfs/xfs_file.c
> > > > +++ b/fs/xfs/xfs_file.c
> > > > @@ -1431,8 +1431,25 @@ xfs_file_open(
> > > >  			FMODE_DIO_PARALLEL_WRITE | FMODE_CAN_ODIRECT;
> > > > 
> > > >  	error = fsverity_file_open(inode, file);
> > > > -	if (error)
> > > > +	switch (error) {
> > > > +	case -EFBIG:
> > > > +	case -EINVAL:
> > > > +	case -EMSGSIZE:
> > > > +	case -EFSCORRUPTED:
> > > > +		/*
> > > > +		 * Be selective about which fsverity errors we propagate to
> > > > +		 * userspace; we still want to be able to open this file even
> > > > +		 * if reads don't work.  Someone might want to perform an
> > > > +		 * online repair.
> > > > +		 */
> > > > +		if (has_capability_noaudit(current, CAP_SYS_ADMIN))
> > > > +			break;
> > > 
> > > As I understand it, fsverity (and dm-verity) are desirable in
> > > high-safety and integrity requirement cases where the goal is for the
> > > system to "fail closed" if errors in general are detected; anything
> > > that would have the system be in an ill-defined state.
> > 
> > Is "open() fails if verity metadata are trashed" a hard requirement?
> > 
> > Reads will still fail due to (iomap) readahead returning EIO for a file
> > that is IS_VERITY() && !fsverity_active().  This is (afaict) the state
> > you end up with when the fsverity open fails.  ext4/f2fs don't do that,
> > but they also don't have online fsck so once a file's dead it's dead.
> > 
> 
> We really should have the same behavior on all filesystems, and that behavior
> should be documented in Documentation/filesystems/fsverity.rst.  I guess you
> want this for XFS_IOC_SCRUB_METADATA?

Yes.  xfs_scrub tries to open every regular file that it can, but if the
fsverity metadata is too badly damaged then the open() returns EMSGSIZE
or EINVAL or something.  The EMSGSIZE is particularly nasty since it's
not listed in the openat() manpage as a possible error code, which
surprised me.

>                                        That takes in an inode number directly,
> in xfs_scrub_metadata::sm_ino; does it even need to be executed on the same file
> it's checking?

<nod> The metadata repairs themselves can use scrub-by-handle mode, so
it's not *so* hard to handle it gracefully.

>                 Anyway, allowing the open means that the case of IS_VERITY() &&
> !fsverity_active() needs to be handled later in any case when I/O may be done to
> the file.  We need to be super careful to ensure that all cases are handled.

I /think/ most everything else is gated on IS_VERITY, right?

> Even just considering this patchset and XFS only, it looks like you got it wrong
> in xfs_file_read_iter().  You're allowing direct I/O to files that have
> IS_VERITY() && !fsverity_active().

Ahaha, yeah, that needs to be changed to:

	else if ((iocb->ki_flags & IOCB_DIRECT) && !IS_VERITY(inode))
		ret = xfs_file_dio_read(iocb, to);

Good catch.

> This change also invalidates the documentation for fsverity_active() which is:
> 
> /**
>  * fsverity_active() - do reads from the inode need to go through fs-verity?
>  * @inode: inode to check
>  *
>  * This checks whether ->i_verity_info has been set.
>  *
>  * Filesystems call this from ->readahead() to check whether the pages need to
>  * be verified or not.  Don't use IS_VERITY() for this purpose; it's subject to
>  * a race condition where the file is being read concurrently with
>  * FS_IOC_ENABLE_VERITY completing.  (S_VERITY is set before ->i_verity_info.)
>  *
>  * Return: true if reads need to go through fs-verity, otherwise false
>  */
> 
> I think that if you'd like to move forward with this, it would take a patchset
> that brings the behavior to all filesystems and considers all callers of
> fsverity_active().

<nod> If you think it's a reasonable thing to allow, then I'll of course
apply it to btr/ext4/f2fs.

> Another consideration will be whether the fsverity builtin signature not
> matching the file, not being trusted, or being malformed counts as "the fsverity
> metadata being damaged".

<shrug> Can you easily check that in the open routine?  I figured that
signature validation problems would manifest as read errors.

--D

> - Eric
>

Darrick J. Wong April 3, 2024, 1:39 a.m. UTC | #7

On Tue, Apr 02, 2024 at 08:10:15PM -0400, Colin Walters wrote:
> [cc alexl@, retained quotes for context]
> 
> On Tue, Apr 2, 2024, at 6:52 PM, Darrick J. Wong wrote:
> > On Tue, Apr 02, 2024 at 04:00:06PM -0400, Colin Walters wrote:
> >> 
> >> 
> >> On Fri, Mar 29, 2024, at 8:43 PM, Darrick J. Wong wrote:
> >> > From: Darrick J. Wong <[e-mail]>
> >> >
> >> > There are more things that one can do with an open file descriptor on
> >> > XFS -- query extended attributes, scan for metadata damage, repair
> >> > metadata, etc.  None of this is possible if the fsverity metadata are
> >> > damaged, because that prevents the file from being opened.
> >> >
> >> > Ignore a selective set of error codes that we know fsverity_file_open to
> >> > return if the verity descriptor is nonsense.
> >> >
> >> > Signed-off-by: Darrick J. Wong <[e-mail]>
> >> > ---
> >> >  fs/iomap/buffered-io.c |    8 ++++++++
> >> >  fs/xfs/xfs_file.c      |   19 ++++++++++++++++++-
> >> >  2 files changed, 26 insertions(+), 1 deletion(-)
> >> >
> >> >
> >> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> >> > index 9f9d929dfeebc..e68a15b72dbdd 100644
> >> > --- a/fs/iomap/buffered-io.c
> >> > +++ b/fs/iomap/buffered-io.c
> >> > @@ -487,6 +487,14 @@ static loff_t iomap_readpage_iter(const struct 
> >> > iomap_iter *iter,
> >> >  	size_t poff, plen;
> >> >  	sector_t sector;
> >> > 
> >> > +	/*
> >> > +	 * If this verity file hasn't been activated, fail read attempts.  This
> >> > +	 * can happen if the calling filesystem allows files to be opened even
> >> > +	 * with damaged verity metadata.
> >> > +	 */
> >> > +	if (IS_VERITY(iter->inode) && !fsverity_active(iter->inode))
> >> > +		return -EIO;
> >> > +
> >> >  	if (iomap->type == IOMAP_INLINE)
> >> >  		return iomap_read_inline_data(iter, folio);
> >> > 
> >> > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> >> > index c0b3e8146b753..36034eaefbf55 100644
> >> > --- a/fs/xfs/xfs_file.c
> >> > +++ b/fs/xfs/xfs_file.c
> >> > @@ -1431,8 +1431,25 @@ xfs_file_open(
> >> >  			FMODE_DIO_PARALLEL_WRITE | FMODE_CAN_ODIRECT;
> >> > 
> >> >  	error = fsverity_file_open(inode, file);
> >> > -	if (error)
> >> > +	switch (error) {
> >> > +	case -EFBIG:
> >> > +	case -EINVAL:
> >> > +	case -EMSGSIZE:
> >> > +	case -EFSCORRUPTED:
> >> > +		/*
> >> > +		 * Be selective about which fsverity errors we propagate to
> >> > +		 * userspace; we still want to be able to open this file even
> >> > +		 * if reads don't work.  Someone might want to perform an
> >> > +		 * online repair.
> >> > +		 */
> >> > +		if (has_capability_noaudit(current, CAP_SYS_ADMIN))
> >> > +			break;
> >> 
> >> As I understand it, fsverity (and dm-verity) are desirable in
> >> high-safety and integrity requirement cases where the goal is for the
> >> system to "fail closed" if errors in general are detected; anything
> >> that would have the system be in an ill-defined state.
> >
> > Is "open() fails if verity metadata are trashed" a hard requirement?
> 
> I can't say authoritatively, but I do want to ensure we've dug into
> the semantics here, and I agree with Eric that it would make the most
> sense to have this be consistent across filesystems.
> 
> > Reads will still fail due to (iomap) readahead returning EIO for a file
> > that is IS_VERITY() && !fsverity_active().  This is (afaict) the state
> > you end up with when the fsverity open fails.  ext4/f2fs don't do that,
> > but they also don't have online fsck so once a file's dead it's dead.
> 
> OK, right.  Allowing an open() but having read() fail seems like it
> doesn't weaken things too much in reality.  I think what makes me
> uncomfortable is the error-swallowing; but yes, in theory we should
> get the same or similar error on a subsequent read().

<nod> I /could/ write up some tests to make sure that happens.

> > <shrug> I don't know if regular (i.e. non-verity) xattrs are one of the
> > things that get frozen by verity?  Storing fsverity metadata in private
> > namespace xattrs is unique to xfs.
> 
> No, verity only covers file contents, no other metadata.  This is one
> of the rationales for composefs (e.g. ensuring things like the suid
> bit, security.selinux xattr etc. are covered as well as in general
> complete filesystem trees).
> 
> >> I hesitate to say it but maybe there should be some ioctl for online
> >> repair use cases only, or perhaps a new O_NOVERITY special flag to
> >> openat2()?
> >
> > "openat2 but without meddling from the VFS"?  Tempting... ;)
> 
> Or really any lower level even filesystem-specific API for the online
> fsck case.  Adding a blanket new special case for all CAP_SYS_ADMIN
> processes covers a lot of things that don't need that.

I suppose there could be an O_NOVALIDATION to turn off data checksum
validation on btrfs/bcachefs too.  But then you'd want to careful
controls on who gets to use it.  Maybe not liblzma_la-crc64-fast.o.

--D

Dave Chinner April 3, 2024, 1:59 a.m. UTC | #8

On Tue, Apr 02, 2024 at 06:39:03PM -0700, Darrick J. Wong wrote:
> On Tue, Apr 02, 2024 at 08:10:15PM -0400, Colin Walters wrote:
> > >> I hesitate to say it but maybe there should be some ioctl for online
> > >> repair use cases only, or perhaps a new O_NOVERITY special flag to
> > >> openat2()?
> > >
> > > "openat2 but without meddling from the VFS"?  Tempting... ;)
> > 
> > Or really any lower level even filesystem-specific API for the online
> > fsck case.  Adding a blanket new special case for all CAP_SYS_ADMIN
> > processes covers a lot of things that don't need that.
> 
> I suppose there could be an O_NOVALIDATION to turn off data checksum
> validation on btrfs/bcachefs too.  But then you'd want to careful
> controls on who gets to use it.  Maybe not liblzma_la-crc64-fast.o.

Just use XFS_IOC_OPEN_BY_HANDLE same as xfs_fsr and xfsdump do. The
handle can be build in userspace from the inode bulkstat
information, and for typical inode contents verification purposes we
don't actually need path-based open access to the inodes. That would
then mean we can simple add our own open flag to return a fd that
can do data operations that short-circuit verification...

Cheers,

Dave.

Darrick J. Wong April 3, 2024, 3:19 a.m. UTC | #9

On Wed, Apr 03, 2024 at 12:59:22PM +1100, Dave Chinner wrote:
> On Tue, Apr 02, 2024 at 06:39:03PM -0700, Darrick J. Wong wrote:
> > On Tue, Apr 02, 2024 at 08:10:15PM -0400, Colin Walters wrote:
> > > >> I hesitate to say it but maybe there should be some ioctl for online
> > > >> repair use cases only, or perhaps a new O_NOVERITY special flag to
> > > >> openat2()?
> > > >
> > > > "openat2 but without meddling from the VFS"?  Tempting... ;)
> > > 
> > > Or really any lower level even filesystem-specific API for the online
> > > fsck case.  Adding a blanket new special case for all CAP_SYS_ADMIN
> > > processes covers a lot of things that don't need that.
> > 
> > I suppose there could be an O_NOVALIDATION to turn off data checksum
> > validation on btrfs/bcachefs too.  But then you'd want to careful
> > controls on who gets to use it.  Maybe not liblzma_la-crc64-fast.o.
> 
> Just use XFS_IOC_OPEN_BY_HANDLE same as xfs_fsr and xfsdump do. The
> handle can be build in userspace from the inode bulkstat
> information, and for typical inode contents verification purposes we
> don't actually need path-based open access to the inodes. That would
> then mean we can simple add our own open flag to return a fd that
> can do data operations that short-circuit verification...

Heh, ok.  Are there any private flags that get passed via
xfs_fsop_handlereq_t::oflags?  Or does that mean defining a top level
O_FLAG that cannot be passed through openat but /can/ be sent via
open_by_handle?

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> [e-mail]
>

Alexander Larsson April 3, 2024, 8:35 a.m. UTC | #10

On Tue, 2024-04-02 at 20:10 -0400, Colin Walters wrote:
> [cc alexl@, retained quotes for context]
> 
> On Tue, Apr 2, 2024, at 6:52 PM, Darrick J. Wong wrote:
> > On Tue, Apr 02, 2024 at 04:00:06PM -0400, Colin Walters wrote:
> > > 
> > > 
> > > On Fri, Mar 29, 2024, at 8:43 PM, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <[e-mail]>
> > > > 
> > > > There are more things that one can do with an open file
> > > > descriptor on
> > > > XFS -- query extended attributes, scan for metadata damage,
> > > > repair
> > > > metadata, etc.  None of this is possible if the fsverity
> > > > metadata are
> > > > damaged, because that prevents the file from being opened.
> > > > 
> > > > Ignore a selective set of error codes that we know
> > > > fsverity_file_open to
> > > > return if the verity descriptor is nonsense.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <[e-mail]>
> > > > ---
> > > >  fs/iomap/buffered-io.c |    8 ++++++++
> > > >  fs/xfs/xfs_file.c      |   19 ++++++++++++++++++-
> > > >  2 files changed, 26 insertions(+), 1 deletion(-)
> > > > 
> > > > 
> > > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > > > index 9f9d929dfeebc..e68a15b72dbdd 100644
> > > > --- a/fs/iomap/buffered-io.c
> > > > +++ b/fs/iomap/buffered-io.c
> > > > @@ -487,6 +487,14 @@ static loff_t iomap_readpage_iter(const
> > > > struct 
> > > > iomap_iter *iter,
> > > >  	size_t poff, plen;
> > > >  	sector_t sector;
> > > > 
> > > > +	/*
> > > > +	 * If this verity file hasn't been activated, fail
> > > > read attempts.  This
> > > > +	 * can happen if the calling filesystem allows files
> > > > to be opened even
> > > > +	 * with damaged verity metadata.
> > > > +	 */
> > > > +	if (IS_VERITY(iter->inode) && !fsverity_active(iter-
> > > > >inode))
> > > > +		return -EIO;
> > > > +
> > > >  	if (iomap->type == IOMAP_INLINE)
> > > >  		return iomap_read_inline_data(iter, folio);
> > > > 
> > > > diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> > > > index c0b3e8146b753..36034eaefbf55 100644
> > > > --- a/fs/xfs/xfs_file.c
> > > > +++ b/fs/xfs/xfs_file.c
> > > > @@ -1431,8 +1431,25 @@ xfs_file_open(
> > > >  			FMODE_DIO_PARALLEL_WRITE |
> > > > FMODE_CAN_ODIRECT;
> > > > 
> > > >  	error = fsverity_file_open(inode, file);
> > > > -	if (error)
> > > > +	switch (error) {
> > > > +	case -EFBIG:
> > > > +	case -EINVAL:
> > > > +	case -EMSGSIZE:
> > > > +	case -EFSCORRUPTED:
> > > > +		/*
> > > > +		 * Be selective about which fsverity errors we
> > > > propagate to
> > > > +		 * userspace; we still want to be able to open
> > > > this file even
> > > > +		 * if reads don't work.  Someone might want to
> > > > perform an
> > > > +		 * online repair.
> > > > +		 */
> > > > +		if (has_capability_noaudit(current,
> > > > CAP_SYS_ADMIN))
> > > > +			break;
> > > 
> > > As I understand it, fsverity (and dm-verity) are desirable in
> > > high-safety and integrity requirement cases where the goal is for
> > > the
> > > system to "fail closed" if errors in general are detected;
> > > anything
> > > that would have the system be in an ill-defined state.
> > 
> > Is "open() fails if verity metadata are trashed" a hard
> > requirement?
> 
> I can't say authoritatively, but I do want to ensure we've dug into
> the semantics here, and I agree with Eric that it would make the most
> sense to have this be consistent across filesystems.

In terms of userspace I think this semantic change is fine. Even if the
metadata is broken we will still not see any non-validated data. It's
as if we didn't try to use the broken fsverity metadata until it needed
to be used. I agree with others though that having the same behavior
across all filesystems would make sense. Also, it might be useful
information that the filesystem has an error, so maybe we should log
the swallowed errors.

For kernel use, in overlayfs when using verity_mode=require, we do use
open() (in ovl_validate_verity) to trigger the initialization of
fsverity_info . However I took a look at this code, and it seems to
properly handle (i.e. fail) the case where IS_VERITY(inode) is true but
there is no fsverity_info after open.

Similarly, IMA (in ima_get_verity_digest) relies on the digest loaded
from the header. But it also seems to handle this case correctly.

> > Reads will still fail due to (iomap) readahead returning EIO for a
> > file
> > that is IS_VERITY() && !fsverity_active().  This is (afaict) the
> > state
> > you end up with when the fsverity open fails.  ext4/f2fs don't do
> > that,
> > but they also don't have online fsck so once a file's dead it's
> > dead.
> 
> OK, right.  Allowing an open() but having read() fail seems like it
> doesn't weaken things too much in reality.  I think what makes me
> uncomfortable is the error-swallowing; but yes, in theory we should
> get the same or similar error on a subsequent read().

If anything the explicit error list seems a bit fragile to me. What if
the underlying fs reported some new error when reading the metadata,
should we then suddenly fail here when we didn't before? 

>

Dave Chinner April 3, 2024, 10:22 p.m. UTC | #11

On Tue, Apr 02, 2024 at 08:19:10PM -0700, Darrick J. Wong wrote:
> On Wed, Apr 03, 2024 at 12:59:22PM +1100, Dave Chinner wrote:
> > On Tue, Apr 02, 2024 at 06:39:03PM -0700, Darrick J. Wong wrote:
> > > On Tue, Apr 02, 2024 at 08:10:15PM -0400, Colin Walters wrote:
> > > > >> I hesitate to say it but maybe there should be some ioctl for online
> > > > >> repair use cases only, or perhaps a new O_NOVERITY special flag to
> > > > >> openat2()?
> > > > >
> > > > > "openat2 but without meddling from the VFS"?  Tempting... ;)
> > > > 
> > > > Or really any lower level even filesystem-specific API for the online
> > > > fsck case.  Adding a blanket new special case for all CAP_SYS_ADMIN
> > > > processes covers a lot of things that don't need that.
> > > 
> > > I suppose there could be an O_NOVALIDATION to turn off data checksum
> > > validation on btrfs/bcachefs too.  But then you'd want to careful
> > > controls on who gets to use it.  Maybe not liblzma_la-crc64-fast.o.
> > 
> > Just use XFS_IOC_OPEN_BY_HANDLE same as xfs_fsr and xfsdump do. The
> > handle can be build in userspace from the inode bulkstat
> > information, and for typical inode contents verification purposes we
> > don't actually need path-based open access to the inodes. That would
> > then mean we can simple add our own open flag to return a fd that
> > can do data operations that short-circuit verification...
> 
> Heh, ok.  Are there any private flags that get passed via
> xfs_fsop_handlereq_t::oflags?  Or does that mean defining a top level
> O_FLAG that cannot be passed through openat but /can/ be sent via
> open_by_handle?

AIUI, open flags are arch specific, but I don't think any use the
high bits of the 32 bit space they are defined in.  So I think we
could probably use the high bits in that field for our own purposes
and not get conflicts with generic open flags...

-Dave.

Patch

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 9f9d929dfeebc..e68a15b72dbdd 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -487,6 +487,14 @@  static loff_t iomap_readpage_iter(const struct iomap_iter *iter,
 	size_t poff, plen;
 	sector_t sector;
 
+	/*
+	 * If this verity file hasn't been activated, fail read attempts.  This
+	 * can happen if the calling filesystem allows files to be opened even
+	 * with damaged verity metadata.
+	 */
+	if (IS_VERITY(iter->inode) && !fsverity_active(iter->inode))
+		return -EIO;
+
 	if (iomap->type == IOMAP_INLINE)
 		return iomap_read_inline_data(iter, folio);
 
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index c0b3e8146b753..36034eaefbf55 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1431,8 +1431,25 @@  xfs_file_open(
 			FMODE_DIO_PARALLEL_WRITE | FMODE_CAN_ODIRECT;
 
 	error = fsverity_file_open(inode, file);
-	if (error)
+	switch (error) {
+	case -EFBIG:
+	case -EINVAL:
+	case -EMSGSIZE:
+	case -EFSCORRUPTED:
+		/*
+		 * Be selective about which fsverity errors we propagate to
+		 * userspace; we still want to be able to open this file even
+		 * if reads don't work.  Someone might want to perform an
+		 * online repair.
+		 */
+		if (has_capability_noaudit(current, CAP_SYS_ADMIN))
+			break;
 		return error;
+	case 0:
+		break;
+	default:
+		return error;
+	}
 
 	return generic_file_open(inode, file);
 }