From: Amir Goldstein <amir73il@gmail.com>
To: Alexander Larsson <alexl@redhat.com>
Cc: Eric Biggers <ebiggers@kernel.org>,
miklos@szeredi.hu, linux-unionfs@vger.kernel.org, tytso@mit.edu,
fsverity@lists.linux.dev
Subject: Re: [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata
Date: Fri, 9 Jun 2023 16:03:23 +0300 [thread overview]
Message-ID: <CAOQ4uxh_y+YO3q7dB=ALCriq31RhapOHGt+jcXTQbOC7iVqYTw@mail.gmail.com> (raw)
In-Reply-To: <CAL7ro1Hqc29w-FuRuoEfcsxiXTnqqwHP73nwvmZRuKVRsz4D9w@mail.gmail.com>
On Mon, May 15, 2023 at 9:15 AM Alexander Larsson <alexl@redhat.com> wrote:
>
> On Sun, May 14, 2023 at 11:00 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Sun, May 14, 2023 at 10:16 PM Eric Biggers <ebiggers@kernel.org> wrote:
> > >
> > > On Wed, May 03, 2023 at 10:51:38AM +0200, Alexander Larsson wrote:
> > > > When resolving lowerdata (lazily or non-lazily) we check the
> > > > overlay.verity xattr on the metadata inode, and if set verify that the
> > > > source lowerdata inode matches it (according to the verity options
> > > > enabled).
> > >
> > > Keep in mind that the lifetime of an inode's fsverity digest is from when it is
> > > first opened to when the inode is evicted from the inode cache.
> > >
> > > If the inode gets evicted from cache and re-instantiated, it could have been
> > > arbitrarily changed.
> > >
> > > Given that, does this verification happen in the right place? I would have
> > > expected it to happen whenever the file is opened, but it seems you do it when
> > > the dentry is looked up instead. Maybe that works too, but I'd appreciate an
> > > explanation.
> >
> > Hmm. I do not think it is wrong because the overlay file cannot be opened before
> > the inode overlay is looked up and fsverity is verified on lookup.
> > In theory, overlay inode with lower could have been instantiated by decode_fh(),
> > but verity=on and nfs_export=on are conflicting options.
> >
> > However, I agree that doing verify check on lookup is a bit too early, as
> > ls -lR will incur the overhead of verifying all file's data even
> > though their data
> > is not accessed in a non-lazy-lower-data scenario.
> >
> > The intuition of doing verity check before file is opened (or copied up)
> > when there is a realfile open is not wrong, it would have gotten rid of the
> > dodgy ovl_ensure_verity_loaded(), but I think that will be a bit harder to
> > implement (not sure).
> >
> > My suggestion for Alexander:
> > - Use ovl_set/test_flag(OVL_VERIFIED, inode) for lazy verify
> > - Implement ovl_maybe_validate_verity() similar to
> > ovl_maybe_lookup_lowerdata()
> > - Implement a helper ovl_verify_lowerdata()
> > that calls them both
> > - Replace the ovl_maybe_lookup_lowerdata() calls with
> > ovl_verify_lowerdata() calls
> >
> > Then before opening (or copy up) a file, it could have either
> > lazy lower data lookup or lazy lower data validate or both (or none).
> >
> > This will not avoid ovl_ensure_verity_loaded(), but it will load fsverity
> > just before it is needed and it is a bit easier to take ovl_inode_lock
> > unconditionally, in those call sites then deeper within copy_up, where
> > ovl_inode_lock is already taken.
> >
> > I *think* this is a good idea, but we won't know until you try it,
> > so please take my suggestion with a grain of salt.
>
> I'll have a look at it in a bit. It would make performance of
> verity=on in the non-lazy-lookup case better.
>
Hi Alex,
Now that lazy lookup is queued for next, I wanted to ask about the
status of your patches.
Is the issue above the only thing you still need to look at?
No rush on my end, just wanted to be in sync.
Thanks,
Amir.
next prev parent reply other threads:[~2023-06-09 13:03 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-03 8:51 [PATCH v2 0/6] ovl: Add support for fs-verity checking of lowerdata Alexander Larsson
2023-05-03 8:51 ` [PATCH v2 1/6] fsverity: Export fsverity_get_digest Alexander Larsson
2023-05-03 8:51 ` [PATCH v2 2/6] ovl: Break out ovl_e_path_real() from ovl_i_path_real() Alexander Larsson
2023-05-03 8:51 ` [PATCH v2 3/6] ovl: Break out ovl_e_path_lowerdata() from ovl_path_lowerdata() Alexander Larsson
2023-05-03 8:51 ` [PATCH v2 4/6] ovl: Add framework for verity support Alexander Larsson
2023-05-03 11:51 ` Amir Goldstein
2023-05-14 19:22 ` Eric Biggers
2023-05-15 5:44 ` Alexander Larsson
2023-05-15 6:00 ` Eric Biggers
2023-05-15 6:46 ` Alexander Larsson
2023-05-03 8:51 ` [PATCH v2 5/6] ovl: Validate verity xattr when resolving lowerdata Alexander Larsson
2023-05-03 15:35 ` Amir Goldstein
2023-05-14 19:16 ` Eric Biggers
2023-05-14 21:00 ` Amir Goldstein
2023-05-15 6:14 ` Alexander Larsson
2023-06-09 13:03 ` Amir Goldstein [this message]
2023-06-10 15:02 ` Alexander Larsson
2023-06-11 11:20 ` Amir Goldstein
2023-06-12 10:32 ` Alexander Larsson
2023-06-16 5:07 ` Amir Goldstein
2023-06-16 5:24 ` Eric Biggers
2023-06-16 5:55 ` Amir Goldstein
2023-06-16 7:50 ` Alexander Larsson
2023-06-16 8:12 ` Amir Goldstein
2023-06-16 8:39 ` Alexander Larsson
2023-06-16 9:27 ` Amir Goldstein
2023-06-16 11:33 ` Alexander Larsson
2023-06-16 12:24 ` Amir Goldstein
2023-06-16 12:28 ` Miklos Szeredi
2023-06-16 13:14 ` Gao Xiang
2023-05-15 6:12 ` Alexander Larsson
2023-05-03 8:51 ` [PATCH v2 6/6] ovl: Handle verity during copy-up Alexander Larsson
2023-05-03 15:33 ` Amir Goldstein
2023-05-14 19:09 ` [PATCH v2 0/6] ovl: Add support for fs-verity checking of lowerdata Eric Biggers
2023-05-14 21:25 ` Amir Goldstein
2023-05-14 22:19 ` Gao Xiang
2023-05-15 5:55 ` Alexander Larsson
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='CAOQ4uxh_y+YO3q7dB=ALCriq31RhapOHGt+jcXTQbOC7iVqYTw@mail.gmail.com' \
--to=amir73il@gmail.com \
--cc=alexl@redhat.com \
--cc=ebiggers@kernel.org \
--cc=fsverity@lists.linux.dev \
--cc=linux-unionfs@vger.kernel.org \
--cc=miklos@szeredi.hu \
--cc=tytso@mit.edu \
/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).