[PATCH] ovl: relax WARN_ON() for overlapping layers use case

[PATCH] ovl: relax WARN_ON() for overlapping layers use case

[Amir Goldstein]

This nasty little syzbot repro:
https://syzkaller.appspot.com/x/repro.syz?x=12c7a94f400000

Creates overlay mounts where the same directory is both in upper
and lower layers. Simplified example:

  mkdir foo work
  mount -t overlay none foo -o"lowerdir=.,upperdir=foo,workdir=work"

The repro runs several threads in parallel that attempt to chdir
into foo and attempt to symlink/rename/exec/mkdir the file bar.

The repro hits a WARN_ON() I placed in ovl_instantiate(), which
suggests that an overlay inode already exists in cache and is hashed
by the pointer of the real upper dentry that ovl_create_real() has
just created. At the point of the WARN_ON(), for overlay dir inode
lock is held and upper dir inode lock, so at first, I did not see how
this was possible.

On a closer look, I see that after ovl_create_real(), because of the
overlapping upper and lower layers, a lookup by another thread can
find the file foo/bar that was just created in upper layer, at overlay
path foo/foo/bar and hash the an overlay inode with the new real dentry
as lower dentry. This is possible because the overlay directory
foo/foo is not locked and the upper dentry foo/bar is in dcache, so
ovl_lookup() can find it without taking upper dir inode shared lock.

Overlapping layers is considered a wrong setup which would result in
unexpected behavior, but it shouldn't crash the kernel and it shouldn't
trigger WARN_ON() either, so relax this WARN_ON() and leave a pr_warn()
instead to cover all cases of failure to get an overlay inode.

The error returned from failure to insert new inode to cache with
inode_insert5() was changed to -EEXIST, to distinguish from the error
-ENOMEM returned on failure to get/allocate inode with iget5_locked().

Reported-by: syzbot+9c69c282adc4edd2b540@syzkaller.appspotmail.com
Fixes: 01b39dcc9568 ("ovl: use inode_insert5() to hash a newly...")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/dir.c   | 2 +-
 fs/overlayfs/inode.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 82c129bfe58d..93872bb50230 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -260,7 +260,7 @@ static int ovl_instantiate(struct dentry *dentry, struct inode *inode,
 		 * hashed directory inode aliases.
 		 */
 		inode = ovl_get_inode(dentry->d_sb, &oip);
-		if (WARN_ON(IS_ERR(inode)))
+		if (IS_ERR(inode))
 			return PTR_ERR(inode);
 	} else {
 		WARN_ON(ovl_inode_real(inode) != d_inode(newdentry));
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 3b7ed5d2279c..b48273e846ad 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -832,7 +832,7 @@ struct inode *ovl_get_inode(struct super_block *sb,
 	int fsid = bylower ? oip->lowerpath->layer->fsid : 0;
 	bool is_dir, metacopy = false;
 	unsigned long ino = 0;
-	int err = -ENOMEM;
+	int err = oip->newinode ? -EEXIST : -ENOMEM;
 
 	if (!realinode)
 		realinode = d_inode(lowerdentry);
@@ -917,6 +917,7 @@ struct inode *ovl_get_inode(struct super_block *sb,
 	return inode;
 
 out_err:
+	pr_warn_ratelimited("overlayfs: failed to get inode (%i)\n", err);
 	inode = ERR_PTR(err);
 	goto out;
 }
-- 
2.17.1

Re: [PATCH] ovl: relax WARN_ON() for overlapping layers use case

[Vivek Goyal]

On Thu, Mar 28, 2019 at 05:38:29PM +0200, Amir Goldstein wrote:
> This nasty little syzbot repro:
> https://syzkaller.appspot.com/x/repro.syz?x=12c7a94f400000
> 
> Creates overlay mounts where the same directory is both in upper
> and lower layers. Simplified example:
> 
>   mkdir foo work
>   mount -t overlay none foo -o"lowerdir=.,upperdir=foo,workdir=work"
> 
> The repro runs several threads in parallel that attempt to chdir
> into foo and attempt to symlink/rename/exec/mkdir the file bar.
> 
> The repro hits a WARN_ON() I placed in ovl_instantiate(), which
> suggests that an overlay inode already exists in cache and is hashed
> by the pointer of the real upper dentry that ovl_create_real() has
> just created. At the point of the WARN_ON(), for overlay dir inode
> lock is held and upper dir inode lock, so at first, I did not see how
> this was possible.
> 
> On a closer look, I see that after ovl_create_real(), because of the
> overlapping upper and lower layers, a lookup by another thread can
> find the file foo/bar that was just created in upper layer, at overlay
> path foo/foo/bar and hash the an overlay inode with the new real dentry
> as lower dentry. This is possible because the overlay directory
> foo/foo is not locked and the upper dentry foo/bar is in dcache, so
> ovl_lookup() can find it without taking upper dir inode shared lock.
> 
> Overlapping layers is considered a wrong setup which would result in
> unexpected behavior, but it shouldn't crash the kernel and it shouldn't
> trigger WARN_ON() either,

Hi Amir,

Is it possible detect this overlapping layer configuration and fail the
mount instead (is_subdir())?

Vivek

Re: [PATCH] ovl: relax WARN_ON() for overlapping layers use case

[Amir Goldstein]

On Thu, Mar 28, 2019 at 11:40 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Thu, Mar 28, 2019 at 05:38:29PM +0200, Amir Goldstein wrote:
> > This nasty little syzbot repro:
> > https://syzkaller.appspot.com/x/repro.syz?x=12c7a94f400000
> >
> > Creates overlay mounts where the same directory is both in upper
> > and lower layers. Simplified example:
> >
> >   mkdir foo work
> >   mount -t overlay none foo -o"lowerdir=.,upperdir=foo,workdir=work"
> >
> > The repro runs several threads in parallel that attempt to chdir
> > into foo and attempt to symlink/rename/exec/mkdir the file bar.
> >
> > The repro hits a WARN_ON() I placed in ovl_instantiate(), which
> > suggests that an overlay inode already exists in cache and is hashed
> > by the pointer of the real upper dentry that ovl_create_real() has
> > just created. At the point of the WARN_ON(), for overlay dir inode
> > lock is held and upper dir inode lock, so at first, I did not see how
> > this was possible.
> >
> > On a closer look, I see that after ovl_create_real(), because of the
> > overlapping upper and lower layers, a lookup by another thread can
> > find the file foo/bar that was just created in upper layer, at overlay
> > path foo/foo/bar and hash the an overlay inode with the new real dentry
> > as lower dentry. This is possible because the overlay directory
> > foo/foo is not locked and the upper dentry foo/bar is in dcache, so
> > ovl_lookup() can find it without taking upper dir inode shared lock.
> >
> > Overlapping layers is considered a wrong setup which would result in
> > unexpected behavior, but it shouldn't crash the kernel and it shouldn't
> > trigger WARN_ON() either,
>
> Hi Amir,
>
> Is it possible detect this overlapping layer configuration and fail the
> mount instead (is_subdir())?
>

Possible? Yes - see ovl_workdir_ok(), feel free to post a patch...

Race free? Not that I know of.

Instead of the fix - No, because upper dir can be moved under lower
layer root also after mount (same for work dir btw) and there are
likely other ways to make a mess that do not involve moving the
layer root (cross layer hardlinks are not pretty).

That is the incentive to my proposal of "rename fences":
https://marc.info/?l=linux-fsdevel&m=155082091218483&w=2

Thanks,
Amir.

Re: [PATCH] ovl: relax WARN_ON() for overlapping layers use case

[Vivek Goyal]

On Thu, Mar 28, 2019 at 05:38:29PM +0200, Amir Goldstein wrote:
> This nasty little syzbot repro:
> https://syzkaller.appspot.com/x/repro.syz?x=12c7a94f400000
> 
> Creates overlay mounts where the same directory is both in upper
> and lower layers. Simplified example:
> 
>   mkdir foo work
>   mount -t overlay none foo -o"lowerdir=.,upperdir=foo,workdir=work"
> 
> The repro runs several threads in parallel that attempt to chdir
> into foo and attempt to symlink/rename/exec/mkdir the file bar.
> 
> The repro hits a WARN_ON() I placed in ovl_instantiate(), which
> suggests that an overlay inode already exists in cache and is hashed
> by the pointer of the real upper dentry that ovl_create_real() has
> just created. At the point of the WARN_ON(), for overlay dir inode
> lock is held and upper dir inode lock, so at first, I did not see how
> this was possible.
> 
> On a closer look, I see that after ovl_create_real(), because of the
> overlapping upper and lower layers, a lookup by another thread can
> find the file foo/bar that was just created in upper layer, at overlay
> path foo/foo/bar and hash the an overlay inode with the new real dentry
> as lower dentry. This is possible because the overlay directory
> foo/foo is not locked and the upper dentry foo/bar is in dcache, so
> ovl_lookup() can find it without taking upper dir inode shared lock.

Hi Amir,

Just to understand better this case, so does following fail.

ovl_verify_inode)() {
        if (upperdentry && ovl_inode_upper(inode) != d_inode(upperdentry))
                return false;
}

Because we already found hashed inode (as lower real file), so
ovl_inode_upper(inode) will not be set and ovl_verify_inode() will fail.

> 
> Overlapping layers is considered a wrong setup which would result in
> unexpected behavior, but it shouldn't crash the kernel and it shouldn't
> trigger WARN_ON() either, so relax this WARN_ON() and leave a pr_warn()
> instead to cover all cases of failure to get an overlay inode.

Is this not equivalent of lower layers being modified while overlay
being mounted. If that's the case, then WARN_ON_ONCE() kind of makes
sense to flag the anomaly in underlying layers?

IOW, overlayfs is not crashing. It is just warning for an anomaly it
found due to overlapping layers. And what's wrong with giving the
warning?

> 
> The error returned from failure to insert new inode to cache with
> inode_insert5() was changed to -EEXIST, to distinguish from the error
> -ENOMEM returned on failure to get/allocate inode with iget5_locked().

This is a separate cleanup and not related to this issue, do I
understand it right?

> 
> Reported-by: syzbot+9c69c282adc4edd2b540@syzkaller.appspotmail.com
> Fixes: 01b39dcc9568 ("ovl: use inode_insert5() to hash a newly...")
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/dir.c   | 2 +-
>  fs/overlayfs/inode.c | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 82c129bfe58d..93872bb50230 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -260,7 +260,7 @@ static int ovl_instantiate(struct dentry *dentry, struct inode *inode,
>  		 * hashed directory inode aliases.
>  		 */
>  		inode = ovl_get_inode(dentry->d_sb, &oip);
> -		if (WARN_ON(IS_ERR(inode)))
> +		if (IS_ERR(inode))
>  			return PTR_ERR(inode);
>  	} else {
>  		WARN_ON(ovl_inode_real(inode) != d_inode(newdentry));
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 3b7ed5d2279c..b48273e846ad 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -832,7 +832,7 @@ struct inode *ovl_get_inode(struct super_block *sb,
>  	int fsid = bylower ? oip->lowerpath->layer->fsid : 0;
>  	bool is_dir, metacopy = false;
>  	unsigned long ino = 0;
> -	int err = -ENOMEM;
> +	int err = oip->newinode ? -EEXIST : -ENOMEM;
>  
>  	if (!realinode)
>  		realinode = d_inode(lowerdentry);
> @@ -917,6 +917,7 @@ struct inode *ovl_get_inode(struct super_block *sb,
>  	return inode;
>  
>  out_err:
> +	pr_warn_ratelimited("overlayfs: failed to get inode (%i)\n", err);
>  	inode = ERR_PTR(err);
>  	goto out;
>  }
> -- 
> 2.17.1
> 

Re: [PATCH] ovl: relax WARN_ON() for overlapping layers use case

[Amir Goldstein]

On Fri, Mar 29, 2019 at 6:40 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Thu, Mar 28, 2019 at 05:38:29PM +0200, Amir Goldstein wrote:
> > This nasty little syzbot repro:
> > https://syzkaller.appspot.com/x/repro.syz?x=12c7a94f400000
> >
> > Creates overlay mounts where the same directory is both in upper
> > and lower layers. Simplified example:
> >
> >   mkdir foo work
> >   mount -t overlay none foo -o"lowerdir=.,upperdir=foo,workdir=work"
> >
> > The repro runs several threads in parallel that attempt to chdir
> > into foo and attempt to symlink/rename/exec/mkdir the file bar.
> >
> > The repro hits a WARN_ON() I placed in ovl_instantiate(), which
> > suggests that an overlay inode already exists in cache and is hashed
> > by the pointer of the real upper dentry that ovl_create_real() has
> > just created. At the point of the WARN_ON(), for overlay dir inode
> > lock is held and upper dir inode lock, so at first, I did not see how
> > this was possible.
> >
> > On a closer look, I see that after ovl_create_real(), because of the
> > overlapping upper and lower layers, a lookup by another thread can
> > find the file foo/bar that was just created in upper layer, at overlay
> > path foo/foo/bar and hash the an overlay inode with the new real dentry
> > as lower dentry. This is possible because the overlay directory
> > foo/foo is not locked and the upper dentry foo/bar is in dcache, so
> > ovl_lookup() can find it without taking upper dir inode shared lock.
>
> Hi Amir,
>
> Just to understand better this case, so does following fail.
>
> ovl_verify_inode)() {
>         if (upperdentry && ovl_inode_upper(inode) != d_inode(upperdentry))
>                 return false;
> }
>
> Because we already found hashed inode (as lower real file), so
> ovl_inode_upper(inode) will not be set and ovl_verify_inode() will fail.
>

Yes, that's the story I told.
I have no proof that this is what happened.
But the above seems possible in the code.

> >
> > Overlapping layers is considered a wrong setup which would result in
> > unexpected behavior, but it shouldn't crash the kernel and it shouldn't
> > trigger WARN_ON() either, so relax this WARN_ON() and leave a pr_warn()
> > instead to cover all cases of failure to get an overlay inode.
>
> Is this not equivalent of lower layers being modified while overlay
> being mounted. If that's the case, then WARN_ON_ONCE() kind of makes
> sense to flag the anomaly in underlying layers?
>
> IOW, overlayfs is not crashing. It is just warning for an anomaly it
> found due to overlapping layers. And what's wrong with giving the
> warning?
>

User input (fuzzing) should not trigger BUG_ON/WARN_ON
Those are asserts that claim a bug in the code, not wrong input.
In similar cases like these that we ran into in the past we converted
overlayfs code to pr_warn_ratelimited().


> >
> > The error returned from failure to insert new inode to cache with
> > inode_insert5() was changed to -EEXIST, to distinguish from the error
> > -ENOMEM returned on failure to get/allocate inode with iget5_locked().
>
> This is a separate cleanup and not related to this issue, do I
> understand it right?
>

Right, but I would like to be able to get an affirmation to the theory
above. Another way to get to WARN_ON() is if inode_insert5()
finds an I_CREATING inode, but I don't think this can be the case here.

Thanks,
Amir.

Re: [PATCH] ovl: relax WARN_ON() for overlapping layers use case

[Miklos Szeredi]

On Thu, Mar 28, 2019 at 4:38 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> This nasty little syzbot repro:
> https://syzkaller.appspot.com/x/repro.syz?x=12c7a94f400000
>
> Creates overlay mounts where the same directory is both in upper
> and lower layers. Simplified example:
>
>   mkdir foo work
>   mount -t overlay none foo -o"lowerdir=.,upperdir=foo,workdir=work"

Shouldn't the mount fail in this case?

Does it make any sense to allow overlapping layers?

If doing the check the dumb way, the number of d_ancestor() calls
would increase quadratically with the number of layers, but I think
it's possible to do it in linear time if necessary.

Thanks,
Miklos

Re: [PATCH] ovl: relax WARN_ON() for overlapping layers use case

[Amir Goldstein]

On Sat, Mar 30, 2019 at 10:45 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Thu, Mar 28, 2019 at 4:38 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > This nasty little syzbot repro:
> > https://syzkaller.appspot.com/x/repro.syz?x=12c7a94f400000
> >
> > Creates overlay mounts where the same directory is both in upper
> > and lower layers. Simplified example:
> >
> >   mkdir foo work
> >   mount -t overlay none foo -o"lowerdir=.,upperdir=foo,workdir=work"
>
> Shouldn't the mount fail in this case?
>

Sure, but that is independent of this patch, because renaming
between lower layers after mount can get you to the same
situation and that is harder to prevent.

> Does it make any sense to allow overlapping layers?
>
> If doing the check the dumb way, the number of d_ancestor() calls
> would increase quadratically with the number of layers, but I think
> it's possible to do it in linear time if necessary.

As I wrote to Vivek, we can do that, but it's only going to
benefit users that make an innocent mistake (what's the odds of that?).
An adversary could easily work around the mount check by moving
lower dirs after mount.

As I wrote in LSF proposal, it would be quite simple to mark
the dentry of layer's root with a flag (i.e. RENAME_FENCE) that would
prevent taking rename_lock when such dentry is in the ancestry of
either src or target.

The challenge is how to make that API generic enough and which
privileges are to be required for setting up such a fence, because
it could be easily used to create DoS in the wrong hands.
Also, if anyone has ever considered making overlayfs mountable
by usrens root, that is going to be another thing to worry about.

Thanks,
Amir.

Re: [PATCH] ovl: relax WARN_ON() for overlapping layers use case

[Amir Goldstein]

On Sun, Mar 31, 2019 at 8:41 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Sat, Mar 30, 2019 at 10:45 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Thu, Mar 28, 2019 at 4:38 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > This nasty little syzbot repro:
> > > https://syzkaller.appspot.com/x/repro.syz?x=12c7a94f400000
> > >
> > > Creates overlay mounts where the same directory is both in upper
> > > and lower layers. Simplified example:
> > >
> > >   mkdir foo work
> > >   mount -t overlay none foo -o"lowerdir=.,upperdir=foo,workdir=work"
> >
> > Shouldn't the mount fail in this case?
> >
>
> Sure, but that is independent of this patch, because renaming
> between lower layers after mount can get you to the same
> situation and that is harder to prevent.
>
> > Does it make any sense to allow overlapping layers?
> >
> > If doing the check the dumb way, the number of d_ancestor() calls
> > would increase quadratically with the number of layers, but I think
> > it's possible to do it in linear time if necessary.
>
> As I wrote to Vivek, we can do that, but it's only going to
> benefit users that make an innocent mistake (what's the odds of that?).
> An adversary could easily work around the mount check by moving
> lower dirs after mount.
>

What do you guys thing of this idea:

On mount we "poison" overlay inode cache with canary overlay inode
for each layer root (including upper and work).
The canary inode has the layer root inode as i_private, but both
 ovl_inode_lower() and ovl_inode_upper() NULL, so it will always
fail ovl_verify_inode().

Then in  ovl_lookup_single(), we do a sanity
ovl_lookup_inode(this->d_inode). If it returns -ESTALE, we fail
the lookup.

This should immune overlayfs from moving layers root
into other layers after mount.

During mount, after setting up all canary inodes, we can walk
back ancestors of all layers roots and use ovl_lookup_inode()
on real ancestors to check for bad setup.

A simple optimization for docker and similar container managers -
if all layers roots have the same parent or same grandparent
(e.g. /var/lib/docker/overlay), we can skip the ancestors walk
on mount, but still keep the lookup sanity checks for post mount
tampering protection.

Thoughts?

Amir.

Re: [PATCH] ovl: relax WARN_ON() for overlapping layers use case

[Amir Goldstein]

On Sat, Mar 30, 2019 at 10:45 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Thu, Mar 28, 2019 at 4:38 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > This nasty little syzbot repro:
> > https://syzkaller.appspot.com/x/repro.syz?x=12c7a94f400000
> >
> > Creates overlay mounts where the same directory is both in upper
> > and lower layers. Simplified example:
> >
> >   mkdir foo work
> >   mount -t overlay none foo -o"lowerdir=.,upperdir=foo,workdir=work"
>
> Shouldn't the mount fail in this case?
>
> Does it make any sense to allow overlapping layers?
>
> If doing the check the dumb way, the number of d_ancestor() calls
> would increase quadratically with the number of layers, but I think
> it's possible to do it in linear time if necessary.
>

Miklos,

I saw you did not pick this one for next.
IMO, regardless of preventing mount with overlapping layers,
the WARN_ON() is inappropriate and this patch that replaces it with
pr_warn_reatelimited() has merit on its own.

WARN_ON() should reflect a case that we don't think is possible
in current code and as API constrain assertion.
Neither is the case here.
We know that it *is* possible to hit this case, even with checking overlapping
layers on mount and user does get an error when we hit the case.

Thanks,
Amir.

Re: [PATCH] ovl: relax WARN_ON() for overlapping layers use case

[Miklos Szeredi]

On Wed, May 8, 2019 at 1:03 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Sat, Mar 30, 2019 at 10:45 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Thu, Mar 28, 2019 at 4:38 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > This nasty little syzbot repro:
> > > https://syzkaller.appspot.com/x/repro.syz?x=12c7a94f400000
> > >
> > > Creates overlay mounts where the same directory is both in upper
> > > and lower layers. Simplified example:
> > >
> > >   mkdir foo work
> > >   mount -t overlay none foo -o"lowerdir=.,upperdir=foo,workdir=work"
> >
> > Shouldn't the mount fail in this case?
> >
> > Does it make any sense to allow overlapping layers?
> >
> > If doing the check the dumb way, the number of d_ancestor() calls
> > would increase quadratically with the number of layers, but I think
> > it's possible to do it in linear time if necessary.
> >
>
> Miklos,
>
> I saw you did not pick this one for next.
> IMO, regardless of preventing mount with overlapping layers,
> the WARN_ON() is inappropriate and this patch that replaces it with
> pr_warn_reatelimited() has merit on its own.
>
> WARN_ON() should reflect a case that we don't think is possible
> in current code and as API constrain assertion.
> Neither is the case here.
> We know that it *is* possible to hit this case, even with checking overlapping
> layers on mount and user does get an error when we hit the case.

Okay.  Picked up this one too.

Thanks,
Miklos