On Mon, 08 Nov 2010 16:51:02 +0530 Suresh Jayaraman <[email protected]> wrote:
> On 11/08/2010 04:42 PM, Jeff Layton wrote: > > On Mon, 08 Nov 2010 14:05:03 +0530 > > Suresh Jayaraman <[email protected]> wrote: > > > >> On 11/08/2010 10:21 AM, Steve French wrote: > >> > >>> On Sun, Nov 7, 2010 at 8:12 PM, Jeff Layton <[email protected]> wrote: > >>>> On Sun, 7 Nov 2010 16:44:46 +0000 (GMT) > >>>> Kjell Rune Skaaraas <[email protected]> wrote: > >>>> > >>>>> After upgrading from 2.6.36 for other reasons, starting certain apps > >>>>> like wine utorrent.exe will cause a kernel oops. I run the x86_64 > >>>>> version of Ubuntu 10.10 with various modified packages all around and > >>>>> the 2.6.37-rc1 kernel from the kernel PPA team. I experienced the same > >>>>> with a kernel I tried compiling myself too. > >>>>> > >>>>> Nov �7 17:25:50 wodan kernel: [77498.450787] BUG: unable to handle > >>>>> kernel NULL pointer dereference at 0000000000000040 > >>>>> Nov �7 17:25:50 wodan kernel: [77498.450883] IP: [<ffffffffa0395729>] > >>>>> cifs_ioctl+0x39/0x2f0 [cifs] > >> > >> Does the below patch fixes your problem? > >> > >> > >> From: Suresh Jayaraman <[email protected]> > >> Subject: [PATCH] cifs: fix a NULL pointer dereference in cifs_ioctl() when > >> the fd is bad > >> > >> The commit ba00ba modified cifs_ioctl() to use tcon pointer in cifsFileInfo > >> via tlink instead of cifs_sb->tcon. When the file handle is not valid the > >> cifsFileInfo->tlink will be NULL. Fix this by getting the tcon pointer by > >> calling cifs_sb_master_tcon(). > >> > >> Here's a hackish reproducer: > >> > >> #include <fcntl.h> > >> #include <sys/ioctl.h> > >> #include <sys/stat.h> > >> #include <sys/types.h> > >> #include <unistd.h> > >> > >> #define CIFS_IOC_CHECKUMOUNT _IO(0xCF, 2) > >> > >> int main (int argc, char* argv[]) > >> { > >> int fd = open (argv[1], O_RDWR); > >> > >> ioctl(fd, CIFS_IOC_CHECKUMOUNT); > >> > >> close(fd); > >> return 0; > >> } > >> > >> This program will cause an oops when called with cifs mount point as an > >> argument. I have tested the fix with the reproducer and it no longer > >> oopses. > >> > >> Reported-by: Kjell Rune <[email protected]> > >> Cc: Jeff Layton <[email protected]> > >> Signed-off-by: Suresh Jayaraman <[email protected]> > >> --- > >> fs/cifs/ioctl.c | 6 ++---- > >> 1 files changed, 2 insertions(+), 4 deletions(-) > >> > >> diff --git a/fs/cifs/ioctl.c b/fs/cifs/ioctl.c > >> index 2fa22f2..b8f680a 100644 > >> --- a/fs/cifs/ioctl.c > >> +++ b/fs/cifs/ioctl.c > >> @@ -35,10 +35,10 @@ long cifs_ioctl(struct file *filep, unsigned int > >> command, unsigned long arg) > >> struct inode *inode = filep->f_dentry->d_inode; > >> int rc = -ENOTTY; /* strange error - but the precedent */ > >> int xid; > >> - struct cifs_sb_info *cifs_sb; > >> + struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb); > >> #ifdef CONFIG_CIFS_POSIX > >> struct cifsFileInfo *pSMBFile = filep->private_data; > >> - struct cifsTconInfo *tcon = tlink_tcon(pSMBFile->tlink); > >> + struct cifsTconInfo *tcon = cifs_sb_master_tcon(cifs_sb); > >> __u64 ExtAttrBits = 0; > >> __u64 ExtAttrMask = 0; > >> __u64 caps = le64_to_cpu(tcon->fsUnixInfo.Capability); > >> @@ -48,8 +48,6 @@ long cifs_ioctl(struct file *filep, unsigned int > >> command, unsigned long arg) > >> > >> cFYI(1, "ioctl file %p cmd %u arg %lu", filep, command, arg); > >> > >> - cifs_sb = CIFS_SB(inode->i_sb); > >> - > >> switch (command) { > >> case CIFS_IOC_CHECKUMOUNT: > >> cFYI(1, "User unmount attempted"); > > > > NAK. This will mean that you're allowing people to do ioctls against > > I missed this.. though I was not quite sure about this patch.. > > > files using other people's credentials. If this is the problem then the > > I'm sure the problem is pSMBFile->tlink is NULL. And this is because > open() fails and the test program didn't check for the error and > continues using the return value (in this case -1 which will be > UNSIGNED_INT_MAX) in the ioctl. Since the file is actually not open, > cifs_new_fileinfo() won't have a chance to setup pSMBFile->tlink properly. > That doesn't sound right. The VFS shouldn't allow an ioctl on an invalid file descriptor... It seems more likely that pSMBFile is actually NULL here. cifs doesn't have f_op->open routine for directories, so that's entirely handled at the VFS layer. The right fix is probably to add one, and make sure that filp->private_data is appropriately populated on the open call for directories. -- Jeff Layton <[email protected]> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
