[PATCH] overlayfs: Fix redirect traversal on metacopy dentries

[PATCH] overlayfs: Fix redirect traversal on metacopy dentries

[Vivek Goyal]

Amir pointed me to metacopy test cases in unionmount-testsuite and
I decided to run "./run --ov=10 --meta" and it failed while running
test "rename-mass-5.py".

Problem is w.r.t absolute redirect traversal on intermediate metacopy
dentry. We do not store intermediate metacopy dentries and also skip
current loop/layer and move onto lookup in next layer. But at the end
of loop, we have logic to reset "poe" and layer index if currnently
looked up dentry has absolute redirect. We skip all that and that
means lookup in next layer will fail.

Following is simple test case to reproduce this.

- mkdir -p lower upper work merged lower/a lower/b
- touch lower/a/foo.txt
- mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work,metacopy=on none merged

# Following will create absolute redirect "/a/foo.txt" on upper/b/bar.txt.
- mv merged/a/foo.txt merged/b/bar.txt

# unmount overlay and use upper as lower layer (lower2) for next mount.
- umount merged
- mv upper lower2
- rm -rf work; mkdir -p upper work
- mount -t overlay -o lowerdir=lower2:lower,upperdir=upper,workdir=work,metacopy=on none merged

# Force a metacopy copy-up
- chown bin:bin merged/b/bar.txt

# unmount overlay and use upper as lower layer (lower3) for next mount.
- umount merged
- mv upper lower3
- rm -rf work; mkdir -p upper work
- mount -t overlay -o lowerdir=lower3:lower2:lower,upperdir=upper,workdir=work,metacopy=on none merged

# ls merged/b/bar.txt
ls: cannot access 'bar.txt': Input/output error

Intermediate lower layer (lower2) has metacopy dentry b/bar.txt with absolute
redirect "/a/foo.txt". We skipped redirect processing at the end of loop
which sets poe to roe and sets the appropriate next lower layer index. And
that means lookup failed in next layer.

Fix this by continuing the loop for any intermediate dentries. We still do not
save these at lower stack. With this fix applied unionmount-testsuite,
"./run --ov-10 --meta" now passes.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/namei.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index da05e33db9ce..df81ec0e179f 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -913,15 +913,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			goto out_put;
 		}
 
-		/*
-		 * Do not store intermediate metacopy dentries in chain,
-		 * except top most lower metacopy dentry
-		 */
-		if (d.metacopy && ctr) {
-			dput(this);
-			continue;
-		}
-
 		/*
 		 * If no origin fh is stored in upper of a merge dir, store fh
 		 * of lower dir and set upper parent "impure".
@@ -956,9 +947,20 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			origin = this;
 		}
 
-		stack[ctr].dentry = this;
-		stack[ctr].layer = lower.layer;
-		ctr++;
+		if (d.metacopy && ctr) {
+			/*
+			 * Do not store intermediate metacopy dentries in
+			 * lower chain, except top most lower metacopy dentry.
+			 * Continue the loop so that if there is an absolute
+			 * redirect on this dentry, poe can be reset to roe.
+			 */
+			dput(this);
+			this = NULL;
+		} else {
+			stack[ctr].dentry = this;
+			stack[ctr].layer = lower.layer;
+			ctr++;
+		}
 
 		/*
 		 * Following redirects can have security consequences: it's like
-- 
2.25.4

Re: [PATCH] overlayfs: Fix redirect traversal on metacopy dentries

[Amir Goldstein]

On Tue, Jun 2, 2020 at 6:23 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> Amir pointed me to metacopy test cases in unionmount-testsuite and
> I decided to run "./run --ov=10 --meta" and it failed while running
> test "rename-mass-5.py".
>
> Problem is w.r.t absolute redirect traversal on intermediate metacopy
> dentry. We do not store intermediate metacopy dentries and also skip
> current loop/layer and move onto lookup in next layer. But at the end
> of loop, we have logic to reset "poe" and layer index if currnently
> looked up dentry has absolute redirect. We skip all that and that
> means lookup in next layer will fail.
>
> Following is simple test case to reproduce this.
>
> - mkdir -p lower upper work merged lower/a lower/b
> - touch lower/a/foo.txt
> - mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work,metacopy=on none merged
>
> # Following will create absolute redirect "/a/foo.txt" on upper/b/bar.txt.
> - mv merged/a/foo.txt merged/b/bar.txt
>
> # unmount overlay and use upper as lower layer (lower2) for next mount.
> - umount merged
> - mv upper lower2
> - rm -rf work; mkdir -p upper work
> - mount -t overlay -o lowerdir=lower2:lower,upperdir=upper,workdir=work,metacopy=on none merged
>
> # Force a metacopy copy-up
> - chown bin:bin merged/b/bar.txt
>
> # unmount overlay and use upper as lower layer (lower3) for next mount.
> - umount merged
> - mv upper lower3
> - rm -rf work; mkdir -p upper work
> - mount -t overlay -o lowerdir=lower3:lower2:lower,upperdir=upper,workdir=work,metacopy=on none merged
>
> # ls merged/b/bar.txt
> ls: cannot access 'bar.txt': Input/output error
>
> Intermediate lower layer (lower2) has metacopy dentry b/bar.txt with absolute
> redirect "/a/foo.txt". We skipped redirect processing at the end of loop
> which sets poe to roe and sets the appropriate next lower layer index. And
> that means lookup failed in next layer.
>
> Fix this by continuing the loop for any intermediate dentries. We still do not
> save these at lower stack. With this fix applied unionmount-testsuite,
> "./run --ov-10 --meta" now passes.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

I though I ran this test - I guess not...

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

> ---
>  fs/overlayfs/namei.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index da05e33db9ce..df81ec0e179f 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -913,15 +913,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                         goto out_put;
>                 }
>
> -               /*
> -                * Do not store intermediate metacopy dentries in chain,
> -                * except top most lower metacopy dentry
> -                */
> -               if (d.metacopy && ctr) {
> -                       dput(this);
> -                       continue;
> -               }
> -
>                 /*
>                  * If no origin fh is stored in upper of a merge dir, store fh
>                  * of lower dir and set upper parent "impure".
> @@ -956,9 +947,20 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                         origin = this;
>                 }
>
> -               stack[ctr].dentry = this;
> -               stack[ctr].layer = lower.layer;
> -               ctr++;
> +               if (d.metacopy && ctr) {
> +                       /*
> +                        * Do not store intermediate metacopy dentries in
> +                        * lower chain, except top most lower metacopy dentry.
> +                        * Continue the loop so that if there is an absolute
> +                        * redirect on this dentry, poe can be reset to roe.
> +                        */
> +                       dput(this);
> +                       this = NULL;
> +               } else {
> +                       stack[ctr].dentry = this;
> +                       stack[ctr].layer = lower.layer;
> +                       ctr++;
> +               }
>
>                 /*
>                  * Following redirects can have security consequences: it's like
> --
> 2.25.4
>

Re: [PATCH] overlayfs: Fix redirect traversal on metacopy dentries

[Vivek Goyal]

On Tue, Jun 02, 2020 at 11:23:38AM -0400, Vivek Goyal wrote:
> Amir pointed me to metacopy test cases in unionmount-testsuite and
> I decided to run "./run --ov=10 --meta" and it failed while running
> test "rename-mass-5.py".

Hi Miklos,

This patch applies on top of the patch series I posted previously.

https://marc.info/?l=linux-unionfs&m=159102703820108&w=2

Thanks
Vivek

> 
> Problem is w.r.t absolute redirect traversal on intermediate metacopy
> dentry. We do not store intermediate metacopy dentries and also skip
> current loop/layer and move onto lookup in next layer. But at the end
> of loop, we have logic to reset "poe" and layer index if currnently
> looked up dentry has absolute redirect. We skip all that and that
> means lookup in next layer will fail.
> 
> Following is simple test case to reproduce this.
> 
> - mkdir -p lower upper work merged lower/a lower/b
> - touch lower/a/foo.txt
> - mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work,metacopy=on none merged
> 
> # Following will create absolute redirect "/a/foo.txt" on upper/b/bar.txt.
> - mv merged/a/foo.txt merged/b/bar.txt
> 
> # unmount overlay and use upper as lower layer (lower2) for next mount.
> - umount merged
> - mv upper lower2
> - rm -rf work; mkdir -p upper work
> - mount -t overlay -o lowerdir=lower2:lower,upperdir=upper,workdir=work,metacopy=on none merged
> 
> # Force a metacopy copy-up
> - chown bin:bin merged/b/bar.txt
> 
> # unmount overlay and use upper as lower layer (lower3) for next mount.
> - umount merged
> - mv upper lower3
> - rm -rf work; mkdir -p upper work
> - mount -t overlay -o lowerdir=lower3:lower2:lower,upperdir=upper,workdir=work,metacopy=on none merged
> 
> # ls merged/b/bar.txt
> ls: cannot access 'bar.txt': Input/output error
> 
> Intermediate lower layer (lower2) has metacopy dentry b/bar.txt with absolute
> redirect "/a/foo.txt". We skipped redirect processing at the end of loop
> which sets poe to roe and sets the appropriate next lower layer index. And
> that means lookup failed in next layer.
> 
> Fix this by continuing the loop for any intermediate dentries. We still do not
> save these at lower stack. With this fix applied unionmount-testsuite,
> "./run --ov-10 --meta" now passes.
> 
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/namei.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index da05e33db9ce..df81ec0e179f 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -913,15 +913,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>  			goto out_put;
>  		}
>  
> -		/*
> -		 * Do not store intermediate metacopy dentries in chain,
> -		 * except top most lower metacopy dentry
> -		 */
> -		if (d.metacopy && ctr) {
> -			dput(this);
> -			continue;
> -		}
> -
>  		/*
>  		 * If no origin fh is stored in upper of a merge dir, store fh
>  		 * of lower dir and set upper parent "impure".
> @@ -956,9 +947,20 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>  			origin = this;
>  		}
>  
> -		stack[ctr].dentry = this;
> -		stack[ctr].layer = lower.layer;
> -		ctr++;
> +		if (d.metacopy && ctr) {
> +			/*
> +			 * Do not store intermediate metacopy dentries in
> +			 * lower chain, except top most lower metacopy dentry.
> +			 * Continue the loop so that if there is an absolute
> +			 * redirect on this dentry, poe can be reset to roe.
> +			 */
> +			dput(this);
> +			this = NULL;
> +		} else {
> +			stack[ctr].dentry = this;
> +			stack[ctr].layer = lower.layer;
> +			ctr++;
> +		}
>  
>  		/*
>  		 * Following redirects can have security consequences: it's like
> -- 
> 2.25.4
> 

Re: [PATCH] overlayfs: Fix redirect traversal on metacopy dentries

[Miklos Szeredi]

On Tue, Jun 2, 2020 at 8:39 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Tue, Jun 02, 2020 at 11:23:38AM -0400, Vivek Goyal wrote:
> > Amir pointed me to metacopy test cases in unionmount-testsuite and
> > I decided to run "./run --ov=10 --meta" and it failed while running
> > test "rename-mass-5.py".

Thanks, applied.

Miklos