[PATCH] cifs: cope with negative dentries in cifs_get_root

[PATCH] cifs: cope with negative dentries in cifs_get_root

[Jeff Layton]

The loop around lookup_one_len doesn't handle the case where it might
return a negative dentry, which can cause an oops on the next pass
through the loop. Check for that and break out of the loop with an
error of -ENOENT if there is one.

Fixes the panic reported here:

    https://bugzilla.redhat.com/show_bug.cgi?id=727927

Reported-by: TR Bentley <home@trarbentley.net>
Reported-by: Iain Arnell <iarnell@gmail.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: stable@kernel.org
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/cifsfs.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 212e562..f93eb94 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -563,6 +563,10 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb)
 		mutex_unlock(&dir->i_mutex);
 		dput(dentry);
 		dentry = child;
+		if (!dentry->d_inode) {
+			dput(dentry);
+			dentry = ERR_PTR(-ENOENT);
+		}
 	} while (!IS_ERR(dentry));
 	_FreeXid(xid);
 	kfree(full_path);
-- 
1.7.6

Re: [PATCH] cifs: cope with negative dentries in cifs_get_root

[Al Viro]

On Fri, Aug 05, 2011 at 09:02:40AM -0400, Jeff Layton wrote:
> The loop around lookup_one_len doesn't handle the case where it might
> return a negative dentry, which can cause an oops on the next pass
> through the loop. Check for that and break out of the loop with an
> error of -ENOENT if there is one.

Ouch.  Nice catch and that one is my damn fault.  Do you want that one
to go through cifs or vfs tree?  ACK, in any case...

Re: [PATCH] cifs: cope with negative dentries in cifs_get_root

[Steve French]

On Fri, Aug 5, 2011 at 9:54 AM, Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org> wrote:
> On Fri, Aug 05, 2011 at 09:02:40AM -0400, Jeff Layton wrote:
>> The loop around lookup_one_len doesn't handle the case where it might
>> return a negative dentry, which can cause an oops on the next pass
>> through the loop. Check for that and break out of the loop with an
>> error of -ENOENT if there is one.
>
> Ouch.  Nice catch and that one is my damn fault.  Do you want that one
> to go through cifs or vfs tree?  ACK, in any case...

Probably safer to go through my tree, reduce chance of merge conflict
Was planning on sending a merge request up soon in any case.



-- 
Thanks,

Steve

Re: [PATCH] cifs: cope with negative dentries in cifs_get_root

[Steve French]

On Fri, Aug 5, 2011 at 8:02 AM, Jeff Layton <jlayton@redhat.com> wrote:
> The loop around lookup_one_len doesn't handle the case where it might
> return a negative dentry, which can cause an oops on the next pass
> through the loop. Check for that and break out of the loop with an
> error of -ENOENT if there is one.
>

merged into cifs-2.6.git

-- 
Thanks,

Steve

Re: [PATCH] cifs: cope with negative dentries in cifs_get_root

[Al Viro]

On Fri, Aug 05, 2011 at 10:01:47AM -0500, Steve French wrote:
> On Fri, Aug 5, 2011 at 9:54 AM, Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org> wrote:
> > On Fri, Aug 05, 2011 at 09:02:40AM -0400, Jeff Layton wrote:
> >> The loop around lookup_one_len doesn't handle the case where it might
> >> return a negative dentry, which can cause an oops on the next pass
> >> through the loop. Check for that and break out of the loop with an
> >> error of -ENOENT if there is one.
> >
> > Ouch. ?Nice catch and that one is my damn fault. ?Do you want that one
> > to go through cifs or vfs tree? ?ACK, in any case...
> 
> Probably safer to go through my tree, reduce chance of merge conflict
> Was planning on sending a merge request up soon in any case.

OK

Acked-by: Al Viro <viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org>

Re: [PATCH] cifs: cope with negative dentries in cifs_get_root

[Pavel Shilovsky]

2011/8/5 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> The loop around lookup_one_len doesn't handle the case where it might
> return a negative dentry, which can cause an oops on the next pass
> through the loop. Check for that and break out of the loop with an
> error of -ENOENT if there is one.
>
> Fixes the panic reported here:
>
>    https://bugzilla.redhat.com/show_bug.cgi?id=727927
>
> Reported-by: TR Bentley <home-XKIT1lZdXBwO+8BAsp4z4w@public.gmane.org>
> Reported-by: Iain Arnell <iarnell-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Al Viro <viro-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org>
> Cc: stable-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifsfs.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 212e562..f93eb94 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -563,6 +563,10 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb)
>                mutex_unlock(&dir->i_mutex);
>                dput(dentry);
>                dentry = child;
> +               if (!dentry->d_inode) {

dentry can be NULL here (returned from lookup_one_len) and it can
cause a null pointer dereference.

> +                       dput(dentry);
> +                       dentry = ERR_PTR(-ENOENT);
> +               }
>        } while (!IS_ERR(dentry));
>        _FreeXid(xid);
>        kfree(full_path);
> --
> 1.7.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH] cifs: cope with negative dentries in cifs_get_root

[Al Viro]

On Tue, Aug 09, 2011 at 09:41:33AM +0400, Pavel Shilovsky wrote:
> > ? ? ? ? ? ? ? ?mutex_unlock(&dir->i_mutex);
> > ? ? ? ? ? ? ? ?dput(dentry);
> > ? ? ? ? ? ? ? ?dentry = child;
> > + ? ? ? ? ? ? ? if (!dentry->d_inode) {
> 
> dentry can be NULL here (returned from lookup_one_len) and it can
> cause a null pointer dereference.

Not NULL, ERR_PTR(...).  IOW, we need to check IS_ERR(dentry) to protect
that check of dentry->d_inode.