| Message ID | 20250210194512.417339-5-mszeredi@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/5] ovl: don't allow datadir only | expand |
On Mon, Feb 10, 2025 at 8:45 PM Miklos Szeredi <mszeredi@redhat.com> wrote: > > Allow the "verity" mount option to be used with "userxattr" data-only > layer(s). This standalone sentence sounds like a security risk, because unpriv users could change the verity digest. I suggest explaining it better. > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> > --- > fs/overlayfs/params.c | 15 +++------------ > 1 file changed, 3 insertions(+), 12 deletions(-) > > diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c > index 54468b2b0fba..7300ed904e6d 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 > @@ -986,10 +981,6 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx, > pr_err("metacopy requires permission to access trusted xattrs\n"); > return -EPERM; > } > - if (config->verity_mode) { > - pr_err("verity requires permission to access trusted xattrs\n"); > - return -EPERM; > - } This looks wrong. I don't think you meant to change the case of (!config->userxattr && !capable(CAP_SYS_ADMIN)) Thanks, Amir.
On Tue, 11 Feb 2025 at 11:50, Amir Goldstein <amir73il@gmail.com> wrote: > > On Mon, Feb 10, 2025 at 8:45 PM Miklos Szeredi <mszeredi@redhat.com> wrote: > > > > Allow the "verity" mount option to be used with "userxattr" data-only > > layer(s). > > This standalone sentence sounds like a security risk, > because unpriv users could change the verity digest. > I suggest explaining it better. Same condition as in previous patch applies: if xattr is on a read-only layer or modification is prevented in any other way, then it's safe. Otherwise no. > > @@ -986,10 +981,6 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx, > > pr_err("metacopy requires permission to access trusted xattrs\n"); > > return -EPERM; > > } > > - if (config->verity_mode) { > > - pr_err("verity requires permission to access trusted xattrs\n"); > > - return -EPERM; > > - } > > This looks wrong. > I don't think you meant to change the case of > (!config->userxattr && !capable(CAP_SYS_ADMIN)) Yep, good catch. Thanks, Miklos
On Tue, Feb 11, 2025 at 1:14 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Tue, 11 Feb 2025 at 11:50, Amir Goldstein <amir73il@gmail.com> wrote: > > > > On Mon, Feb 10, 2025 at 8:45 PM Miklos Szeredi <mszeredi@redhat.com> wrote: > > > > > > Allow the "verity" mount option to be used with "userxattr" data-only > > > layer(s). > > > > This standalone sentence sounds like a security risk, > > because unpriv users could change the verity digest. > > I suggest explaining it better. > > Same condition as in previous patch applies: if xattr is on a > read-only layer or modification is prevented in any other way, then > it's safe. Otherwise no. > Yes, but one has to follow the series to figure out that userxattr means that redirect/metacopy are allowed from lower -> data only, so it is better to mention this again in the context of the commit message that relaxes the requirement. And also even if lower is on a read-only layer, maybe we need to fix the uppermetacpy vector from index to make it safe. Thanks, Amir.
diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c index 54468b2b0fba..7300ed904e6d 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 @@ -986,10 +981,6 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx, pr_err("metacopy requires permission to access trusted xattrs\n"); return -EPERM; } - if (config->verity_mode) { - pr_err("verity requires permission to access trusted xattrs\n"); - return -EPERM; - } if (ctx->nr_data > 0) { pr_err("lower data-only dirs require permission to access trusted xattrs\n"); return -EPERM;
Allow the "verity" mount option to be used with "userxattr" data-only layer(s). Signed-off-by: Miklos Szeredi <mszeredi@redhat.com> --- fs/overlayfs/params.c | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-)