| Message ID | 20250325104634.162496-6-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ovl: metacopy/verity fixes and improvements | expand |
On Tue, Mar 25, 2025 at 11:46 AM Miklos Szeredi <mszeredi@redhat.com> 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@redhat.com> > --- > 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. Thanks, Amir.
On Tue, Mar 25, 2025 at 12:33 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Tue, Mar 25, 2025 at 11:46 AM Miklos Szeredi <mszeredi@redhat.com> 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@redhat.com> > > --- > > 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??? Thanks, Amir.
On Tue, 2025-03-25 at 12:47 +0100, Amir Goldstein wrote: > On Tue, Mar 25, 2025 at 12:33 PM Amir Goldstein <amir73il@gmail.com> > wrote: > > > > On Tue, Mar 25, 2025 at 11:46 AM Miklos Szeredi > > <mszeredi@redhat.com> 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@redhat.com> > > > --- > > > 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.
On Tue, 2025-03-25 at 11:46 +0100, Miklos Szeredi 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@redhat.com> Reviewed-by: Alexander Larsson <alexl@redhat.com> This works well enough for composefs, but I agree with Amir that once we start allowing redirects into data-only lowers, even with metacopy=0, then we could at least allow verity=on. > --- > 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) { > /* Don't allow explicit specified conflicting > combinations */ > if (set.metacopy) { > pr_err("conflicting options: > metacopy=off,verity=%s\n", > @@ -945,7 +945,7 @@ int ovl_fs_params_verify(const struct > ovl_fs_context *ctx, > } > > > - /* Resolve userxattr -> !redirect && !metacopy && !verity > dependency */ > + /* Resolve userxattr -> !redirect && !metacopy dependency */ > if (config->userxattr) { > if (set.redirect && > config->redirect_mode != OVL_REDIRECT_NOFOLLOW) > { > @@ -957,11 +957,6 @@ int ovl_fs_params_verify(const struct > ovl_fs_context *ctx, > pr_err("conflicting options: > userxattr,metacopy=on\n"); > return -EINVAL; > } > - if (config->verity_mode) { > - pr_err("conflicting options: > userxattr,verity=%s\n", > - ovl_verity_mode(config)); > - return -EINVAL; > - } > /* > * Silently disable default setting of redirect and > metacopy. > * This shall be the default in the future as well: > these
On Tue, 25 Mar 2025 at 12:35, Amir Goldstein <amir73il@gmail.com> wrote: > > --- 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? Yeah, now it's clear that metacopy has little to do with the data redirect feature that verity was added for. I don't really understand the copy-up logic around verity=require, though. Why does that not return EIO like open? Thanks, Miklos
On Wed, 2025-03-26 at 11:24 +0100, Miklos Szeredi wrote: > On Tue, 25 Mar 2025 at 12:35, Amir Goldstein <amir73il@gmail.com> > wrote: > > > > --- 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? > > Yeah, now it's clear that metacopy has little to do with the data > redirect feature that verity was added for. > > I don't really understand the copy-up logic around verity=require, > though. Why does that not return EIO like open? If a lowerdir file doesn't have fsverity enabled, there is no struct fsverity_info, so no digest available to use. This means we cannot make a verity-enforced redirect to it. This is not an VERITY_REQUIRE failure, those are when we find a redirect with a missing digest xattr, but in this case the lower file is a real data file, not a redirect. Note: This actually happens in composefs. We don't use redirect for tiny files (smaller than the redirect xattrs would be), instead we embed them directly in the EROFS image.
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) { /* Don't allow explicit specified conflicting combinations */ if (set.metacopy) { pr_err("conflicting options: metacopy=off,verity=%s\n", @@ -945,7 +945,7 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx, } - /* Resolve userxattr -> !redirect && !metacopy && !verity dependency */ + /* Resolve userxattr -> !redirect && !metacopy dependency */ if (config->userxattr) { if (set.redirect && config->redirect_mode != OVL_REDIRECT_NOFOLLOW) { @@ -957,11 +957,6 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx, pr_err("conflicting options: userxattr,metacopy=on\n"); return -EINVAL; } - if (config->verity_mode) { - pr_err("conflicting options: userxattr,verity=%s\n", - ovl_verity_mode(config)); - return -EINVAL; - } /* * Silently disable default setting of redirect and metacopy. * This shall be the default in the future as well: these
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@redhat.com> --- fs/overlayfs/params.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)