On Mon, 08 Nov 2010 16:51:02 +0530
Suresh Jayaraman <sjayaraman@xxxxxxx> wrote:
> On 11/08/2010 04:42 PM, Jeff Layton wrote:
> > On Mon, 08 Nov 2010 14:05:03 +0530
> > Suresh Jayaraman <sjayaraman@xxxxxxx> wrote:
> >
> >> On 11/08/2010 10:21 AM, Steve French wrote:
> >>
> >>> On Sun, Nov 7, 2010 at 8:12 PM, Jeff Layton <jlayton@xxxxxxxxx> wrote:
> >>>> On Sun, 7 Nov 2010 16:44:46 +0000 (GMT)
> >>>> Kjell Rune Skaaraas <kjella79@xxxxxxxx> 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 <sjayaraman@xxxxxxx>
> >> 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 <kjella79@xxxxxxxx>
> >> Cc: Jeff Layton <jlayton@xxxxxxxxxx>
> >> Signed-off-by: Suresh Jayaraman <sjayaraman@xxxxxxx>
> >> ---
> >> 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 <jlayton@xxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
