[PATCH 1/5] ovl: don't allow datadir only

[PATCH 1/5] ovl: don't allow datadir only

[Miklos Szeredi]

In theory overlayfs could support upper layer directly referring to a data
layer, but there's no current use case for this.

Originally, when data-only layers were introduced, this wasn't allowed,
only introduced by the "datadir+" feture, but without actually handling
this case, resuting in an Oops.

Fix by disallowing datadir without lowerdir.

Reported-by: Giuseppe Scrivano <gscrivan@redhat.com>
Fixes: 24e16e385f22 ("ovl: add support for appending lowerdirs one by one")
Cc: <stable@vger.kernel.org> # v6.7
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/super.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 86ae6f6da36b..b11094acdd8f 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1137,6 +1137,11 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
 		return ERR_PTR(-EINVAL);
 	}
 
+	if (ctx->nr == ctx->nr_data) {
+		pr_err("at least one non-data lowerdir is required\n");
+		return ERR_PTR(-EINVAL);
+	}
+
 	err = -EINVAL;
 	for (i = 0; i < ctx->nr; i++) {
 		l = &ctx->lower[i];
-- 
2.48.1

[PATCH 2/5] ovl: remove unused forward declaration

[Miklos Szeredi]

From: Giuseppe Scrivano <gscrivan@redhat.com>

The ovl_get_verity_xattr() function was never added only its declaration.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Fixes: 184996e92e86 ("ovl: Validate verity xattr when resolving lowerdata")
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/overlayfs.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 0021e2025020..be86d2ed71d6 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -540,8 +540,6 @@ int ovl_set_metacopy_xattr(struct ovl_fs *ofs, struct dentry *d,
 bool ovl_is_metacopy_dentry(struct dentry *dentry);
 char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int padding);
 int ovl_ensure_verity_loaded(struct path *path);
-int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path,
-			 u8 *digest_buf, int *buf_length);
 int ovl_validate_verity(struct ovl_fs *ofs,
 			struct path *metapath,
 			struct path *datapath);
-- 
2.48.1

Re: [PATCH 2/5] ovl: remove unused forward declaration

[Amir Goldstein]

On Mon, Feb 10, 2025 at 8:45 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> From: Giuseppe Scrivano <gscrivan@redhat.com>
>
> The ovl_get_verity_xattr() function was never added only its declaration.
>
> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
> Fixes: 184996e92e86 ("ovl: Validate verity xattr when resolving lowerdata")
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/overlayfs.h | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 0021e2025020..be86d2ed71d6 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -540,8 +540,6 @@ int ovl_set_metacopy_xattr(struct ovl_fs *ofs, struct dentry *d,
>  bool ovl_is_metacopy_dentry(struct dentry *dentry);
>  char *ovl_get_redirect_xattr(struct ovl_fs *ofs, const struct path *path, int padding);
>  int ovl_ensure_verity_loaded(struct path *path);
> -int ovl_get_verity_xattr(struct ovl_fs *ofs, const struct path *path,
> -                        u8 *digest_buf, int *buf_length);
>  int ovl_validate_verity(struct ovl_fs *ofs,
>                         struct path *metapath,
>                         struct path *datapath);
> --
> 2.48.1
>

[PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Miklos Szeredi]

When overlayfs finds a file with metacopy and/or redirect attributes and
the metacopy and/or redirect features are not enabled, then it refuses to
act on those attributes while also issuing a warning.

There was a slight inconsistency of only warning on an upper metacopy if it
found the next file on the lower layer, while always warning for metacopy
found on a lower layer.

Fix this inconsistency and make the logic more straightforward, pavig the
way for following patches to change when dataredirects are allowed.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/namei.c | 67 +++++++++++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 23 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index be5c65d6f848..da322e9768d1 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1040,6 +1040,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	struct inode *inode = NULL;
 	bool upperopaque = false;
 	char *upperredirect = NULL;
+	bool nextredirect = false;
+	bool nextmetacopy = false;
 	struct dentry *this;
 	unsigned int i;
 	int err;
@@ -1087,8 +1089,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			if (err)
 				goto out_put_upper;
 
-			if (d.metacopy)
+			if (d.metacopy) {
 				uppermetacopy = true;
+				nextmetacopy = true;
+			}
 			metacopy_size = d.metacopy;
 		}
 
@@ -1099,6 +1103,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 				goto out_put_upper;
 			if (d.redirect[0] == '/')
 				poe = roe;
+			nextredirect = true;
 		}
 		upperopaque = d.opaque;
 	}
@@ -1113,6 +1118,29 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	for (i = 0; !d.stop && i < ovl_numlower(poe); i++) {
 		struct ovl_path lower = ovl_lowerstack(poe)[i];
 
+		/*
+		 * Following redirects/metacopy can have security consequences:
+		 * it's like a symlink into the lower layer without the
+		 * permission checks.
+		 *
+		 * This is only a problem if the upper layer is untrusted (e.g
+		 * comes from an USB drive).  This can allow a non-readable file
+		 * or directory to become readable.
+		 *
+		 * Only following redirects when redirects are enabled disables
+		 * this attack vector when not necessary.
+		 */
+		if (nextmetacopy && !ofs->config.metacopy) {
+			pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry);
+			err = -EPERM;
+			goto out_put;
+		}
+		if (nextredirect && !ovl_redirect_follow(ofs)) {
+			pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n", dentry);
+			err = -EPERM;
+			goto out_put;
+		}
+
 		if (!ovl_redirect_follow(ofs))
 			d.last = i == ovl_numlower(poe) - 1;
 		else if (d.is_dir || !ofs->numdatalayer)
@@ -1126,12 +1154,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		if (!this)
 			continue;
 
-		if ((uppermetacopy || d.metacopy) && !ofs->config.metacopy) {
-			dput(this);
-			err = -EPERM;
-			pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry);
-			goto out_put;
-		}
+		if (d.metacopy)
+			nextmetacopy = true;
 
 		/*
 		 * If no origin fh is stored in upper of a merge dir, store fh
@@ -1185,22 +1209,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			ctr++;
 		}
 
-		/*
-		 * Following redirects can have security consequences: it's like
-		 * a symlink into the lower layer without the permission checks.
-		 * This is only a problem if the upper layer is untrusted (e.g
-		 * comes from an USB drive).  This can allow a non-readable file
-		 * or directory to become readable.
-		 *
-		 * Only following redirects when redirects are enabled disables
-		 * this attack vector when not necessary.
-		 */
-		err = -EPERM;
-		if (d.redirect && !ovl_redirect_follow(ofs)) {
-			pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n",
-					    dentry);
-			goto out_put;
-		}
+		if (d.redirect)
+			nextredirect = true;
 
 		if (d.stop)
 			break;
@@ -1218,6 +1228,17 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		ctr++;
 	}
 
+	if (nextmetacopy && !ofs->config.metacopy) {
+		pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry);
+		err = -EPERM;
+		goto out_put;
+	}
+	if (nextredirect && !ovl_redirect_follow(ofs)) {
+		pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n", dentry);
+		err = -EPERM;
+		goto out_put;
+	}
+
 	/*
 	 * For regular non-metacopy upper dentries, there is no lower
 	 * path based lookup, hence ctr will be zero. If a dentry is found
-- 
2.48.1

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Amir Goldstein]

On Mon, Feb 10, 2025 at 8:45 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> When overlayfs finds a file with metacopy and/or redirect attributes and
> the metacopy and/or redirect features are not enabled, then it refuses to
> act on those attributes while also issuing a warning.
>
> There was a slight inconsistency of only warning on an upper metacopy if it
> found the next file on the lower layer, while always warning for metacopy
> found on a lower layer.
>
> Fix this inconsistency and make the logic more straightforward, pavig the
> way for following patches to change when dataredirects are allowed.
>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/overlayfs/namei.c | 67 +++++++++++++++++++++++++++++---------------
>  1 file changed, 44 insertions(+), 23 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index be5c65d6f848..da322e9768d1 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -1040,6 +1040,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>         struct inode *inode = NULL;
>         bool upperopaque = false;
>         char *upperredirect = NULL;
> +       bool nextredirect = false;
> +       bool nextmetacopy = false;
>         struct dentry *this;
>         unsigned int i;
>         int err;
> @@ -1087,8 +1089,10 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                         if (err)
>                                 goto out_put_upper;
>
> -                       if (d.metacopy)
> +                       if (d.metacopy) {
>                                 uppermetacopy = true;
> +                               nextmetacopy = true;
> +                       }
>                         metacopy_size = d.metacopy;
>                 }
>
> @@ -1099,6 +1103,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                                 goto out_put_upper;
>                         if (d.redirect[0] == '/')
>                                 poe = roe;
> +                       nextredirect = true;
>                 }
>                 upperopaque = d.opaque;
>         }
> @@ -1113,6 +1118,29 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>         for (i = 0; !d.stop && i < ovl_numlower(poe); i++) {
>                 struct ovl_path lower = ovl_lowerstack(poe)[i];
>
> +               /*
> +                * Following redirects/metacopy can have security consequences:
> +                * it's like a symlink into the lower layer without the
> +                * permission checks.
> +                *
> +                * This is only a problem if the upper layer is untrusted (e.g
> +                * comes from an USB drive).  This can allow a non-readable file
> +                * or directory to become readable.
> +                *
> +                * Only following redirects when redirects are enabled disables
> +                * this attack vector when not necessary.
> +                */

What do you say about moving this comment outside the loop and leaving here
only:

    /* Should redirects/metacopy to lower layers be followed? */
    if ((nextmetacopy && !ofs->config.metacopy) ||
        (nextredirect && !ovl_redirect_follow(ofs)))
          break;

> +               if (nextmetacopy && !ofs->config.metacopy) {
> +                       pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry);
> +                       err = -EPERM;
> +                       goto out_put;
> +               }
> +               if (nextredirect && !ovl_redirect_follow(ofs)) {
> +                       pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n", dentry);
> +                       err = -EPERM;
> +                       goto out_put;
> +               }
> +
>                 if (!ovl_redirect_follow(ofs))
>                         d.last = i == ovl_numlower(poe) - 1;
>                 else if (d.is_dir || !ofs->numdatalayer)
> @@ -1126,12 +1154,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 if (!this)
>                         continue;
>
> -               if ((uppermetacopy || d.metacopy) && !ofs->config.metacopy) {
> -                       dput(this);
> -                       err = -EPERM;
> -                       pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry);
> -                       goto out_put;
> -               }
> +               if (d.metacopy)
> +                       nextmetacopy = true;
>
>                 /*
>                  * If no origin fh is stored in upper of a merge dir, store fh
> @@ -1185,22 +1209,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                         ctr++;
>                 }
>
> -               /*
> -                * Following redirects can have security consequences: it's like
> -                * a symlink into the lower layer without the permission checks.
> -                * This is only a problem if the upper layer is untrusted (e.g
> -                * comes from an USB drive).  This can allow a non-readable file
> -                * or directory to become readable.
> -                *
> -                * Only following redirects when redirects are enabled disables
> -                * this attack vector when not necessary.
> -                */
> -               err = -EPERM;
> -               if (d.redirect && !ovl_redirect_follow(ofs)) {
> -                       pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n",
> -                                           dentry);
> -                       goto out_put;
> -               }
> +               if (d.redirect)
> +                       nextredirect = true;
>
>                 if (d.stop)
>                         break;
> @@ -1218,6 +1228,17 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 ctr++;
>         }
>
> +       if (nextmetacopy && !ofs->config.metacopy) {
> +               pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry);
> +               err = -EPERM;
> +               goto out_put;
> +       }
> +       if (nextredirect && !ovl_redirect_follow(ofs)) {
> +               pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n", dentry);
> +               err = -EPERM;
> +               goto out_put;
> +       }
> +
>         /*
>          * For regular non-metacopy upper dentries, there is no lower
>          * path based lookup, hence ctr will be zero. If a dentry is found
> --
> 2.48.1
>

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Miklos Szeredi]

On Tue, 11 Feb 2025 at 12:13, Amir Goldstein <amir73il@gmail.com> wrote:

> What do you say about moving this comment outside the loop and leaving here
> only:
>
>     /* Should redirects/metacopy to lower layers be followed? */
>     if ((nextmetacopy && !ofs->config.metacopy) ||
>         (nextredirect && !ovl_redirect_follow(ofs)))
>           break;

Nice idea, except it would break the next patch.

I don't like the duplication either, maybe move
nextmetacopy/nextredirect into ovl_lookup_data and create a helper?

Thanks,
Miklos

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Amir Goldstein]

On Tue, Feb 11, 2025 at 12:46 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, 11 Feb 2025 at 12:13, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > What do you say about moving this comment outside the loop and leaving here
> > only:
> >
> >     /* Should redirects/metacopy to lower layers be followed? */
> >     if ((nextmetacopy && !ofs->config.metacopy) ||
> >         (nextredirect && !ovl_redirect_follow(ofs)))
> >           break;
>
> Nice idea, except it would break the next patch.

Really? I looked at the next patch before suggesting this
I did not see the breakage. Can you point it out?

BTW, this patch is adding consistency to following upperredirect
but the case of upperredirect and uppermetacopy read from
index still does not check metacopy/redirect config.

Looking closer at ovl_maybe_validate_verity(), it's actually
worse - if you create an upper without metacopy above
a lower with metacopy, ovl_validate_verity() will only check
the metacopy xattr on metapath, which is the uppermost
and find no md5digest, so create an upper above a metacopy
lower is a way to avert verity check.

So I think lookup code needs to disallow finding metacopy
in middle layer and need to enforce that also when upper is found
via index.

Thanks,
Amir.

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Miklos Szeredi]

On Tue, 11 Feb 2025 at 13:01, Amir Goldstein <amir73il@gmail.com> wrote:

> Really? I looked at the next patch before suggesting this
> I did not see the breakage. Can you point it out?

When lookup "falls off" of the normal lower layers while in metacopy
mode with an absolute redirect, then it jumps to the data-only layers.
The above suggestion imitates this falling off when it's really a
permission problem, which would result in weird behavior, AFAICS.

> BTW, this patch is adding consistency to following upperredirect
> but the case of upperredirect and uppermetacopy read from
> index still does not check metacopy/redirect config.

True.  Also that case should probably "loop back" to verifying that
the redirection indeed results in the same origin as pointed to by the
index, right?

> Looking closer at ovl_maybe_validate_verity(), it's actually
> worse - if you create an upper without metacopy above
> a lower with metacopy, ovl_validate_verity() will only check
> the metacopy xattr on metapath, which is the uppermost
> and find no md5digest, so create an upper above a metacopy
> lower is a way to avert verity check.

I need to dig into how verity is supposed to work as I'm not seeing it
clearly yet...

> So I think lookup code needs to disallow finding metacopy
> in middle layer and need to enforce that also when upper is found
> via index.

That's the hard link case.  I.e. with metacopy=on,index=on it's
possible that one link is metacopyied up, and the other one is then
found through the index.  Metacopy *should* work in this case, no?

Thanks,
Miklos

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Amir Goldstein]

On Tue, Feb 11, 2025 at 1:34 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, 11 Feb 2025 at 13:01, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > Really? I looked at the next patch before suggesting this
> > I did not see the breakage. Can you point it out?
>
> When lookup "falls off" of the normal lower layers while in metacopy
> mode with an absolute redirect, then it jumps to the data-only layers.
> The above suggestion imitates this falling off when it's really a
> permission problem, which would result in weird behavior, AFAICS.
>

Yes, I see it now.
I don't have a better idea at the moment.

> > BTW, this patch is adding consistency to following upperredirect
> > but the case of upperredirect and uppermetacopy read from
> > index still does not check metacopy/redirect config.
>
> True.  Also that case should probably "loop back" to verifying that
> the redirection indeed results in the same origin as pointed to by the
> index, right?
>

It sounds very complicated. Is that even possible?
Do we always know the path of the upper alias?
IIRC, the absolute redirect path in upper is not necessary
the absolute path where the origin is found.
e.g. if there are middle layer redirects of parents.

> > Looking closer at ovl_maybe_validate_verity(), it's actually
> > worse - if you create an upper without metacopy above
> > a lower with metacopy, ovl_validate_verity() will only check
> > the metacopy xattr on metapath, which is the uppermost
> > and find no md5digest, so create an upper above a metacopy
> > lower is a way to avert verity check.
>
> I need to dig into how verity is supposed to work as I'm not seeing it
> clearly yet...
>

The short version - for lazy data lookup we store the lowerdata
redirect absolute path in the ovl entry stack, but we do not store
the verity digest, we just store OVL_HAS_DIGEST inode flag if there
is a digest in metacopy xattr.

If we store the digest from lookup time in ovl entry stack, your changes
may be easier.

> > So I think lookup code needs to disallow finding metacopy
> > in middle layer and need to enforce that also when upper is found
> > via index.
>
> That's the hard link case.  I.e. with metacopy=on,index=on it's
> possible that one link is metacopyied up, and the other one is then
> found through the index.  Metacopy *should* work in this case, no?
>

Right. So I guess we only need to disallow uppermetacopy from
index when metacoy=off.

Thanks,
Amir.

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Miklos Szeredi]

On Tue, 11 Feb 2025 at 16:52, Amir Goldstein <amir73il@gmail.com> wrote:

> It sounds very complicated. Is that even possible?
> Do we always know the path of the upper alias?
> IIRC, the absolute redirect path in upper is not necessary
> the absolute path where the origin is found.
> e.g. if there are middle layer redirects of parents.

Okay, it was a stupid idea.

> > > Looking closer at ovl_maybe_validate_verity(), it's actually
> > > worse - if you create an upper without metacopy above
> > > a lower with metacopy, ovl_validate_verity() will only check
> > > the metacopy xattr on metapath, which is the uppermost
> > > and find no md5digest, so create an upper above a metacopy
> > > lower is a way to avert verity check.
> >
> > I need to dig into how verity is supposed to work as I'm not seeing it
> > clearly yet...
> >
>
> The short version - for lazy data lookup we store the lowerdata
> redirect absolute path in the ovl entry stack, but we do not store
> the verity digest, we just store OVL_HAS_DIGEST inode flag if there
> is a digest in metacopy xattr.
>
> If we store the digest from lookup time in ovl entry stack, your changes
> may be easier.

Sorry, I can't wrap my head around this issue.  Cc-ing Giuseppe.

> > > So I think lookup code needs to disallow finding metacopy
> > > in middle layer and need to enforce that also when upper is found
> > > via index.
> >
> > That's the hard link case.  I.e. with metacopy=on,index=on it's
> > possible that one link is metacopyied up, and the other one is then
> > found through the index.  Metacopy *should* work in this case, no?
> >
>
> Right. So I guess we only need to disallow uppermetacopy from
> index when metacoy=off.

Yes.

Thanks,
Miklos

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Giuseppe Scrivano]

Miklos Szeredi <miklos@szeredi.hu> writes:

> On Tue, 11 Feb 2025 at 16:52, Amir Goldstein <amir73il@gmail.com> wrote:
>
>> It sounds very complicated. Is that even possible?
>> Do we always know the path of the upper alias?
>> IIRC, the absolute redirect path in upper is not necessary
>> the absolute path where the origin is found.
>> e.g. if there are middle layer redirects of parents.
>
> Okay, it was a stupid idea.
>
>> > > Looking closer at ovl_maybe_validate_verity(), it's actually
>> > > worse - if you create an upper without metacopy above
>> > > a lower with metacopy, ovl_validate_verity() will only check
>> > > the metacopy xattr on metapath, which is the uppermost
>> > > and find no md5digest, so create an upper above a metacopy
>> > > lower is a way to avert verity check.
>> >
>> > I need to dig into how verity is supposed to work as I'm not seeing it
>> > clearly yet...
>> >
>>
>> The short version - for lazy data lookup we store the lowerdata
>> redirect absolute path in the ovl entry stack, but we do not store
>> the verity digest, we just store OVL_HAS_DIGEST inode flag if there
>> is a digest in metacopy xattr.
>>
>> If we store the digest from lookup time in ovl entry stack, your changes
>> may be easier.
>
> Sorry, I can't wrap my head around this issue.  Cc-ing Giuseppe.
>
>> > > So I think lookup code needs to disallow finding metacopy
>> > > in middle layer and need to enforce that also when upper is found
>> > > via index.
>> >
>> > That's the hard link case.  I.e. with metacopy=on,index=on it's
>> > possible that one link is metacopyied up, and the other one is then
>> > found through the index.  Metacopy *should* work in this case, no?
>> >
>>
>> Right. So I guess we only need to disallow uppermetacopy from
>> index when metacoy=off.

is that be safe from a user namespace?

Regards,
Giuseppe

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Miklos Szeredi]

On Thu, 20 Feb 2025 at 10:54, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>
> Miklos Szeredi <miklos@szeredi.hu> writes:
>
> > On Tue, 11 Feb 2025 at 16:52, Amir Goldstein <amir73il@gmail.com> wrote:

> >> The short version - for lazy data lookup we store the lowerdata
> >> redirect absolute path in the ovl entry stack, but we do not store
> >> the verity digest, we just store OVL_HAS_DIGEST inode flag if there
> >> is a digest in metacopy xattr.
> >>
> >> If we store the digest from lookup time in ovl entry stack, your changes
> >> may be easier.
> >
> > Sorry, I can't wrap my head around this issue.  Cc-ing Giuseppe.

Giuseppe, can you describe what should happen when verity is enabled
and a file on a composefs setup is copied up?

> >> Right. So I guess we only need to disallow uppermetacopy from
> >> index when metacoy=off.
>
> is that be safe from a user namespace?

You mean disallowing uppermetacopy?  It's obviously safer than allowing it, no?

Thanks,
Miklos

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Giuseppe Scrivano]

Miklos Szeredi <miklos@szeredi.hu> writes:

> On Thu, 20 Feb 2025 at 10:54, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>>
>> Miklos Szeredi <miklos@szeredi.hu> writes:
>>
>> > On Tue, 11 Feb 2025 at 16:52, Amir Goldstein <amir73il@gmail.com> wrote:
>
>> >> The short version - for lazy data lookup we store the lowerdata
>> >> redirect absolute path in the ovl entry stack, but we do not store
>> >> the verity digest, we just store OVL_HAS_DIGEST inode flag if there
>> >> is a digest in metacopy xattr.
>> >>
>> >> If we store the digest from lookup time in ovl entry stack, your changes
>> >> may be easier.
>> >
>> > Sorry, I can't wrap my head around this issue.  Cc-ing Giuseppe.
>
> Giuseppe, can you describe what should happen when verity is enabled
> and a file on a composefs setup is copied up?

we don't care much about this case since the composefs metadata is in
the EROFS file system.  Once copied up it is fine to discard this
information.  Adding Alex to the discussion as he might have a different
opinion/use case in mind.

>> >> Right. So I guess we only need to disallow uppermetacopy from
>> >> index when metacoy=off.
>>
>> is that be safe from a user namespace?
>
> You mean disallowing uppermetacopy?  It's obviously safer than allowing it, no?

sorry I read th "only need" as "loosening the conditions when
uppermetacopy is allowed"; so I was asking if there are cases when
uppermetacopy is considered safe in a user namespace (if there are any).
If that is not the case, please ignore my question.

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Miklos Szeredi]

On Thu, 20 Feb 2025 at 12:39, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>
> Miklos Szeredi <miklos@szeredi.hu> writes:
>
> > On Thu, 20 Feb 2025 at 10:54, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> >>
> >> Miklos Szeredi <miklos@szeredi.hu> writes:
> >>
> >> > On Tue, 11 Feb 2025 at 16:52, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> >> >> The short version - for lazy data lookup we store the lowerdata
> >> >> redirect absolute path in the ovl entry stack, but we do not store
> >> >> the verity digest, we just store OVL_HAS_DIGEST inode flag if there
> >> >> is a digest in metacopy xattr.
> >> >>
> >> >> If we store the digest from lookup time in ovl entry stack, your changes
> >> >> may be easier.
> >> >
> >> > Sorry, I can't wrap my head around this issue.  Cc-ing Giuseppe.
> >
> > Giuseppe, can you describe what should happen when verity is enabled
> > and a file on a composefs setup is copied up?
>
> we don't care much about this case since the composefs metadata is in
> the EROFS file system.  Once copied up it is fine to discard this
> information.  Adding Alex to the discussion as he might have a different
> opinion/use case in mind.

Okay.

Amir, do I understand correctly that your worry is that after copy-up
verity digest is still being used?  If that's the case, we just need
to make sure that OVL_HAS_DIGEST is cleared on copy-up?

Or am I still misunderstanding this completely?

> >> >> Right. So I guess we only need to disallow uppermetacopy from
> >> >> index when metacoy=off.
> >>
> >> is that be safe from a user namespace?
> >
> > You mean disallowing uppermetacopy?  It's obviously safer than allowing it, no?
>
> sorry I read th "only need" as "loosening the conditions when
> uppermetacopy is allowed"; so I was asking if there are cases when
> uppermetacopy is considered safe in a user namespace (if there are any).
> If that is not the case, please ignore my question.

Yeah, that "only" was referring to my stupid idea, I guess.

Thanks,
Miklos

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Amir Goldstein]

On Thu, Feb 20, 2025 at 12:48 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Thu, 20 Feb 2025 at 12:39, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> >
> > Miklos Szeredi <miklos@szeredi.hu> writes:
> >
> > > On Thu, 20 Feb 2025 at 10:54, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> > >>
> > >> Miklos Szeredi <miklos@szeredi.hu> writes:
> > >>
> > >> > On Tue, 11 Feb 2025 at 16:52, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > >> >> The short version - for lazy data lookup we store the lowerdata
> > >> >> redirect absolute path in the ovl entry stack, but we do not store
> > >> >> the verity digest, we just store OVL_HAS_DIGEST inode flag if there
> > >> >> is a digest in metacopy xattr.
> > >> >>
> > >> >> If we store the digest from lookup time in ovl entry stack, your changes
> > >> >> may be easier.
> > >> >
> > >> > Sorry, I can't wrap my head around this issue.  Cc-ing Giuseppe.
> > >
> > > Giuseppe, can you describe what should happen when verity is enabled
> > > and a file on a composefs setup is copied up?
> >
> > we don't care much about this case since the composefs metadata is in
> > the EROFS file system.  Once copied up it is fine to discard this
> > information.  Adding Alex to the discussion as he might have a different
> > opinion/use case in mind.
>
> Okay.
>
> Amir, do I understand correctly that your worry is that after copy-up
> verity digest is still being used?  If that's the case, we just need
> to make sure that OVL_HAS_DIGEST is cleared on copy-up?
>
> Or am I still misunderstanding this completely?

Sorry, I have somehow missed this email.

TBH, I am not sure what is expected to happen in the use case in question
on copy up - that is if a full copy up on any metadata change is acceptable.

Technically, we could allow a metacopy upper as long as we take the md5digest
from the middle layer but that complicates things and I am not sure if we need
to care - can't wrap my head around this case either.

Thanks,
Amir.

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Miklos Szeredi]

On Tue, 25 Mar 2025 at 13:16, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Thu, Feb 20, 2025 at 12:48 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Thu, 20 Feb 2025 at 12:39, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> > >
> > > Miklos Szeredi <miklos@szeredi.hu> writes:
> > >
> > > > On Thu, 20 Feb 2025 at 10:54, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> > > >>
> > > >> Miklos Szeredi <miklos@szeredi.hu> writes:
> > > >>
> > > >> > On Tue, 11 Feb 2025 at 16:52, Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > >> >> The short version - for lazy data lookup we store the lowerdata
> > > >> >> redirect absolute path in the ovl entry stack, but we do not store
> > > >> >> the verity digest, we just store OVL_HAS_DIGEST inode flag if there
> > > >> >> is a digest in metacopy xattr.
> > > >> >>
> > > >> >> If we store the digest from lookup time in ovl entry stack, your changes
> > > >> >> may be easier.
> > > >> >
> > > >> > Sorry, I can't wrap my head around this issue.  Cc-ing Giuseppe.
> > > >
> > > > Giuseppe, can you describe what should happen when verity is enabled
> > > > and a file on a composefs setup is copied up?
> > >
> > > we don't care much about this case since the composefs metadata is in
> > > the EROFS file system.  Once copied up it is fine to discard this
> > > information.  Adding Alex to the discussion as he might have a different
> > > opinion/use case in mind.
> >
> > Okay.
> >
> > Amir, do I understand correctly that your worry is that after copy-up
> > verity digest is still being used?  If that's the case, we just need
> > to make sure that OVL_HAS_DIGEST is cleared on copy-up?
> >
> > Or am I still misunderstanding this completely?
>
> Sorry, I have somehow missed this email.
>
> TBH, I am not sure what is expected to happen in the use case in question
> on copy up - that is if a full copy up on any metadata change is acceptable.
>
> Technically, we could allow a metacopy upper as long as we take the md5digest
> from the middle layer but that complicates things and I am not sure if we need
> to care - can't wrap my head around this case either.

I've been thinking.  If a lower file has verity enabled, and it is
meta-copied up on ovl with verity=on (or verity=require), then it will
have the digest stored in the .overlay.metacopy xattr. What this
ensures is that the lower file cannot be swapped out without ovl
noticing.  However the .overlay.origin xattr ensures the same thing,
so as long as the user is unable to change the origin integrity should
be guaranteed.  IOW, what we need is just to always check origin on
metacopy regardless of the index option.

But I'm not even sure this is used at all, since the verity code was
added for the composefs use case, which does not use this path AFAICS.
Alex, can you clarify?

(BTW, that origin check could be simplified for non-dir, since we only
need to compare origin_path->dentry->d_inode to this->d_inode.)

Thanks,
Miklos

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Amir Goldstein]

On Thu, Mar 27, 2025 at 4:28 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, 25 Mar 2025 at 13:16, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Thu, Feb 20, 2025 at 12:48 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Thu, 20 Feb 2025 at 12:39, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> > > >
> > > > Miklos Szeredi <miklos@szeredi.hu> writes:
> > > >
> > > > > On Thu, 20 Feb 2025 at 10:54, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> > > > >>
> > > > >> Miklos Szeredi <miklos@szeredi.hu> writes:
> > > > >>
> > > > >> > On Tue, 11 Feb 2025 at 16:52, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > >
> > > > >> >> The short version - for lazy data lookup we store the lowerdata
> > > > >> >> redirect absolute path in the ovl entry stack, but we do not store
> > > > >> >> the verity digest, we just store OVL_HAS_DIGEST inode flag if there
> > > > >> >> is a digest in metacopy xattr.
> > > > >> >>
> > > > >> >> If we store the digest from lookup time in ovl entry stack, your changes
> > > > >> >> may be easier.
> > > > >> >
> > > > >> > Sorry, I can't wrap my head around this issue.  Cc-ing Giuseppe.
> > > > >
> > > > > Giuseppe, can you describe what should happen when verity is enabled
> > > > > and a file on a composefs setup is copied up?
> > > >
> > > > we don't care much about this case since the composefs metadata is in
> > > > the EROFS file system.  Once copied up it is fine to discard this
> > > > information.  Adding Alex to the discussion as he might have a different
> > > > opinion/use case in mind.
> > >
> > > Okay.
> > >
> > > Amir, do I understand correctly that your worry is that after copy-up
> > > verity digest is still being used?  If that's the case, we just need
> > > to make sure that OVL_HAS_DIGEST is cleared on copy-up?
> > >
> > > Or am I still misunderstanding this completely?
> >
> > Sorry, I have somehow missed this email.
> >
> > TBH, I am not sure what is expected to happen in the use case in question
> > on copy up - that is if a full copy up on any metadata change is acceptable.
> >
> > Technically, we could allow a metacopy upper as long as we take the md5digest
> > from the middle layer but that complicates things and I am not sure if we need
> > to care - can't wrap my head around this case either.
>
> I've been thinking.  If a lower file has verity enabled, and it is
> meta-copied up on ovl with verity=on (or verity=require), then it will
> have the digest stored in the .overlay.metacopy xattr. What this
> ensures is that the lower file cannot be swapped out without ovl
> noticing.

Do you mean the lowerdata file?

> However the .overlay.origin xattr ensures the same thing,

origin xattr only checks from upper to uppermost lower layer IIRC,
do definitely not all the way to lowerdata inode.

> so as long as the user is unable to change the origin integrity should
> be guaranteed.  IOW, what we need is just to always check origin on
> metacopy regardless of the index option.
>
> But I'm not even sure this is used at all, since the verity code was
> added for the composefs use case, which does not use this path AFAICS.
> Alex, can you clarify?

I am not sure how composefs lowerdata layer is being deployed,
but but I am pretty sure that the composefs erofs layers are
designed to be migratable to any fs where the lowerdata repo
exists, so I think hard coding the lowerdata inode is undesired.
But probably I did not understand what you meant?

Thanks,
Amir.

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Miklos Szeredi]

On Thu, 27 Mar 2025 at 18:14, Amir Goldstein <amir73il@gmail.com> wrote:
> origin xattr only checks from upper to uppermost lower layer IIRC,
> do definitely not all the way to lowerdata inode.

Makes sense.

> > so as long as the user is unable to change the origin integrity should
> > be guaranteed.  IOW, what we need is just to always check origin on
> > metacopy regardless of the index option.
> >
> > But I'm not even sure this is used at all, since the verity code was
> > added for the composefs use case, which does not use this path AFAICS.
> > Alex, can you clarify?
>
> I am not sure how composefs lowerdata layer is being deployed,
> but but I am pretty sure that the composefs erofs layers are
> designed to be migratable to any fs where the lowerdata repo
> exists, so I think hard coding the lowerdata inode is undesired.

Yeah, I understand the basic composefs architecture, and storing the
digest in the metadata inode makes perfect sense.

What I'm not sure is what is being used outside of that.

Anyway, I don't see any issue with the current architecture, just
trying to understand what this is useful for and possible
simplifications based on that.

For example the copy-up code is apparently unused, and could be
removed.  OTOH it could be useful for the idmapping case from
Guiseppe.

Thanks,
Miklos

Thanks,
Miklos

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Alexander Larsson]

On Thu, 2025-03-27 at 20:23 +0100, Miklos Szeredi wrote:
> On Thu, 27 Mar 2025 at 18:14, Amir Goldstein <amir73il@gmail.com>
> wrote:
> > origin xattr only checks from upper to uppermost lower layer IIRC,
> > do definitely not all the way to lowerdata inode.
> 
> Makes sense.
> 
> > > so as long as the user is unable to change the origin integrity
> > > should
> > > be guaranteed.  IOW, what we need is just to always check origin
> > > on
> > > metacopy regardless of the index option.
> > > 
> > > But I'm not even sure this is used at all, since the verity code
> > > was
> > > added for the composefs use case, which does not use this path
> > > AFAICS.
> > > Alex, can you clarify?
> > 
> > I am not sure how composefs lowerdata layer is being deployed,
> > but but I am pretty sure that the composefs erofs layers are
> > designed to be migratable to any fs where the lowerdata repo
> > exists, so I think hard coding the lowerdata inode is undesired.
> 
> Yeah, I understand the basic composefs architecture, and storing the
> digest in the metadata inode makes perfect sense.
> 
> What I'm not sure is what is being used outside of that.
> 
> Anyway, I don't see any issue with the current architecture, just
> trying to understand what this is useful for and possible
> simplifications based on that.
> 
> For example the copy-up code is apparently unused, and could be
> removed.  OTOH it could be useful for the idmapping case from
> Guiseppe.

I think there are two basic composefs usecases, first a completely
read-only one with a data-only, an erofs lower and nothing more. The
traditional example here is a read-only rootfs. In this case, clearly
digest copy-up is not needed.

The second usecase is when you use composefs for a container image,
similar to use case 1, but on top of that you have the writable upper
layer that is for the running container itself. In this case, you want
to validate all accesses to the lower layer, but allow the container to
make changes. Obviously, once you create a new file, or modify a lower
one there will not be any validation of that file. 

However, if you for example change just file ownership, then you may
trigger a meta-copy-up, and at this point it makes sense to also copy
up the digest to the metacopy file, because otherwise accesses to it
would read the datadir file without validating its digest. 

Unfortunately this (as you say) weakens the security in the case the
raw upperdir is not trusted, as it would allow the digest xattr to be
changed. But I think this is acceptable, because the alternative
without meta-copy-up is a full copy up, but then you can change the
file data in the upper instead, which is even worse.

As for origin checks, they are really never interesting to any
composefs-style usecase, because those are fundamentally about
transporting images between different systems (with different
filesystems, inodes, etc).


-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a superhumanly strong coffee-fuelled firefighter who hides his 
scarred face behind a mask. She's a sarcastic mute opera singer who 
inherited a spooky stately manor from her late maiden aunt. They fight 
crime! 

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Colin Walters]

On Thu, Mar 27, 2025, at 1:13 PM, Amir Goldstein wrote:
> 
> I am not sure how composefs lowerdata layer is being deployed,
> but but I am pretty sure that the composefs erofs layers are
> designed to be migratable to any fs where the lowerdata repo
> exists, so I think hard coding the lowerdata inode is undesired.

Yes, I don't see any security benefit to binding to specific lower inodes, and it reduces flexibility to do so. A nice feature of composefs is that underneath it's "just files" and all tools and techniques for managing files apply.

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Miklos Szeredi]

On Tue, 11 Feb 2025 at 13:01, Amir Goldstein <amir73il@gmail.com> wrote:
> Looking closer at ovl_maybe_validate_verity(), it's actually
> worse - if you create an upper without metacopy above
> a lower with metacopy, ovl_validate_verity() will only check
> the metacopy xattr on metapath, which is the uppermost
> and find no md5digest, so create an upper above a metacopy
> lower is a way to avert verity check.
>
> So I think lookup code needs to disallow finding metacopy
> in middle layer and need to enforce that also when upper is found
> via index.

So I think the next patch does this: only allow following a metacopy
redirect from lower to data.

It's confusing to call this metacopy, as no copy is performed.  We
could call it data-redirect.  Mixing data-redirect with real meta-copy
is of dubious value, and we might be better to disable it even in the
privileged scenario.

Giuseppe, Alexander, AFAICS the composefs use case employs
data-redirect only and not metacopy, right?

Thanks,
Miklos

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Alexander Larsson]

On Tue, 2025-03-25 at 11:57 +0100, Miklos Szeredi wrote:
> On Tue, 11 Feb 2025 at 13:01, Amir Goldstein <amir73il@gmail.com>
> wrote:
> > Looking closer at ovl_maybe_validate_verity(), it's actually
> > worse - if you create an upper without metacopy above
> > a lower with metacopy, ovl_validate_verity() will only check
> > the metacopy xattr on metapath, which is the uppermost
> > and find no md5digest, so create an upper above a metacopy
> > lower is a way to avert verity check.
> > 
> > So I think lookup code needs to disallow finding metacopy
> > in middle layer and need to enforce that also when upper is found
> > via index.
> 
> So I think the next patch does this: only allow following a metacopy
> redirect from lower to data.
> 
> It's confusing to call this metacopy, as no copy is performed.  We
> could call it data-redirect.  Mixing data-redirect with real meta-
> copy
> is of dubious value, and we might be better to disable it even in the
> privileged scenario.
> 
> Giuseppe, Alexander, AFAICS the composefs use case employs
> data-redirect only and not metacopy, right?

The most common usecase is to get a read-only image, say for
/usr. However, sometimes (for example with containers) we have a
writable upper layer too. I'm not sure how important metacopy is for
that though, it is more commonly used to avoid duplicating things
between e.g. the container image layers. Giuseppe?

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's an impetuous guerilla werewolf fleeing from a secret government 
programme. She's an elegant insomniac socialite with a flame-thrower. 
They fight crime! 

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Giuseppe Scrivano]

Alexander Larsson <alexl@redhat.com> writes:

> On Tue, 2025-03-25 at 11:57 +0100, Miklos Szeredi wrote:
>> On Tue, 11 Feb 2025 at 13:01, Amir Goldstein <amir73il@gmail.com>
>> wrote:
>> > Looking closer at ovl_maybe_validate_verity(), it's actually
>> > worse - if you create an upper without metacopy above
>> > a lower with metacopy, ovl_validate_verity() will only check
>> > the metacopy xattr on metapath, which is the uppermost
>> > and find no md5digest, so create an upper above a metacopy
>> > lower is a way to avert verity check.
>> > 
>> > So I think lookup code needs to disallow finding metacopy
>> > in middle layer and need to enforce that also when upper is found
>> > via index.
>> 
>> So I think the next patch does this: only allow following a metacopy
>> redirect from lower to data.
>> 
>> It's confusing to call this metacopy, as no copy is performed.  We
>> could call it data-redirect.  Mixing data-redirect with real meta-
>> copy
>> is of dubious value, and we might be better to disable it even in the
>> privileged scenario.
>> 
>> Giuseppe, Alexander, AFAICS the composefs use case employs
>> data-redirect only and not metacopy, right?
>
> The most common usecase is to get a read-only image, say for
> /usr. However, sometimes (for example with containers) we have a
> writable upper layer too. I'm not sure how important metacopy is for
> that though, it is more commonly used to avoid duplicating things
> between e.g. the container image layers. Giuseppe?

for the composefs use case we don't need metacopy, but if it is possible
it would be nice to have metacopy since idmapped mounts do not work yet
in a user namespace.  So each time we run a container in a different
mapping we need a fully copy of the image which would be faster with
metacopy.

Regards,
Giuseppe

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Miklos Szeredi]

On Tue, 25 Mar 2025 at 14:34, Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>
> Alexander Larsson <alexl@redhat.com> writes:
>
> > On Tue, 2025-03-25 at 11:57 +0100, Miklos Szeredi wrote:
> >> On Tue, 11 Feb 2025 at 13:01, Amir Goldstein <amir73il@gmail.com>
> >> wrote:
> >> > Looking closer at ovl_maybe_validate_verity(), it's actually
> >> > worse - if you create an upper without metacopy above
> >> > a lower with metacopy, ovl_validate_verity() will only check
> >> > the metacopy xattr on metapath, which is the uppermost
> >> > and find no md5digest, so create an upper above a metacopy
> >> > lower is a way to avert verity check.
> >> >
> >> > So I think lookup code needs to disallow finding metacopy
> >> > in middle layer and need to enforce that also when upper is found
> >> > via index.
> >>
> >> So I think the next patch does this: only allow following a metacopy
> >> redirect from lower to data.
> >>
> >> It's confusing to call this metacopy, as no copy is performed.  We
> >> could call it data-redirect.  Mixing data-redirect with real meta-
> >> copy
> >> is of dubious value, and we might be better to disable it even in the
> >> privileged scenario.
> >>
> >> Giuseppe, Alexander, AFAICS the composefs use case employs
> >> data-redirect only and not metacopy, right?
> >
> > The most common usecase is to get a read-only image, say for
> > /usr. However, sometimes (for example with containers) we have a
> > writable upper layer too. I'm not sure how important metacopy is for
> > that though, it is more commonly used to avoid duplicating things
> > between e.g. the container image layers. Giuseppe?
>
> for the composefs use case we don't need metacopy, but if it is possible
> it would be nice to have metacopy since idmapped mounts do not work yet
> in a user namespace.  So each time we run a container in a different
> mapping we need a fully copy of the image which would be faster with
> metacopy.

Okay, so there is a usecase for compose + metacopy.

Problem seems to be that this negatively affects the security of the
setup, because the digest is now stored on the unverified upper layer.
Am I misunderstanding this?

Thanks,
Miklos

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Alexander Larsson]

On Tue, 2025-03-25 at 14:42 +0100, Miklos Szeredi wrote:
> On Tue, 25 Mar 2025 at 14:34, Giuseppe Scrivano <gscrivan@redhat.com>
> wrote:
> > 
> > Alexander Larsson <alexl@redhat.com> writes:
> > 
> > > On Tue, 2025-03-25 at 11:57 +0100, Miklos Szeredi wrote:
> > > > On Tue, 11 Feb 2025 at 13:01, Amir Goldstein
> > > > <amir73il@gmail.com>
> > > > wrote:
> > > > > Looking closer at ovl_maybe_validate_verity(), it's actually
> > > > > worse - if you create an upper without metacopy above
> > > > > a lower with metacopy, ovl_validate_verity() will only check
> > > > > the metacopy xattr on metapath, which is the uppermost
> > > > > and find no md5digest, so create an upper above a metacopy
> > > > > lower is a way to avert verity check.
> > > > > 
> > > > > So I think lookup code needs to disallow finding metacopy
> > > > > in middle layer and need to enforce that also when upper is
> > > > > found
> > > > > via index.
> > > > 
> > > > So I think the next patch does this: only allow following a
> > > > metacopy
> > > > redirect from lower to data.
> > > > 
> > > > It's confusing to call this metacopy, as no copy is performed. 
> > > > We
> > > > could call it data-redirect.  Mixing data-redirect with real
> > > > meta-
> > > > copy
> > > > is of dubious value, and we might be better to disable it even
> > > > in the
> > > > privileged scenario.
> > > > 
> > > > Giuseppe, Alexander, AFAICS the composefs use case employs
> > > > data-redirect only and not metacopy, right?
> > > 
> > > The most common usecase is to get a read-only image, say for
> > > /usr. However, sometimes (for example with containers) we have a
> > > writable upper layer too. I'm not sure how important metacopy is
> > > for
> > > that though, it is more commonly used to avoid duplicating things
> > > between e.g. the container image layers. Giuseppe?
> > 
> > for the composefs use case we don't need metacopy, but if it is
> > possible
> > it would be nice to have metacopy since idmapped mounts do not work
> > yet
> > in a user namespace.  So each time we run a container in a
> > different
> > mapping we need a fully copy of the image which would be faster
> > with
> > metacopy.
> 
> Okay, so there is a usecase for compose + metacopy.
> 
> Problem seems to be that this negatively affects the security of the
> setup, because the digest is now stored on the unverified upper
> layer.
> Am I misunderstanding this?

Can you explain the exact security model here. The end user should not
be able to arbitrary change the redirect xattr to bypass permission
checks in the lower via the overlayfs mount directly. So, is the worry
that the upper dir is stored somewhere accessible to the end-user for
direct modification? Is there also a worry that you can write directly
to the lower layers?

Anyway, In the example above couldn't podman just create the metacopyup
layer manually and then pass it as a regular lower dir, then we don't
need metacopy in the upper?

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a shy ninja sorceror on the hunt for the last specimen of a great 
and near-mythical creature. She's a plucky cat-loving magician's 
assistant operating on the wrong side of the law. They fight crime! 

Re: [PATCH 3/5] ovl: make redirect/metacopy rejection consistent

[Alexander Larsson]

On Tue, 2025-03-25 at 08:51 -0400, Colin Walters wrote:
> On Tue, Mar 25, 2025 at 7:18 AM Alexander Larsson <alexl@redhat.com>
> wrote:
> 
> > > So I think lookup code needs to disallow finding metacopy
> > > in middle layer and need to enforce that also when upper is found
> > > via index.
> 
> That sounds right to me, yes. Especially when fsverity is required,
> hopefully we can keep the code paths involved as simple as possible.
> 
> > The most common usecase is to get a read-only image
> 
> Yes, especially for signed images that require fsverity - often the
> deployments of those will not want a writable upper because we want
> to be able to ultimately pair it with policies to enforce userspace
> execution from verity/composefs like IPE etc.
> 
> > However, sometimes (for example with containers) we have a writable
> > upper
> > layer too. I'm not sure how important metacopy is for that though,
> > it is more commonly used to avoid duplicating things between
> > e.g. the container image layers. Giuseppe?
> 
> Wait isn't that statement backwards? metacopy is just for optimizing
> the metadata-only copyup-from-lower case when having a writable upper
> (not technically part of the container image layers).
> I don't see what it has to do with the read-only stack for layers one
> might want to create especially for a composefs use case.
> Though on this topic personally I think it can often make sense to
> perform some "eager" flattening of images in userspace, but that's a
> metadata-only operation (in userspace) and has nothing to do with the
> in-kernel optimization of metacopy (which is about optimizing when
> userspace dynamically changes metadata on disk).

I think this misses the question. There are two things in play here
that are sort of handled by the same thing. The redirects from lower to
data-only, and the metadata-change-avoids-copy-up optimization.
However, they are currently enabled/disabled by the same option
(metacopy). 

The problem here is that if metacopy is on in general, then a writable
upper can also use it for general rewrites, and this is considered
problematic:

* Following redirects can have security consequences: it's like
* a symlink into the lower layer without the permission checks.
* This is only a problem if the upper layer is untrusted (e.g
* comes from an USB drive).  This can allow a non-readable file
* or directory to become readable.
*
* Only following redirects when redirects are enabled disables
* this attack vector when not necessary.

So, I think the question is about whether to split these two options so
we can allow only the lower => data-only redirect, without enabling all
the copy-up optimization, and whether there are cases even with
composefs where the copy-up optimization is useful. (And there are, but
they are not necessarily super important.)

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's an old-fashioned vegetarian master criminal moving from town to 
town, helping folk in trouble. She's a bloodthirsty foul-mouthed snake 
charmer with an incredible destiny. They fight crime! 

[PATCH 4/5] ovl: don't require metacopy=on for lower -> data redirect

[Miklos Szeredi]

Allow the special case of a redirect from a lower layer to a data layer
without having to turn on metacopy.  This makes the feature work with
userxattr, which in turn allows data layers to be usable in user
namespaces.

Minimize the risk by only enabling redirect from a single lower layer to a
data layer iff a data layer is specified.  The only way to access a data
layer is to enable this, so there's really no reason no to enable this.

This can be used safely if the lower layer is read-only and the
user.overlay.redirect xattr cannot be modified.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 Documentation/filesystems/overlayfs.rst |  7 ++++++
 fs/overlayfs/namei.c                    | 32 ++++++++++++++-----------
 fs/overlayfs/params.c                   |  5 ----
 3 files changed, 25 insertions(+), 19 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
index 6245b67ae9e0..5d277d79cf2f 100644
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -429,6 +429,13 @@ Only the data of the files in the "data-only" lower layers may be visible
 when a "metacopy" file in one of the lower layers above it, has a "redirect"
 to the absolute path of the "lower data" file in the "data-only" lower layer.
 
+Instead of explicitly enabling "metacopy=on" it is sufficient to specify at
+least one data-only layer to enable redirection of data to a data-only layer.
+In this case other forms of metacopy are rejected.  Note: this way data-only
+layers may be used toghether with "userxattr", in which case careful attention
+must be given to privileges needed to change the "user.overlay.redirect" xattr
+to prevent misuse.
+
 Since kernel version v6.8, "data-only" lower layers can also be added using
 the "datadir+" mount options and the fsconfig syscall from new mount api.
 For example::
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index da322e9768d1..f9dc71b70beb 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1042,6 +1042,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	char *upperredirect = NULL;
 	bool nextredirect = false;
 	bool nextmetacopy = false;
+	bool check_redirect = (ovl_redirect_follow(ofs) || ofs->numdatalayer);
 	struct dentry *this;
 	unsigned int i;
 	int err;
@@ -1053,7 +1054,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		.is_dir = false,
 		.opaque = false,
 		.stop = false,
-		.last = ovl_redirect_follow(ofs) ? false : !ovl_numlower(poe),
+		.last = check_redirect ? false : !ovl_numlower(poe),
 		.redirect = NULL,
 		.metacopy = 0,
 	};
@@ -1141,7 +1142,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			goto out_put;
 		}
 
-		if (!ovl_redirect_follow(ofs))
+		if (!check_redirect)
 			d.last = i == ovl_numlower(poe) - 1;
 		else if (d.is_dir || !ofs->numdatalayer)
 			d.last = lower.layer->idx == ovl_numlower(roe);
@@ -1222,21 +1223,24 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		}
 	}
 
-	/* Defer lookup of lowerdata in data-only layers to first access */
+	/*
+	 * Defer lookup of lowerdata in data-only layers to first access.
+	 * Don't require redirect=follow and metacopy=on in this case.
+	 */
 	if (d.metacopy && ctr && ofs->numdatalayer && d.absolute_redirect) {
 		d.metacopy = 0;
 		ctr++;
-	}
-
-	if (nextmetacopy && !ofs->config.metacopy) {
-		pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry);
-		err = -EPERM;
-		goto out_put;
-	}
-	if (nextredirect && !ovl_redirect_follow(ofs)) {
-		pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n", dentry);
-		err = -EPERM;
-		goto out_put;
+	} else {
+		if (nextmetacopy && !ofs->config.metacopy) {
+			pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry);
+			err = -EPERM;
+			goto out_put;
+		}
+		if (nextredirect && !ovl_redirect_follow(ofs)) {
+			pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n", dentry);
+			err = -EPERM;
+			goto out_put;
+		}
 	}
 
 	/*
diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
index 1115c22deca0..54468b2b0fba 100644
--- a/fs/overlayfs/params.c
+++ b/fs/overlayfs/params.c
@@ -1000,11 +1000,6 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
 		 */
 	}
 
-	if (ctx->nr_data > 0 && !config->metacopy) {
-		pr_err("lower data-only dirs require metacopy support.\n");
-		return -EINVAL;
-	}
-
 	return 0;
 }
 
-- 
2.48.1

Re: [PATCH 4/5] ovl: don't require metacopy=on for lower -> data redirect

[Amir Goldstein]

> Subject: [PATCH] ovl: don't require metacopy=on for lower -> data redirect

Nit: the change does not require metacopy=on nor redirect_dir=follow
so maybe some short title like:

ovl: relax redirect/metacopy requirements for lower -> data redirect


> Allow the special case of a redirect from a lower layer to a data layer
> without having to turn on metacopy.  This makes the feature work with
> userxattr, which in turn allows data layers to be usable in user
> namespaces.
>
> Minimize the risk by only enabling redirect from a single lower layer to a
> data layer iff a data layer is specified.  The only way to access a data
> layer is to enable this, so there's really no reason no to enable this.
>
> This can be used safely if the lower layer is read-only and the
> user.overlay.redirect xattr cannot be modified.
>

Just need to verify that those assumptions are not broken by
upper index with metacopy/redirect -
I think the safety of redirect still holds, but not the safety
of the verity digest.

Thanks,
Amir.

> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  Documentation/filesystems/overlayfs.rst |  7 ++++++
>  fs/overlayfs/namei.c                    | 32 ++++++++++++++-----------
>  fs/overlayfs/params.c                   |  5 ----
>  3 files changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> index 6245b67ae9e0..5d277d79cf2f 100644
> --- a/Documentation/filesystems/overlayfs.rst
> +++ b/Documentation/filesystems/overlayfs.rst
> @@ -429,6 +429,13 @@ Only the data of the files in the "data-only" lower layers may be visible
>  when a "metacopy" file in one of the lower layers above it, has a "redirect"
>  to the absolute path of the "lower data" file in the "data-only" lower layer.
>
> +Instead of explicitly enabling "metacopy=on" it is sufficient to specify at
> +least one data-only layer to enable redirection of data to a data-only layer.
> +In this case other forms of metacopy are rejected.  Note: this way data-only
> +layers may be used toghether with "userxattr", in which case careful attention
> +must be given to privileges needed to change the "user.overlay.redirect" xattr
> +to prevent misuse.
> +
>  Since kernel version v6.8, "data-only" lower layers can also be added using
>  the "datadir+" mount options and the fsconfig syscall from new mount api.
>  For example::
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index da322e9768d1..f9dc71b70beb 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -1042,6 +1042,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>         char *upperredirect = NULL;
>         bool nextredirect = false;
>         bool nextmetacopy = false;
> +       bool check_redirect = (ovl_redirect_follow(ofs) || ofs->numdatalayer);
>         struct dentry *this;
>         unsigned int i;
>         int err;
> @@ -1053,7 +1054,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 .is_dir = false,
>                 .opaque = false,
>                 .stop = false,
> -               .last = ovl_redirect_follow(ofs) ? false : !ovl_numlower(poe),
> +               .last = check_redirect ? false : !ovl_numlower(poe),
>                 .redirect = NULL,
>                 .metacopy = 0,
>         };
> @@ -1141,7 +1142,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                         goto out_put;
>                 }
>
> -               if (!ovl_redirect_follow(ofs))
> +               if (!check_redirect)
>                         d.last = i == ovl_numlower(poe) - 1;
>                 else if (d.is_dir || !ofs->numdatalayer)
>                         d.last = lower.layer->idx == ovl_numlower(roe);
> @@ -1222,21 +1223,24 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 }
>         }
>
> -       /* Defer lookup of lowerdata in data-only layers to first access */
> +       /*
> +        * Defer lookup of lowerdata in data-only layers to first access.
> +        * Don't require redirect=follow and metacopy=on in this case.
> +        */
>         if (d.metacopy && ctr && ofs->numdatalayer && d.absolute_redirect) {
>                 d.metacopy = 0;
>                 ctr++;
> -       }
> -
> -       if (nextmetacopy && !ofs->config.metacopy) {
> -               pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry);
> -               err = -EPERM;
> -               goto out_put;
> -       }
> -       if (nextredirect && !ovl_redirect_follow(ofs)) {
> -               pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n", dentry);
> -               err = -EPERM;
> -               goto out_put;
> +       } else {
> +               if (nextmetacopy && !ofs->config.metacopy) {
> +                       pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n", dentry);
> +                       err = -EPERM;
> +                       goto out_put;
> +               }
> +               if (nextredirect && !ovl_redirect_follow(ofs)) {
> +                       pr_warn_ratelimited("refusing to follow redirect for (%pd2)\n", dentry);
> +                       err = -EPERM;
> +                       goto out_put;
> +               }
>         }
>
>         /*
> diff --git a/fs/overlayfs/params.c b/fs/overlayfs/params.c
> index 1115c22deca0..54468b2b0fba 100644
> --- a/fs/overlayfs/params.c
> +++ b/fs/overlayfs/params.c
> @@ -1000,11 +1000,6 @@ int ovl_fs_params_verify(const struct ovl_fs_context *ctx,
>                  */
>         }
>
> -       if (ctx->nr_data > 0 && !config->metacopy) {
> -               pr_err("lower data-only dirs require metacopy support.\n");
> -               return -EINVAL;
> -       }
> -
>         return 0;
>  }
>
> --
> 2.48.1
>

[PATCH 5/5] ovl: don't require "metacopy=on" for "verity"

[Miklos Szeredi]

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(-)

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;
-- 
2.48.1

Re: [PATCH 5/5] ovl: don't require "metacopy=on" for "verity"

[Amir Goldstein]

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.

Re: [PATCH 5/5] ovl: don't require "metacopy=on" for "verity"

[Miklos Szeredi]

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

Re: [PATCH 5/5] ovl: don't require "metacopy=on" for "verity"

[Amir Goldstein]

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.

Re: [PATCH 1/5] ovl: don't allow datadir only

[Amir Goldstein]

On Mon, Feb 10, 2025 at 8:45 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> In theory overlayfs could support upper layer directly referring to a data
> layer, but there's no current use case for this.
>
> Originally, when data-only layers were introduced, this wasn't allowed,
> only introduced by the "datadir+" feture, but without actually handling

Spelling s/feture/feature

> this case, resuting in an Oops.

Spelling s/resuting/resulting

>
> Fix by disallowing datadir without lowerdir.
>
> Reported-by: Giuseppe Scrivano <gscrivan@redhat.com>
> Fixes: 24e16e385f22 ("ovl: add support for appending lowerdirs one by one")
> Cc: <stable@vger.kernel.org> # v6.7
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/super.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 86ae6f6da36b..b11094acdd8f 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -1137,6 +1137,11 @@ static struct ovl_entry *ovl_get_lowerstack(struct super_block *sb,
>                 return ERR_PTR(-EINVAL);
>         }
>
> +       if (ctx->nr == ctx->nr_data) {
> +               pr_err("at least one non-data lowerdir is required\n");
> +               return ERR_PTR(-EINVAL);
> +       }
> +
>         err = -EINVAL;
>         for (i = 0; i < ctx->nr; i++) {
>                 l = &ctx->lower[i];
> --
> 2.48.1
>