On Sat, Sep 7, 2024 at 5:59 PM Mike Baynton <mike@xxxxxxxxxxxx> wrote:
>
> Hi Amir,
> I apologize for my unfamiliarity with the process. Would you be so kind
> as to point me to the next steps for this patch?
Your next step would be to ping the maintainers ;)
Sorry, as both me and Miklos were on vacation during July,
nobody picked up this patch.
I did skim over the mailing list for missed patches after my vacation,
but I somehow missed it.
I will queue it up and designate it for stable v6.6+.
v6.6 added overlayfs verity feature, but lower datadir was added
already in v6.5.
However, 1. v6.5 is not an LTS kernel, 2. the params.c refactoring
in v6.5 makes it hard to backport beyond v6.6.
Thanks,
Amir.
>
> My team took a wrong turn building on data-only layers on account of my
> not vetting the feature inside user namespaces well enough -- I just
> checked "does it mount and enumerate files successfully." I'm hoping the
> most good that can come from that blunder is saving someone else from
> the same fate in future.
>
> Regards
> Mike
>
> On 7/11/24 08:35, Amir Goldstein wrote:
> > On Thu, Jul 11, 2024 at 7:05 AM Mike Baynton <mike@xxxxxxxxxxxx> wrote:
> >>
> >> Some overlayfs features require permission to read/write trusted.*
> >> xattrs. These include redirect_dir, verity, metacopy, and data-only
> >> layers. This patch adds additional validations at mount time to stop
> >> overlays from mounting in certain cases where the resulting mount would
> >> not function according to the user's expectations because they lack
> >> permission to access trusted.* xattrs (for example, not global root.)
> >>
> >> Similar checks in ovl_make_workdir() that disable features instead of
> >> failing are still relevant and used in cases where the resulting mount
> >> can still work "reasonably well." Generally, if the feature was enabled
> >> through kernel config or module option, any mount that worked before
> >> will still work the same; this applies to redirect_dir and metacopy. The
> >> user must explicitly request these features in order to generate a mount
> >> failure. Verity and data-only layers on the other hand must be explictly
> >> requested and have no "reasonable" disabled or degraded alternative, so
> >> mounts attempting either always fail.
> >>
> >> "lower data-only dirs require metacopy support" moved down in case
> >> userxattr is set, which disables metacopy.
> >>
> >> Signed-off-by: Mike Baynton <mike@xxxxxxxxxxxx>
> >
> > Looks nice
> >
> > Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
> >
> >> ---
> >>
> >> v1 -> v2 not specific to data-only layers, punt on metacopy disable
> >> due to xattr write errors creating a conflicting configuration
> >> when data-only layers are present.
> >>
> >> fs/overlayfs/params.c | 39 +++++++++++++++++++++++++++++++++------
> >> 1 file changed, 33 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
> >> index 4860fcc4611b..107c43e5e4cb 100644
> >> --- a/fs/overlayfs/params.c
> >> +++ b/fs/overlayfs/params.c
> >> @@ -782,11 +782,6 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
> >> {
> >> struct ovl_opt_set set = ctx->set;
> >>
> >> - if (ctx->nr_data > 0 && !config->metacopy) {
> >> - pr_err("lower data-only dirs require metacopy support.\n");
> >> - return -EINVAL;
> >> - }
> >> -
> >> /* Workdir/index are useless in non-upper mount */
> >> if (!config->upperdir) {
> >> if (config->workdir) {
> >> @@ -910,7 +905,6 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
> >> }
> >> }
> >>
> >> -
> >> /* Resolve userxattr -> !redirect && !metacopy && !verity dependency */
> >> if (config->userxattr) {
> >> if (set.redirect &&
> >> @@ -938,6 +932,39 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
> >> config->metacopy = false;
> >> }
> >>
> >> + /*
> >> + * Fail if we don't have trusted xattr capability and a feature was
> >> + * explicitly requested that requires them.
> >> + */
> >> + if (!config->userxattr && !capable(CAP_SYS_ADMIN)) {
> >> + if (set.redirect &&
> >> + config->redirect_mode != OVL_REDIRECT_NOFOLLOW) {
> >> + pr_err("redirect_dir requires permission to access trusted xattrs\n");
> >> + return -EPERM;
> >> + }
> >> + if (config->metacopy && set.metacopy) {
> >> + 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;
> >> + }
> >> + /*
> >> + * Other xattr-dependent features should be disabled without
> >> + * great disturbance to the user in ovl_make_workdir().
> >> + */
> >> + }
> >> +
> >> + if (ctx->nr_data > 0 && !config->metacopy) {
> >> + pr_err("lower data-only dirs require metacopy support.\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> return 0;
> >> }
> >>
> >> --
> >> 2.34.1
> >>
> >>
>
