Re: NULL pointer dereference

From: Eric Paris
Date: Wed Sep 02 2009 - 17:09:42 EST

Next message: Rafael J. Wysocki: "Re: [PATCH V6 1/2] introduce ALS sysfs class"
Previous message: Darren Hart: "Re: RFC: methods of sched event tracing and my assumptions"
In reply to: Frans Pop: "Re: NULL pointer dereference"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, 2009-09-02 at 17:59 +0200, Frans Pop wrote:
> Adding CCs.
>
> The commit that introduced ima_counts_put is:
>
> commit 94e5d714f604d4cb4cb13163f01ede278e69258b
> Author: Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx>
> Date: Fri Jun 26 14:05:27 2009 -0400
> integrity: add ima_counts_put (updated)
>
> This patch fixes an imbalance message as reported by J.R. Okajima.
> The IMA file counters are incremented in ima_path_check. If the
> actual open fails, such as ETXTBSY, decrement the counters to
> prevent unnecessary imbalance messages.
>
> Possibly this has already been fixed by the following commit? It seems
> unlikely though as that was a very early commit after -rc7 and the problem
> kernel was -rc7.git2 (if I read the version correctly).
>
> commit 53a7197aff20e341487fca8575275056fe1c63e5
> Author: Eric Paris <eparis@xxxxxxxxxx>
> Date: Wed Aug 26 14:56:48 2009 -0400
> IMA: iint put in ima_counts_get and put
>
> ima_counts_get() calls ima_iint_find_insert_get() which takes a reference
> to the iint in question, but does not put that reference at the end of the
> function. This can lead to a nasty memory leak.
>
> Cheers,
> FJP
>
> Ciprian Docan wrote:
> > Hi,
> >
> > I got the following in dmesg:
> >
> > BUG: unable to handle kernel NULL pointer dereference at 00000000000000ae
> > IP: [<ffffffff8124701a>] ima_counts_put+0x34/0xb1
> > PGD a05e1067 PUD a05e2067 PMD 0
> > Oops: 0000 [#1] SMP
> > last sysfs file: /sys/devices/pci0000:00/0000:00:1f.3/class
> > CPU 1
> > Modules linked in: fuse sit tunnel4 nfs fscache nfsd lockd nfs_acl
> > auth_rpcgss exportfs autofs4 sunrpc ip6t_REJECT nf_conntrack_ipv6
> > ip6table_filter ip6_tables ipv6 dm_multipath uinput snd_hda_codec_analog
> > snd_hda_intel snd_hda_codec ppdev snd_hwdep snd_pcm parport_pc serio_raw
> > dcdbas snd_timer iTCO_wdt iTCO_vendor_support parport wmi snd soundcore
> > snd_page_alloc i2c_i801 usb_storage e1000e pata_acpi ata_generic i915 drm
> > i2c_algo_bit i2c_core video output [last unloaded: speedstep_lib]
> > Pid: 3898, comm: devkit-disks-da Not tainted
> > 2.6.31-0.174.rc7.git2.fc12.x86_64 #1 OptiPlex 760
> > RIP: 0010:[<ffffffff8124701a>] [<ffffffff8124701a>]
> > ima_counts_put+0x34/0xb1
> > RSP: 0000:ffff8800a05f5d78 EFLAGS: 00010202
> > RAX: ffff8800b8f34148 RBX: 0000000000000004 RCX: 00000000b7698c0f
> > RDX: 0000000000000000 RSI: 0000000000000004 RDI: 0000000000000000
> > RBP: ffff8800a05f5da8 R08: ffff8800a05f5ce8 R09: ffff8800a05f5d18
> > R10: 00000000b7698c0f R11: 0000000000000000 R12: 0000000000000024
> > R13: 0000000000000000 R14: ffff8800a05f5e28 R15: fffffffffffffffa
> > FS: 00007ff4a8490790(0000) GS:ffff880007bde000(0000)
> > knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> > CR2: 00000000000000ae CR3: 00000000a05e0000 CR4: 00000000000406e0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> > Process devkit-disks-da (pid: 3898, threadinfo ffff8800a05f4000, task
> > ffff8800a05f8000)
> > Stack:
> > ffff8800a05f5da8 00000000b7698c0f 0000000000000024 0000000000008001
> > <0> 0000000000000024 0000000000000000 ffff8800a05f5ef8 ffffffff8114e4b8
> > <0> ffff8800a05f5dc8 ffffffffffffff9c ffff8800a05f5dd8 00000000b7698c0f
> > Call Trace:
> > [<ffffffff8114e4b8>] do_filp_open+0x534/0x9f3
> > [<ffffffff810956f1>] ? lock_release_holdtime+0x3f/0x146
> > [<ffffffff811146c4>] ? might_fault+0x71/0xd9
> > [<ffffffff8115a407>] ? alloc_fd+0x125/0x14b
> > [<ffffffff8113f3f1>] do_sys_open+0x71/0x131
> > [<ffffffff8113f51e>] sys_open+0x33/0x49
> > [<ffffffff81012f42>] system_call_fastpath+0x16/0x1b
> > Code: 48 83 ec 18 0f 1f 44 00 00 65 48 8b 04 25 28 00 00 00 48 89 45 d8 31
> > c0 83 3d 36 22 28 01 00 48 8b 47 08 89 f3 48 8b 78 50 74 5e <0f> b7 87 ae
> > 00 00 00 25 00 f0 00 00 3d 00 80 00 00 75 4b e8 da
> > RIP [<ffffffff8124701a>] ima_counts_put+0x34/0xb1
> > RSP <ffff8800a05f5d78>
> > CR2: 00000000000000ae
> > ---[ end trace 027f1f2d55021c25 ]---
> >
> > Kernel version is: 2.6.31-0.174.rc7.git2.fc12.x86_64 #1 SMP Mon Aug 24
> > 23:25:34 EDT 2009 x86_64 x86_64 x86_64 GNU/Linux, from F12 rawhide
> > repository.
> >
> > I am not sure what caused this, as it happened over night, and the machine
> > was idle. The machine is still up and running, so if you need more
> > information about it please let me know and I will try to provide them.
> >
> > Thank you,
> > --
> > Ciprian Docan

Here's the code:

void ima_counts_put(struct path *path, int mask)
{
struct inode *inode = path->dentry->d_inode;
struct ima_iint_cache *iint;

if (!ima_initialized || !S_ISREG(inode->i_mode))
return;
[blah blah blah]

Here's the assembly:

[snip]
ffffffff81247007: 83 3d 36 22 28 01 00 cmpl $0x0,0x1282236(%rip) # ffffffff824c9244 <ima_initialized>
ffffffff8124700e: 48 8b 47 08 mov 0x8(%rdi),%rax
ffffffff81247012: 89 f3 mov %esi,%ebx
ffffffff81247014: 48 8b 78 50 mov 0x50(%rax),%rdi
ffffffff81247018: 74 5e je ffffffff81247078 <ima_counts_put+0x92>
ffffffff8124701a: 0f b7 87 ae 00 00 00 movzwl 0xae(%rdi),%eax <-------- BUG HERE
ffffffff81247021: 25 00 f0 00 00 and $0xf000,%eax
ffffffff81247026: 3d 00 80 00 00 cmp $0x8000,%eax
ffffffff8124702b: 75 4b jne ffffffff81247078 <ima_counts_put+0x92>
[snip]

Pretty clear that we checked ima_initialized (and it was !0) so we moved
on to dereference inode->i_mode. But inode was NULL; From the calling
code we have:

/* Negative dentry, just create the file */
if (!path.dentry->d_inode) {
/*
* This write is needed to ensure that a
* ro->rw transition does not occur between
* the time when the file is created and when
* a permanent write count is taken through
* the 'struct file' in nameidata_to_filp().
*/
error = mnt_want_write(nd.path.mnt);
if (error)
goto exit_mutex_unlock;
error = __open_namei_create(&nd, &path, flag, mode);
if (error) {
mnt_drop_write(nd.path.mnt);
goto exit;
}
filp = nameidata_to_filp(&nd, open_flag);
if (IS_ERR(filp))
ima_counts_put(&nd.path,
acc_mode & (MAY_READ | MAY_WRITE |
MAY_EXEC));

Which I guess has some negative dentry logic the ima code isn't properly
accounting for. Mimi can you try vert that __open_namei_create
returning 0 ALWAYS means it's a positive dentry? If it isn't always a
positive figure out if the ima_counts_put() call is needed?

-Eric

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Rafael J. Wysocki: "Re: [PATCH V6 1/2] introduce ALS sysfs class"
Previous message: Darren Hart: "Re: RFC: methods of sched event tracing and my assumptions"
In reply to: Frans Pop: "Re: NULL pointer dereference"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]