[PATCH v2 0/5] ovl: metacopy/verity fixes and improvements

[PATCH v2 0/5] ovl: metacopy/verity fixes and improvements

[Miklos Szeredi]

The main purpose of this patchset is allowing metadata/data-only layers to
be usable in user namespaces (without super user privs).

v2:
	- drop broken hunk in param.c (Amir)
	- patch header improvements (Amir)

---
Giuseppe Scrivano (1):
  ovl: remove unused forward declaration

Miklos Szeredi (4):
  ovl: don't allow datadir only
  ovl: make redirect/metacopy rejection consistent
  ovl: relax redirect/metacopy requirements for lower -> data redirect
  ovl: don't require "metacopy=on" for "verity"

 Documentation/filesystems/overlayfs.rst |  7 +++
 fs/overlayfs/namei.c                    | 77 ++++++++++++++++---------
 fs/overlayfs/overlayfs.h                |  2 -
 fs/overlayfs/params.c                   | 16 +----
 fs/overlayfs/super.c                    |  5 ++
 5 files changed, 66 insertions(+), 41 deletions(-)

-- 
2.49.0

[PATCH v2 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+" feature, but without actually handling
this case, resulting 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
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
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.49.0

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

[Alexander Larsson]

On Tue, 2025-03-25 at 11:46 +0100, Miklos Szeredi 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+" feature, but without actually
> handling
> this case, resulting 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
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>

Reviewed-by: Alexander Larsson <alexl@redhat.com>


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

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's an obese crooked filmmaker trapped in a world he never made. She's
a 
provocative red-headed stripper from a different time and place. They 
fight crime! 

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

[Christian Brauner]

On Tue, Mar 25, 2025 at 11:46:29AM +0100, Miklos Szeredi 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+" feature, but without actually handling
> this case, resulting 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
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---

Reviewed-by: Christian Brauner <brauner@kernel.org>

[PATCH v2 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")
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
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.49.0

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

[Alexander Larsson]

On Tue, 2025-03-25 at 11:46 +0100, Miklos Szeredi 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")
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>

Reviewed-by: Alexander Larsson <alexl@redhat.com>

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

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a deeply religious vegetarian gentleman spy in drag. She's a 
pregnant renegade politician on the trail of a serial killer. They
fight 
crime! 

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

[Christian Brauner]

On Tue, Mar 25, 2025 at 11:46:30AM +0100, Miklos Szeredi 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")
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---

Reviewed-by: Christian Brauner <brauner@kernel.org>

[PATCH v2 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, paving 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.49.0

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

[Amir Goldstein]

On Tue, Mar 25, 2025 at 11:46 AM 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, paving the
> way for following patches to change when dataredirects are allowed.
>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Reviewed-by: Amir Goldstein <amir73il@gmail.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.49.0
>

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

[Alexander Larsson]

On Tue, 2025-03-25 at 11:46 +0100, Miklos Szeredi 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,
> paving the
> way for following patches to change when dataredirects are allowed.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>

Reviewed-by: Alexander Larsson <alexl@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

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a witless guerilla astronaut with a mysterious suitcase handcuffed
to his arm. She's a mistrustful communist journalist with an incredible
destiny. They fight crime! 

[PATCH v2 4/5] ovl: relax redirect/metacopy requirements 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.49.0

Re: [PATCH v2 4/5] ovl: relax redirect/metacopy requirements for lower -> data redirect

[Amir Goldstein]

On Tue, Mar 25, 2025 at 11:46 AM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> 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>
Reviewed-by: Amir Goldstein <amir73il@gmail.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.49.0
>

Re: [PATCH v2 4/5] ovl: relax redirect/metacopy requirements for lower -> data redirect

[Alexander Larsson]

On Tue, 2025-03-25 at 11:46 +0100, Miklos Szeredi wrote:
> 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>
Reviewed-by: Alexander Larsson <alexl@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;
>  }
>  

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a one-legged overambitious farmboy who hides his scarred face
behind 
a mask. She's a psychotic communist mercenary who don't take no shit
from 
nobody. They fight crime! 

[PATCH v2 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).

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) {
 		/* 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
-- 
2.49.0

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

[Amir Goldstein]

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.

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

[Amir Goldstein]

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.

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

[Alexander Larsson]

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.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a benighted devious farmboy possessed of the uncanny powers of an 
insect. She's a strong-willed blonde stripper fleeing from a Satanic 
cult. They fight crime! 

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

[Miklos Szeredi]

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

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

[Alexander Larsson]

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.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a lonely Jewish vampire hunter on a search for his missing sister.
She's a man-hating Buddhist socialite trying to make a difference in a 
man's world. They fight crime! 

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

[Alexander Larsson]

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

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's an oversexed soccer-playing filmmaker possessed of the uncanny 
powers of an insect. She's a foxy nymphomaniac mercenary on the trail
of 
a serial killer. They fight crime! 

Re: [PATCH v2 0/5] ovl: metacopy/verity fixes and improvements

[Amir Goldstein]

On Tue, Mar 25, 2025 at 11:46 AM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
> The main purpose of this patchset is allowing metadata/data-only layers to
> be usable in user namespaces (without super user privs).

Please add test coverage to this use case.
I think a userxattr variant of test overlay/080 should be easy.

Thanks,
Amir.