[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().
> <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.
