dcache: NULL ptr deref in dentry_kill

dcache: NULL ptr deref in dentry_kill

[Sasha Levin]

Hi all,

While fuzzing with trinity inside a KVM tools guest running the latest -next
kernel, I've stumbled on the following spew:

[  434.580818] BUG: unable to handle kernel NULL pointer dereference at 0000000000000090
[  434.582208] IP: do_raw_spin_trylock (./arch/x86/include/asm/spinlock.h:108 kernel/locking/spinlock_debug.c:143)
[  434.583243] PGD 0
[  434.583602] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[  434.584568] Dumping ftrace buffer:
[  434.585202]    (ftrace buffer empty)
[  434.585888] Modules linked in:
[  434.586416] CPU: 2 PID: 10385 Comm: trinity-c54 Not tainted 3.17.0-rc7-next-20141003-sasha-00039-g5069327 #1323
[  434.588072] task: ffff880051593000 ti: ffff88098708c000 task.ti: ffff88098708c000
[  434.589373] RIP: do_raw_spin_trylock (./arch/x86/include/asm/spinlock.h:108 kernel/locking/spinlock_debug.c:143)
[  434.590025] RSP: 0000:ffff88098708fbf0  EFLAGS: 00010292
[  434.590025] RAX: 0000000000000002 RBX: 0000000000000090 RCX: 0000000000000000
[  434.590025] RDX: 0000000000000000 RSI: ffff880170c22928 RDI: 0000000000000090
[  434.590025] RBP: ffff88098708fc18 R08: 0000000000000001 R09: 0000000000000000
[  434.590025] R10: 0000000000000000 R11: 0000000000000000 R12: ffff880170c22910
[  434.590025] R13: 0000000000000000 R14: 0000000000000000 R15: ffff880170c22880
[  434.590025] FS:  00007fa20164e700(0000) GS:ffff880177a00000(0000) knlGS:0000000000000000
[  434.590025] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  434.590025] CR2: 0000000000000090 CR3: 000000001de31000 CR4: 00000000000006a0
[  434.590025] DR0: 00000000006ef000 DR1: 00000000006ef000 DR2: 0000000000000000
[  434.590025] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000600
[  434.590025] Stack:
[  434.590025]  ffffffff9c525d23 ffff88098708fc18 ffffffff99b56b41 ffff88098708fc18
[  434.590025]  ffff880170c22880 ffff88098708fc48 ffffffff99349126 ffff880174260700
[  434.590025]  0000000000000008 ffff880a5a998ac0 ffff8805c2e60020 ffff88098708fc98
[  434.590025] Call Trace:
[  434.590025] ? _raw_spin_trylock (include/linux/spinlock_api_smp.h:89 kernel/locking/spinlock.c:135)
[  434.590025] ? lockref_put_or_lock (lib/lockref.c:131)
[  434.590025] dput (fs/dcache.c:513 fs/dcache.c:616)
[  434.590025] __fput (fs/file_table.c:235)
[  434.590025] ____fput (fs/file_table.c:253)
[  434.590025] task_work_run (kernel/task_work.c:125 (discriminator 1))
[  434.590025] do_exit (kernel/exit.c:761)
[  434.590025] ? get_signal (kernel/signal.c:2332)
[  434.590025] ? debug_smp_processor_id (lib/smp_processor_id.c:57)
[  434.590025] ? put_lock_stats.isra.12 (./arch/x86/include/asm/preempt.h:98 kernel/locking/lockdep.c:254)
[  434.590025] ? _raw_spin_unlock_irq (./arch/x86/include/asm/paravirt.h:819 include/linux/spinlock_api_smp.h:168 kernel/locking/spinlock.c:199)
[  434.590025] do_group_exit (kernel/exit.c:890)
[  434.590025] get_signal (kernel/signal.c:2350)
[  434.590025] ? kvm_clock_read (./arch/x86/include/asm/preempt.h:90 arch/x86/kernel/kvmclock.c:86)
[  434.590025] ? sched_clock_local (kernel/sched/clock.c:214)
[  434.590025] do_signal (arch/x86/kernel/signal.c:703)
[  434.590025] ? get_parent_ip (kernel/sched/core.c:2606)
[  434.590025] ? context_tracking_user_exit (include/linux/vtime.h:89 include/linux/jump_label.h:114 include/trace/events/context_tracking.h:47 kernel/context_tracking.c:180)
[  434.590025] ? context_tracking_user_exit (./arch/x86/include/asm/paravirt.h:809 (discriminator 2) kernel/context_tracking.c:184 (discriminator 2))
[  434.590025] ? __this_cpu_preempt_check (lib/smp_processor_id.c:63)
[  434.590025] ? trace_hardirqs_on_caller (kernel/locking/lockdep.c:2559 kernel/locking/lockdep.c:2601)
[  434.590025] ? trace_hardirqs_on (kernel/locking/lockdep.c:2609)
[  434.590025] do_notify_resume (arch/x86/kernel/signal.c:756)
[  434.590025] int_signal (arch/x86/kernel/entry_64.S:587)
[  434.590025] Code: 65 f0 89 43 08 4c 8b 6d f8 65 48 8b 04 25 c0 cb 00 00 48 89 43 10 48 8b 5d e8 c9 c3 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 <8b> 0f 55 31 f6 48 89 e5 89 ca 0f b7 c1 c1 ea 10 83 e2 fe 39 d0
[  434.590025] RIP do_raw_spin_trylock (./arch/x86/include/asm/spinlock.h:108 kernel/locking/spinlock_debug.c:143)
[  434.590025]  RSP <ffff88098708fbf0>
[  434.590025] CR2: 0000000000000090


Thanks,
Sasha

Re: dcache: NULL ptr deref in dentry_kill

[Al Viro]

On Sun, Oct 05, 2014 at 08:27:47PM -0400, Sasha Levin wrote:

> [  434.580818] BUG: unable to handle kernel NULL pointer dereference at 0000000000000090
> [  434.582208] IP: do_raw_spin_trylock (./arch/x86/include/asm/spinlock.h:108 kernel/locking/spinlock_debug.c:143)
[snip]
spin_lock((void *)0x90)
> [  434.590025] ? _raw_spin_trylock (include/linux/spinlock_api_smp.h:89 kernel/locking/spinlock.c:135)
> [  434.590025] ? lockref_put_or_lock (lib/lockref.c:131)
> [  434.590025] dput (fs/dcache.c:513 fs/dcache.c:616)

ummm...  lockref_put_or_lock(&dentry->d_lockref) ending up with 0x90 passed
to lockref_put_or_lock()...  What offset does d_lockref have on your build?
On my config it's 0x80 (i.e. that would mean dput((void *)0x10)); could you
check it on yours?  Do you have CONFIG_DEBUG_LOCK_ALLOC and CONFIG_LOCK_STAT?

> [  434.590025] __fput (fs/file_table.c:235)

... and here we have a struct file with ->f_path.dentry being a small positive
number.

Might make sense to slap BUG_ON((unsigned long)dentry < PAGE_SIZE); into
__fput() and try to reproduce.  And throw
	BUG_ON((unsigned long)file->f_path.dentry < PAGE_SIZE);
into fput(), while we are at it, to see if that's bogus ->f_path.dentry at
fput() time or if it's getting corrupted between task_work_add() and actual
execution of __fput().

And the value of offsetof(struct dentry, d_lock) on your build would be
interesting to see - that would tell us which value are we seeing passed
to dput().

Re: dcache: NULL ptr deref in dentry_kill

[Sasha Levin]

On 10/05/2014 11:13 PM, Al Viro wrote:
> On Sun, Oct 05, 2014 at 08:27:47PM -0400, Sasha Levin wrote:
> 
>> [  434.580818] BUG: unable to handle kernel NULL pointer dereference at 0000000000000090
>> [  434.582208] IP: do_raw_spin_trylock (./arch/x86/include/asm/spinlock.h:108 kernel/locking/spinlock_debug.c:143)
> [snip]
> spin_lock((void *)0x90)
>> [  434.590025] ? _raw_spin_trylock (include/linux/spinlock_api_smp.h:89 kernel/locking/spinlock.c:135)
>> [  434.590025] ? lockref_put_or_lock (lib/lockref.c:131)
>> [  434.590025] dput (fs/dcache.c:513 fs/dcache.c:616)
> 
> ummm...  lockref_put_or_lock(&dentry->d_lockref) ending up with 0x90 passed
> to lockref_put_or_lock()...  What offset does d_lockref have on your build?

0x90

> On my config it's 0x80 (i.e. that would mean dput((void *)0x10)); could you
> check it on yours?  Do you have CONFIG_DEBUG_LOCK_ALLOC and CONFIG_LOCK_STAT?

I have both of the config options.

>> [  434.590025] __fput (fs/file_table.c:235)
> 
> ... and here we have a struct file with ->f_path.dentry being a small positive
> number.
> 
> Might make sense to slap BUG_ON((unsigned long)dentry < PAGE_SIZE); into
> __fput() and try to reproduce.  And throw
> 	BUG_ON((unsigned long)file->f_path.dentry < PAGE_SIZE);
> into fput(), while we are at it, to see if that's bogus ->f_path.dentry at
> fput() time or if it's getting corrupted between task_work_add() and actual
> execution of __fput().

I'll put those BUG_ONs in and will update if they get hit.

> And the value of offsetof(struct dentry, d_lock) on your build would be
> interesting to see - that would tell us which value are we seeing passed
> to dput().

This is 0x90 as well.


Thanks,
Sasha

Re: dcache: NULL ptr deref in dentry_kill

[Al Viro]

On Sun, Oct 05, 2014 at 11:42:40PM -0400, Sasha Levin wrote:
> On 10/05/2014 11:13 PM, Al Viro wrote:
> > On Sun, Oct 05, 2014 at 08:27:47PM -0400, Sasha Levin wrote:
> > 
> >> [  434.580818] BUG: unable to handle kernel NULL pointer dereference at 0000000000000090
> >> [  434.582208] IP: do_raw_spin_trylock (./arch/x86/include/asm/spinlock.h:108 kernel/locking/spinlock_debug.c:143)
> > [snip]
> > spin_lock((void *)0x90)
> >> [  434.590025] ? _raw_spin_trylock (include/linux/spinlock_api_smp.h:89 kernel/locking/spinlock.c:135)
> >> [  434.590025] ? lockref_put_or_lock (lib/lockref.c:131)
> >> [  434.590025] dput (fs/dcache.c:513 fs/dcache.c:616)
> > 
> > ummm...  lockref_put_or_lock(&dentry->d_lockref) ending up with 0x90 passed
> > to lockref_put_or_lock()...  What offset does d_lockref have on your build?
> 
> 0x90

Huh???  It means that we got to that lockref_put_or_lock with dentry == NULL.
But that makes no sense at all - we have
void dput(struct dentry *dentry)
{
        if (unlikely(!dentry))
                return;

repeat:
        if (lockref_put_or_lock(&dentry->d_lockref))
                return;
	...
and the only branch to repeat: is
        if (dentry)
                goto repeat;

If we get to that lockref_put_or_lock() with dentry == NULL, something's
very wrong with compiler.  And the only other lockref_put_or_lock() in
there is
                while (dentry && !lockref_put_or_lock(&dentry->d_lockref)) {
which would also make NULL dentry a miscompile.

Could you put fs/dcache.s for your build on some anonftp?  That really
smells like compiler breakage; had the address it tried to access been
close but not equal to that offsetof(), we would be dealing with bogus
->f_path.dentry (close to, but not quite NULL).  As it is, it looks like
dput() somehow getting to that line with NULL dentry, which should've
been prevented by the checks there...

Re: dcache: NULL ptr deref in dentry_kill

[Sasha Levin]

On 10/06/2014 12:25 AM, Al Viro wrote:
> On Sun, Oct 05, 2014 at 11:42:40PM -0400, Sasha Levin wrote:
>> On 10/05/2014 11:13 PM, Al Viro wrote:
>>> On Sun, Oct 05, 2014 at 08:27:47PM -0400, Sasha Levin wrote:
>>>
>>>> [  434.580818] BUG: unable to handle kernel NULL pointer dereference at 0000000000000090
>>>> [  434.582208] IP: do_raw_spin_trylock (./arch/x86/include/asm/spinlock.h:108 kernel/locking/spinlock_debug.c:143)
>>> [snip]
>>> spin_lock((void *)0x90)
>>>> [  434.590025] ? _raw_spin_trylock (include/linux/spinlock_api_smp.h:89 kernel/locking/spinlock.c:135)
>>>> [  434.590025] ? lockref_put_or_lock (lib/lockref.c:131)
>>>> [  434.590025] dput (fs/dcache.c:513 fs/dcache.c:616)
>>>
>>> ummm...  lockref_put_or_lock(&dentry->d_lockref) ending up with 0x90 passed
>>> to lockref_put_or_lock()...  What offset does d_lockref have on your build?
>>
>> 0x90
> 
> Huh???  It means that we got to that lockref_put_or_lock with dentry == NULL.
> But that makes no sense at all - we have
> void dput(struct dentry *dentry)
> {
>         if (unlikely(!dentry))
>                 return;
> 
> repeat:
>         if (lockref_put_or_lock(&dentry->d_lockref))
>                 return;
> 	...
> and the only branch to repeat: is
>         if (dentry)
>                 goto repeat;
> 
> If we get to that lockref_put_or_lock() with dentry == NULL, something's
> very wrong with compiler.  And the only other lockref_put_or_lock() in
> there is
>                 while (dentry && !lockref_put_or_lock(&dentry->d_lockref)) {
> which would also make NULL dentry a miscompile.
> 
> Could you put fs/dcache.s for your build on some anonftp?  That really
> smells like compiler breakage; had the address it tried to access been
> close but not equal to that offsetof(), we would be dealing with bogus
> ->f_path.dentry (close to, but not quite NULL).  As it is, it looks like
> dput() somehow getting to that line with NULL dentry, which should've
> been prevented by the checks there...

I think you've misread the stack trace. The lockref_put_or_lock isn't reliable
(probably just a derelict from the previous, successful call to it), and the
stack trace's reliable symbols takes us elsewhere.

Stack trace shows that:

[  434.590025] dput (fs/dcache.c:513 fs/dcache.c:616)
[  434.590025] __fput (fs/file_table.c:235)
[  434.590025] ____fput (fs/file_table.c:253)

Looking at fs/dcache.c:616 we see:

	kill_it:
	        dentry = dentry_kill(dentry); <=== This
        	if (dentry)
                	goto repeat;

And within dentry_kill() (fs/dcache.c:513):

        if (!IS_ROOT(dentry)) {
                parent = dentry->d_parent;
                if (unlikely(!spin_trylock(&parent->d_lock))) { <=== here
                        if (inode)
                                spin_unlock(&inode->i_lock);
                        goto failed;

We're trying to deref a NULL 'parent'.

Looking at git history, this check was slightly changed to remove the
'parent != NULL' condition in e55fd01154 ("split dentry_kill()"):

	-       }
	        if (!IS_ROOT(dentry))
	                parent = dentry->d_parent;
	-       if (parent && !spin_trylock(&parent->d_lock)) {
	-               if (inode)
	-                       spin_unlock(&inode->i_lock);
	[...]
	+       if (!IS_ROOT(dentry)) {
	+               parent = dentry->d_parent;
	+               if (unlikely(!spin_trylock(&parent->d_lock))) {
	+                       if (inode)
	+                               spin_unlock(&inode->i_lock);
	+                       goto failed;
	+               }
	+       }

So this is a case where dentry is not root, but has parent set to NULL.

Although I don't see how that can happen.


Thanks,
Sasha

Re: dcache: NULL ptr deref in dentry_kill

[Al Viro]

On Mon, Oct 06, 2014 at 12:39:16AM -0400, Sasha Levin wrote:
>         if (!IS_ROOT(dentry)) {
>                 parent = dentry->d_parent;
>                 if (unlikely(!spin_trylock(&parent->d_lock))) { <=== here
>                         if (inode)
>                                 spin_unlock(&inode->i_lock);
>                         goto failed;
> 
> We're trying to deref a NULL 'parent'.
 
->d_parent is *never* NULL.  There are very few places where it's modified,
all of them in fs/dcache.c:
fs/dcache.c:1416:       dentry->d_parent = dentry;
fs/dcache.c:1453:       dentry->d_parent = parent;
fs/dcache.c:2478:               dentry->d_parent = target->d_parent;
fs/dcache.c:2479:               target->d_parent = target;
fs/dcache.c:2484:               swap(dentry->d_parent, target->d_parent);

The fifth one exchanges two something->d_parent.  Can't introduce NULL.
Neither can the third one (again, foo->d_parent = bar->d_parent).  The
first and the fourth are also obvious - p->d_parent = p will oops with
p == NULL and store a non-NULL otherwise.  Which leaves the second -
d_alloc().  And there the lines immediately after that assignment are
        list_add(&dentry->d_u.d_child, &parent->d_subdirs);
        spin_unlock(&parent->d_lock);
which would oops with parent == NULL.

Dentries are allocated by __d_alloc().  By the time somebody might
observe them, they already have non-NULL ->d_parent.  And they never
get it set to NULL afterwards.  I don't see any variables (auto or not)
of type struct dentry and I don't see anything that would contain
struct dentry as a field.  It doesn't guarantee that nobody manages
to allocate one somehow or hide a conversion of some strange pointer to
struct dentry *, but any such place would be very likely to trigger tons
of oopsen - it would have to manage to hide initialization of ->d_subdirs,
->d_lru, ->d_alias, etc. and it's hard to do accidentally.

Another possibility is that this pointer either never went to struct dentry
or used to point to one in times long past, and memory had been zeroed
since then.  Or that something has shat some zeroes into a real struct
dentry, corrupting ->d_parent in process.

But any struct dentry with NULL ->d_parent is a serious bug.  That really
should never, ever happen.