From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suresh Jayaraman Subject: Re: Kernel oops: NULL pointer dereference in cifs_ioctl on 2.6.37-rc1 Date: Mon, 08 Nov 2010 16:51:02 +0530 Message-ID: <4CD7DD1E.3030601@suse.de> References: <484246.91210.qm@web27103.mail.ukl.yahoo.com> <20101107211202.3b3468dd@corrin.poochiereds.net> <4CD7B637.1070004@suse.de> <20101108061227.051706ff@tlielax.poochiereds.net> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Steve French , Kjell Rune Skaaraas , linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Jeff Layton Return-path: In-Reply-To: <20101108061227.051706ff-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org> Sender: linux-cifs-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-ID: On 11/08/2010 04:42 PM, Jeff Layton wrote: > On Mon, 08 Nov 2010 14:05:03 +0530 > Suresh Jayaraman wrote: >=20 >> On 11/08/2010 10:21 AM, Steve French wrote: >> >>> On Sun, Nov 7, 2010 at 8:12 PM, Jeff Layton wro= te: >>>> On Sun, 7 Nov 2010 16:44:46 +0000 (GMT) >>>> Kjell Rune Skaaraas wrote: >>>> >>>>> After upgrading from 2.6.36 for other reasons, starting certain a= pps like wine utorrent.exe will cause a kernel oops. I run the x86_64 v= ersion of Ubuntu 10.10 with various modified packages all around and th= e 2.6.37-rc1 kernel from the kernel PPA team. I experienced the same wi= th a kernel I tried compiling myself too. >>>>> >>>>> Nov =EF=BF=BD7 17:25:50 wodan kernel: [77498.450787] BUG: unable = to handle kernel NULL pointer dereference at 0000000000000040 >>>>> Nov =EF=BF=BD7 17:25:50 wodan kernel: [77498.450883] IP: [] cifs_ioctl+0x39/0x2f0 [cifs] >> >> Does the below patch fixes your problem? >> >> >> From: Suresh Jayaraman >> 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 cifsF= ileInfo >> via tlink instead of cifs_sb->tcon. When the file handle is not vali= d the >> cifsFileInfo->tlink will be NULL. Fix this by getting the tcon point= er by >> calling cifs_sb_master_tcon(). >> >> Here's a hackish reproducer: >> >> #include >> #include >> #include >> #include >> #include >> >> #define CIFS_IOC_CHECKUMOUNT _IO(0xCF, 2) >> >> int main (int argc, char* argv[]) >> { >> int fd =3D 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 >> Cc: Jeff Layton =20 >> Signed-off-by: Suresh Jayaraman >> --- >> 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 =3D filep->f_dentry->d_inode; >> int rc =3D -ENOTTY; /* strange error - but the precedent */ >> int xid; >> - struct cifs_sb_info *cifs_sb; >> + struct cifs_sb_info *cifs_sb =3D CIFS_SB(inode->i_sb); >> #ifdef CONFIG_CIFS_POSIX >> struct cifsFileInfo *pSMBFile =3D filep->private_data; >> - struct cifsTconInfo *tcon =3D tlink_tcon(pSMBFile->tlink); >> + struct cifsTconInfo *tcon =3D cifs_sb_master_tcon(cifs_sb); >> __u64 ExtAttrBits =3D 0; >> __u64 ExtAttrMask =3D 0; >> __u64 caps =3D le64_to_cpu(tcon->fsUnixInfo.Capability); >> @@ -48,8 +48,6 @@ long cifs_ioctl(struct file *filep, unsigned int c= ommand, unsigned long arg) >> =20 >> cFYI(1, "ioctl file %p cmd %u arg %lu", filep, command, arg); >> =20 >> - cifs_sb =3D CIFS_SB(inode->i_sb); >> - >> switch (command) { >> case CIFS_IOC_CHECKUMOUNT: >> cFYI(1, "User unmount attempted"); >=20 > 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 t= he 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 proper= ly. --=20 Suresh Jayaraman