| Message ID | 1312549360-4317-1-git-send-email-jlayton@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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... -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 5, 2011 at 9:54 AM, Al Viro <viro@zeniv.linux.org.uk> 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.
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
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@zeniv.linux.org.uk> 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@zeniv.linux.org.uk> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2011/8/5 Jeff Layton <jlayton@redhat.com>: > 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) { 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
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. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)