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

[Alexander Larsson <alexl@redhat.com>]

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!