From: Alexander Larsson <alexl@redhat.com>
To: Colin Walters <walters@verbum.org>,
"Darrick J. Wong" <djwong@kernel.org>
Cc: Eric Biggers <ebiggers@kernel.org>,
Andrey Albershteyn <aalbersh@redhat.com>,
xfs <linux-xfs@vger.kernel.org>,
linux-fsdevel@vger.kernel.org, fsverity@lists.linux.dev
Subject: Re: [PATCH 28/29] xfs: allow verity files to be opened even if the fsverity metadata is damaged
Date: Wed, 03 Apr 2024 10:35:30 +0200 [thread overview]
Message-ID: <fffa121a156d92b907136774c41bd83c43e859ca.camel@redhat.com> (raw)
In-Reply-To: <992e84c7-66f5-42d2-a042-9a850891b705@app.fastmail.com>
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 <djwong@kernel.org>
> > > >
> > > > 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@kernel.org>
> > > > ---
> > > > 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?
>
--
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
Alexander Larsson Red Hat,
Inc
alexl@redhat.com alexander.larsson@gmail.com
He's a lonely alcoholic firefighter looking for a cure to the poison
coursing through his veins. She's a tortured insomniac Hell's Angel on
the trail of a serial killer. They fight crime!
next prev parent reply other threads:[~2024-04-03 8:35 UTC|newest]
Thread overview: 111+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-30 0:30 [PATCHBOMB v5.5] fs-verity support for XFS Darrick J. Wong
2024-03-30 0:32 ` [PATCHSET v5.5 1/2] fs-verity: support merkle tree access by blocks Darrick J. Wong
2024-03-30 0:32 ` [PATCH 01/13] fs: add FS_XFLAG_VERITY for verity files Darrick J. Wong
2024-03-30 0:33 ` [PATCH 02/13] fsverity: pass tree_blocksize to end_enable_verity() Darrick J. Wong
2024-03-30 0:33 ` [PATCH 03/13] fsverity: support block-based Merkle tree caching Darrick J. Wong
2024-04-05 2:31 ` Eric Biggers
2024-04-24 21:25 ` Darrick J. Wong
2024-04-24 22:08 ` Eric Biggers
2024-04-25 0:27 ` Darrick J. Wong
2024-04-25 0:46 ` Eric Biggers
2024-04-25 0:53 ` Darrick J. Wong
2024-03-30 0:33 ` [PATCH 04/13] fsverity: add per-sb workqueue for post read processing Darrick J. Wong
2024-04-05 2:39 ` Eric Biggers
2024-04-24 21:33 ` Darrick J. Wong
2024-03-30 0:33 ` [PATCH 05/13] fsverity: add tracepoints Darrick J. Wong
2024-03-30 0:34 ` [PATCH 06/13] fsverity: send the level of the merkle tree block to ->read_merkle_tree_block Darrick J. Wong
2024-04-05 2:42 ` Eric Biggers
2024-04-25 0:30 ` Darrick J. Wong
2024-03-30 0:34 ` [PATCH 07/13] fsverity: pass the new tree size and block size to ->begin_enable_verity Darrick J. Wong
2024-04-05 2:46 ` Eric Biggers
2024-04-24 21:36 ` Darrick J. Wong
2024-03-30 0:34 ` [PATCH 08/13] fsverity: expose merkle tree geometry to callers Darrick J. Wong
2024-04-05 2:50 ` Eric Biggers
2024-04-25 0:45 ` Darrick J. Wong
2024-04-25 0:49 ` Eric Biggers
2024-04-25 1:01 ` Darrick J. Wong
2024-04-25 1:04 ` Eric Biggers
2024-03-30 0:35 ` [PATCH 09/13] fsverity: box up the write_merkle_tree_block parameters too Darrick J. Wong
2024-04-05 2:52 ` Eric Biggers
2024-04-25 0:46 ` Darrick J. Wong
2024-03-30 0:35 ` [PATCH 10/13] fsverity: pass the zero-hash value to the implementation Darrick J. Wong
2024-04-05 2:57 ` Eric Biggers
2024-04-24 19:02 ` Darrick J. Wong
2024-04-24 19:19 ` Eric Biggers
2024-04-24 20:23 ` Darrick J. Wong
2024-04-24 20:59 ` Eric Biggers
2024-04-24 21:43 ` Darrick J. Wong
2024-03-30 0:35 ` [PATCH 11/13] fsverity: report validation errors back to the filesystem Darrick J. Wong
2024-04-05 3:09 ` Eric Biggers
2024-04-24 18:18 ` Darrick J. Wong
2024-04-24 18:52 ` Eric Biggers
2024-04-24 19:03 ` Darrick J. Wong
2024-03-30 0:35 ` [PATCH 12/13] fsverity: remove system-wide workqueue Darrick J. Wong
2024-04-05 3:14 ` Eric Biggers
2024-04-24 18:05 ` Darrick J. Wong
2024-04-24 18:41 ` Eric Biggers
2024-04-29 10:15 ` Andrey Albershteyn
2024-04-29 16:35 ` Darrick J. Wong
2024-03-30 0:36 ` [PATCH 13/13] iomap: integrate fs-verity verification into iomap's read path Darrick J. Wong
2024-03-30 0:32 ` [PATCHSET v5.5 2/2] xfs: fs-verity support Darrick J. Wong
2024-03-30 0:36 ` [PATCH 01/29] xfs: use unsigned ints for non-negative quantities in xfs_attr_remote.c Darrick J. Wong
2024-04-02 9:51 ` Andrey Albershteyn
2024-04-02 16:25 ` Darrick J. Wong
2024-03-30 0:36 ` [PATCH 02/29] xfs: turn XFS_ATTR3_RMT_BUF_SPACE into a function Darrick J. Wong
2024-04-02 10:09 ` Andrey Albershteyn
2024-03-30 0:36 ` [PATCH 03/29] xfs: create a helper to compute the blockcount of a max sized remote value Darrick J. Wong
2024-04-02 10:09 ` Andrey Albershteyn
2024-03-30 0:37 ` [PATCH 04/29] xfs: minor cleanups of xfs_attr3_rmt_blocks Darrick J. Wong
2024-04-02 10:11 ` Andrey Albershteyn
2024-03-30 0:37 ` [PATCH 05/29] xfs: add attribute type for fs-verity Darrick J. Wong
2024-03-30 0:37 ` [PATCH 06/29] xfs: do not use xfs_attr3_rmt_hdr for remote verity value blocks Darrick J. Wong
2024-03-30 0:37 ` [PATCH 07/29] xfs: add fs-verity ro-compat flag Darrick J. Wong
2024-03-30 0:38 ` [PATCH 08/29] xfs: add inode on-disk VERITY flag Darrick J. Wong
2024-03-30 0:38 ` [PATCH 09/29] xfs: initialize fs-verity on file open and cleanup on inode destruction Darrick J. Wong
2024-03-30 0:38 ` [PATCH 10/29] xfs: don't allow to enable DAX on fs-verity sealed inode Darrick J. Wong
2024-03-30 0:38 ` [PATCH 11/29] xfs: disable direct read path for fs-verity files Darrick J. Wong
2024-03-30 0:39 ` [PATCH 12/29] xfs: widen flags argument to the xfs_iflags_* helpers Darrick J. Wong
2024-04-02 12:37 ` Andrey Albershteyn
2024-04-02 16:27 ` Darrick J. Wong
2024-03-30 0:39 ` [PATCH 13/29] xfs: add fs-verity support Darrick J. Wong
2024-04-02 8:42 ` Andrey Albershteyn
2024-04-02 16:34 ` Darrick J. Wong
2024-04-25 1:14 ` Darrick J. Wong
2024-03-30 0:39 ` [PATCH 14/29] xfs: create a per-mount shrinker for verity inodes merkle tree blocks Darrick J. Wong
2024-04-05 3:16 ` Eric Biggers
2024-04-24 17:39 ` Darrick J. Wong
2024-03-30 0:39 ` [PATCH 15/29] xfs: create an icache tag for files with cached " Darrick J. Wong
2024-03-30 0:40 ` [PATCH 16/29] xfs: shrink verity blob cache Darrick J. Wong
2024-03-30 0:40 ` [PATCH 17/29] xfs: only allow the verity iflag for regular files Darrick J. Wong
2024-04-02 12:52 ` Andrey Albershteyn
2024-03-30 0:40 ` [PATCH 18/29] xfs: don't store trailing zeroes of merkle tree blocks Darrick J. Wong
2024-03-30 0:41 ` [PATCH 19/29] xfs: use merkle tree offset as attr hash Darrick J. Wong
2024-03-30 0:41 ` [PATCH 20/29] xfs: don't bother storing merkle tree blocks for zeroed data blocks Darrick J. Wong
2024-03-30 0:41 ` [PATCH 21/29] xfs: add fs-verity ioctls Darrick J. Wong
2024-03-30 0:41 ` [PATCH 22/29] xfs: advertise fs-verity being available on filesystem Darrick J. Wong
2024-04-02 13:44 ` Andrey Albershteyn
2024-03-30 0:42 ` [PATCH 23/29] xfs: make scrub aware of verity dinode flag Darrick J. Wong
2024-03-30 0:42 ` [PATCH 24/29] xfs: teach online repair to evaluate fsverity xattrs Darrick J. Wong
2024-04-02 15:42 ` Andrey Albershteyn
2024-04-02 16:42 ` Darrick J. Wong
2024-03-30 0:42 ` [PATCH 25/29] xfs: report verity failures through the health system Darrick J. Wong
2024-04-02 16:16 ` Andrey Albershteyn
2024-03-30 0:42 ` [PATCH 26/29] xfs: clear the verity iflag when not appropriate Darrick J. Wong
2024-04-02 16:26 ` Andrey Albershteyn
2024-03-30 0:43 ` [PATCH 27/29] xfs: make it possible to disable fsverity Darrick J. Wong
2024-04-02 17:15 ` Andrey Albershteyn
2024-04-02 23:25 ` Eric Biggers
2024-04-03 1:26 ` Darrick J. Wong
2024-03-30 0:43 ` [PATCH 28/29] xfs: allow verity files to be opened even if the fsverity metadata is damaged Darrick J. Wong
2024-04-02 18:04 ` Andrey Albershteyn
2024-04-02 20:00 ` Colin Walters
2024-04-02 22:52 ` Darrick J. Wong
2024-04-02 23:45 ` Eric Biggers
2024-04-03 1:34 ` Darrick J. Wong
2024-04-03 0:10 ` Colin Walters
2024-04-03 1:39 ` Darrick J. Wong
2024-04-03 1:59 ` Dave Chinner
2024-04-03 3:19 ` Darrick J. Wong
2024-04-03 22:22 ` Dave Chinner
2024-04-03 8:35 ` Alexander Larsson [this message]
2024-03-30 0:43 ` [PATCH 29/29] xfs: enable ro-compat fs-verity flag Darrick J. Wong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fffa121a156d92b907136774c41bd83c43e859ca.camel@redhat.com \
--to=alexl@redhat.com \
--cc=aalbersh@redhat.com \
--cc=djwong@kernel.org \
--cc=ebiggers@kernel.org \
--cc=fsverity@lists.linux.dev \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
--cc=walters@verbum.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).