net: decnet: NULL ptr deref on connect()

net: decnet: NULL ptr deref on connect()

[Sasha Levin]

Hi all,

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

[  279.107409] BUG: unable to handle kernel NULL pointer dereference at           (null)
[  279.108676] IP: dnet_select_source.isra.25 (net/decnet/dn_route.c:926)
[  279.109876] PGD 19dd92067 PUD 1a25ab067 PMD 0
[  279.110186] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[  279.110186] Dumping ftrace buffer:
[  279.110186]    (ftrace buffer empty)
[  279.110186] Modules linked in:
[  279.110186] CPU: 1 PID: 17317 Comm: trinity-c78 Not tainted 3.14.0-next-20140403-sasha-00022-g10224c0 #377
[  279.110186] task: ffff880196c60000 ti: ffff8801b6e8a000 task.ti: ffff8801b6e8a000
[  279.110186] RIP: dnet_select_source.isra.25 (net/decnet/dn_route.c:926)
[  279.110186] RSP: 0018:ffff8801b6e8bc88  EFLAGS: 00010202
[  279.110186] RAX: 0000000000000001 RBX: 0000000000000000 RCX: 0000000000000001
[  279.110186] RDX: 0000000000000001 RSI: ffffffffa9e88100 RDI: 0000000000000282
[  279.110186] RBP: ffff8801b6e8bcb8 R08: 0000000000000001 R09: ffff880196c60cf0
[  279.110186] R10: 0000000000000001 R11: 0000000000000000 R12: ffff8801b6e8be18
[  279.110186] R13: 0000000000000000 R14: 00000000000000fe R15: 0000000000000000
[  279.110186] FS:  00007f333a961700(0000) GS:ffff880063000000(0000) knlGS:0000000000000000
[  279.110186] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  279.110186] CR2: 0000000000000000 CR3: 000000019cc2d000 CR4: 00000000000006a0
[  279.110186] DR0: 0000000000696000 DR1: 0000000000696000 DR2: 0000000000696000
[  279.110186] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000600
[  279.110186] Stack:
[  279.110186]  ffffffffa82c3225 ffffffffa507aac5 ffff880436a39160 ffff8801b6e8be18
[  279.110186]  0000000000000000 ffff8800c5dc7408 ffff8801b6e8bd68 ffffffffa82c5803
[  279.110186]  ffff880196c60000 0000000000000007 0000000000000006 0000000000000082
[  279.110186] Call Trace:
[  279.110186] ? dnet_select_source.isra.25 (net/decnet/dn_route.c:916)
[  279.110186] ? sched_clock (arch/x86/include/asm/paravirt.h:192 arch/x86/kernel/tsc.c:305)
[  279.110186] dn_route_output_slow (net/decnet/dn_route.c:1042)
[  279.110186] __dn_route_output_key (net/decnet/dn_route.c:1267)
[  279.110186] ? __dn_route_output_key (include/linux/bottom_half.h:19 include/linux/rcupdate.h:850 net/decnet/dn_route.c:1249)
[  279.110186] dn_route_output_sock (net/decnet/dn_route.c:1290)
[  279.110186] __dn_connect (net/decnet/af_decnet.c:954)
[  279.110186] ? __local_bh_enable_ip (arch/x86/include/asm/paravirt.h:819 kernel/softirq.c:171)
[  279.110186] ? dn_connect (net/decnet/af_decnet.c:979)
[  279.110186] dn_connect (net/decnet/af_decnet.c:980)
[  279.110186] SYSC_connect (net/socket.c:1701)
[  279.110186] ? trace_hardirqs_on (kernel/locking/lockdep.c:2607)
[  279.110186] ? syscall_trace_enter (include/linux/context_tracking.h:27 arch/x86/kernel/ptrace.c:1461)
[  279.110186] SyS_connect (net/socket.c:1683)
[  279.110186] tracesys (arch/x86/kernel/entry_64.S:749)
[  279.110186] Code: fc 85 c0 75 26 48 c7 c2 68 bf 69 a9 be 9d 03 00 00 48 c7 c7 b7 61 c7 a9 c6 05 42 4c cc 02 01 e8 1f cb ef fc 0f 1f 80 00 00 00 00 <48> 8b 1b e8 60 84 f1 fc 85 c0 74 5c 80 3d 24 4c cc 02 00 75 53
[  279.110186] RIP dnet_select_source.isra.25 (net/decnet/dn_route.c:926)
[  279.110186]  RSP <ffff8801b6e8bc88>


Thanks,
Sasha

[PATCH] decnet: fix possible NULL deref in dnet_select_source()

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

dnet_select_source() should make sure dn_ptr is not NULL.

While looking at this decnet code, I believe I found a device
reference leak, lets fix it as well.
 
Reported-by: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
It seems this bug is very old, no recent change is involved.

 net/decnet/dn_route.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/decnet/dn_route.c b/net/decnet/dn_route.c
index ce0cbbfe0f43..4d1608dfb0bd 100644
--- a/net/decnet/dn_route.c
+++ b/net/decnet/dn_route.c
@@ -923,6 +923,8 @@ static __le16 dnet_select_source(const struct net_device *dev, __le16 daddr, int
 
 	rcu_read_lock();
 	dn_db = rcu_dereference(dev->dn_ptr);
+	if (!dn_db)
+		goto out;
 	for (ifa = rcu_dereference(dn_db->ifa_list);
 	     ifa != NULL;
 	     ifa = rcu_dereference(ifa->ifa_next)) {
@@ -938,6 +940,7 @@ static __le16 dnet_select_source(const struct net_device *dev, __le16 daddr, int
 		if (best_match == 0)
 			saddr = ifa->ifa_local;
 	}
+out:
 	rcu_read_unlock();
 
 	return saddr;
@@ -1034,7 +1037,6 @@ source_ok:
 		if (dev_out)
 			dev_put(dev_out);
 		dev_out = init_net.loopback_dev;
-		dev_hold(dev_out);
 		if (!fld.daddr) {
 			fld.daddr =
 			fld.saddr = dnet_select_source(dev_out, 0,
@@ -1042,6 +1044,7 @@ source_ok:
 			if (!fld.daddr)
 				goto out;
 		}
+		dev_hold(dev_out);
 		fld.flowidn_oif = LOOPBACK_IFINDEX;
 		res.type = RTN_LOCAL;
 		goto make_route;

Re: [PATCH] decnet: fix possible NULL deref in dnet_select_source()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sun, 06 Apr 2014 14:59:14 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> dnet_select_source() should make sure dn_ptr is not NULL.
> 
> While looking at this decnet code, I believe I found a device
> reference leak, lets fix it as well.
>  
> Reported-by: Sasha Levin <sasha.levin@oracle.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> It seems this bug is very old, no recent change is involved.

The callers work hard to ensure this.

Analyzing all call sites:

1) __dn_fib_res_prefsrc() uses the FIB entry device pointer, we should not
   be adding FIB entries pointing to devices which do not have their
   decnet private initialized yet.

2) dn_route_output_slow()

   The paths leading to the dnet_select_address() call(s) check if
   dev_out->dn_ptr is not NULL, except when using loopback.

In some other paths the device comes from neigh->dev, from which the
'neigh' was looked up in dn_neigh_table.  There should not be neighbour
entries in this table pointing to devices which do not have their
decnet private setup yet.

And in the loopback case, it is the decnet stack's responsibility to
make sure ->dn_ptr is setup properly, else it should fail the module
load and stack initialization.

I think there is some core fundamental issue here, and just adding
a NULL check to dnet_select_source() is just papering around the issue.

Please look closer at the stack trace, this code, and my analysis
above to figure out what's really going on so we can fix this properly.

Thanks.

Re: [PATCH] decnet: fix possible NULL deref in dnet_select_source()

[Eric Dumazet]

On Mon, 2014-04-07 at 15:18 -0400, David Miller wrote:

> And in the loopback case, it is the decnet stack's responsibility to
> make sure ->dn_ptr is setup properly, else it should fail the module
> load and stack initialization.
> can fix this properly.

This was based on Sasha report and my limited time.
It seems you have more time than me to spend on decnet !

Re: [PATCH] decnet: fix possible NULL deref in dnet_select_source()

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 07 Apr 2014 21:51:44 -0700

> On Mon, 2014-04-07 at 15:18 -0400, David Miller wrote:
> 
>> And in the loopback case, it is the decnet stack's responsibility to
>> make sure ->dn_ptr is setup properly, else it should fail the module
>> load and stack initialization.
>> can fix this properly.
> 
> This was based on Sasha report and my limited time.
> It seems you have more time than me to spend on decnet !

Understood, I'll take a deeper look into this.

Re: [PATCH] decnet: fix possible NULL deref in dnet_select_source()

[Vegard Nossum]

On 7 April 2014 at 21:18, David Miller <davem@davemloft.net> wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Sun, 06 Apr 2014 14:59:14 -0700
>
>> From: Eric Dumazet <edumazet@google.com>
>>
>> dnet_select_source() should make sure dn_ptr is not NULL.
>>
>> While looking at this decnet code, I believe I found a device
>> reference leak, lets fix it as well.
>>
>> Reported-by: Sasha Levin <sasha.levin@oracle.com>
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> ---
>> It seems this bug is very old, no recent change is involved.
>
> The callers work hard to ensure this.
>
> Analyzing all call sites:
>
> 1) __dn_fib_res_prefsrc() uses the FIB entry device pointer, we should not
>    be adding FIB entries pointing to devices which do not have their
>    decnet private initialized yet.
>
> 2) dn_route_output_slow()
>
>    The paths leading to the dnet_select_address() call(s) check if
>    dev_out->dn_ptr is not NULL, except when using loopback.
>
> In some other paths the device comes from neigh->dev, from which the
> 'neigh' was looked up in dn_neigh_table.  There should not be neighbour
> entries in this table pointing to devices which do not have their
> decnet private setup yet.
>
> And in the loopback case, it is the decnet stack's responsibility to
> make sure ->dn_ptr is setup properly, else it should fail the module
> load and stack initialization.
>
> I think there is some core fundamental issue here, and just adding
> a NULL check to dnet_select_source() is just papering around the issue.
>
> Please look closer at the stack trace, this code, and my analysis
> above to figure out what's really going on so we can fix this properly.
>

Hi,

(Reviving old thread: https://lkml.org/lkml/2014/4/6/101)

I've just run into the same bug and I can confirm it's still present
on a stock Ubuntu kernel and can be reliably triggered by a non-root
user given that the loopback device is in a "down" state.

So as far as I understand:

dev_out->dn_ptr is assigned a non-NULL value in dn_dev_up() ->
dn_dev_create() when the loopback device is brought up (and set to
NULL when it is brought down).

dn_route_output_slow() doesn't check for a NULL value in the "No
destination? Assume its local" (!fld.daddr) case -- it also doesn't
check in any other way if the device is up or down.

Another bit in dn_route_output_slow() uses dn_dev_get_default() in
another "Not there? Perhaps its a local address" case, which _does_
check ->dn_ptr (but it uses decnet_default_device, not
init_net.loopback_dev).

There are other users of init_net.loopback_dev which don't seem to
check its ->dn_ptr.

I'm a bit uncertain about the other callsites that check ->dn_ptr for
a NULL value -- unless they take RTNL, how are they safe against a
race with somebody else bringing the device down (see
dn_dev_down()/dn_dev_delete()) and freeing ->dn_ptr after they get
ahold of it?

I think we could add NULL checks to dn_route_output_slow(). In any
case we shouldn't allow the device to be used if it's down, right?

I also understood from Sasha that decnet is generally in a bit of a
sorry state -- should we just add 'depends on BROKEN' in the Kconfig
to prevent more problems down the line?


Vegard