On Tue, 2025-03-25 at 12:47 +0100, Amir Goldstein wrote:
> On Tue, Mar 25, 2025 at 12:33 PM Amir Goldstein <amir73il@xxxxxxxxx>
> wrote:
> >
> > On Tue, Mar 25, 2025 at 11:46 AM Miklos Szeredi
> > <mszeredi@xxxxxxxxxx> wrote:
> > >
> > > Allow the "verity" mount option to be used with "userxattr" data-
> > > only
> > > layer(s).
> > >
> > > Previous patches made sure that with "userxattr" metacopy only
> > > works in the
> > > lower -> data scenario.
> > >
> > > In this scenario the lower (metadata) layer must be secured
> > > against
> > > tampering, in which case the verity checksums contained in this
> > > layer can
> > > ensure integrity of data even in the case of an untrusted data
> > > layer.
> > >
> > > Signed-off-by: Miklos Szeredi <mszeredi@xxxxxxxxxx>
> > > ---
> > > fs/overlayfs/params.c | 11 +++--------
> > > 1 file changed, 3 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
> > > index 54468b2b0fba..8ac0997dca13 100644
> > > --- a/fs/overlayfs/params.c
> > > +++ b/fs/overlayfs/params.c
> > > @@ -846,8 +846,8 @@ int ovl_fs_params_verify(const struct
> > > ovl_fs_context *ctx,
> > > config->uuid = OVL_UUID_NULL;
> > > }
> > >
> > > - /* Resolve verity -> metacopy dependency */
> > > - if (config->verity_mode && !config->metacopy) {
> > > + /* Resolve verity -> metacopy dependency (unless used
> > > with userxattr) */
> > > + if (config->verity_mode && !config->metacopy && !config-
> > > >userxattr) {
> >
> > This is very un-intuitive to me.
> >
> > Why do we need to keep the dependency verity -> metacopy with
> > trusted xattrs?
> >
> > Anyway, I'd like an ACK from composefs guys on this change.
>
> What do you guys think about disallowing the relaxed
> OVL_VERITY_ON mode in case of !metacopy or in case of userxattr?
>
> I am not sure if it makes any sense wrt security, but if user is
> putting their
> trust on the lower layer's immutable content, it feels like this
> content
> should include the verity digests???
In the case of composefs, we will always either pass metacopy or
userxattrs, so this is moot and the patches as-is look good for
composefs.
However, I agree that it is a bit weird. The new behavior is that as
soon as numdatalayer > 0 we following redirects into a data-layer even
if metacopy=0. This is a change from the old behavior which would
previously have thrown an error here. I think this change is safe, but
once we have decided to allow it I don't see any increased risk in also
allowing verity=on in this case.
So, the case you're talking about is: data-only used, verity=on,
metacopy & userxattrs not set.
In this case with the new patch it would (due to numdatalayer check)
allow following redirects into a data layer. This sounds ok to me, but
it does change behavior in other ways than just the verity check (i.e.
it used to error on a redirect). Once we allow this behavior change I
don't see any reason to not also allow verifying the destination digest
(verity=on). This can only result in possible errors on read, and never
grant more rights.
The verity=require case is less clear.
--
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
Alexander Larsson Red Hat,
Inc
alexl@xxxxxxxxxx alexander.larsson@xxxxxxxxx
He's a benighted devious farmboy possessed of the uncanny powers of an
insect. She's a strong-willed blonde stripper fleeing from a Satanic
cult. They fight crime!
