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 <djwong@xxxxxxxxxx>
> >> >
> >> > 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 <djwong@xxxxxxxxxx>
> >> > ---
> >> > 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
