netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] ipv4: Fix NULL pointer dereference in ipv4_neigh_lookup()
@ 2019-07-04 16:26 Ido Schimmel
  2019-07-04 19:24 ` David Miller
  2019-07-05 23:19 ` David Miller
  0 siblings, 2 replies; 5+ messages in thread
From: Ido Schimmel @ 2019-07-04 16:26 UTC (permalink / raw)
  To: netdev; +Cc: davem, dsahern, jiri, shalomt, mlxsw, Ido Schimmel

From: Ido Schimmel <idosch@mellanox.com>

Both ip_neigh_gw4() and ip_neigh_gw6() can return either a valid pointer
or an error pointer, but the code currently checks that the pointer is
not NULL.

Fix this by checking that the pointer is not an error pointer, as this
can result in a NULL pointer dereference [1]. Specifically, I believe
that what happened is that ip_neigh_gw4() returned '-EINVAL'
(0xffffffffffffffea) to which the offset of 'refcnt' (0x70) was added,
which resulted in the address 0x000000000000005a.

[1]
 BUG: KASAN: null-ptr-deref in refcount_inc_not_zero_checked+0x6e/0x180
 Read of size 4 at addr 000000000000005a by task swapper/2/0

 CPU: 2 PID: 0 Comm: swapper/2 Not tainted 5.2.0-rc6-custom-reg-179657-gaa32d89 #396
 Hardware name: Mellanox Technologies Ltd. MSN2010/SA002610, BIOS 5.6.5 08/24/2017
 Call Trace:
 <IRQ>
 dump_stack+0x73/0xbb
 __kasan_report+0x188/0x1ea
 kasan_report+0xe/0x20
 refcount_inc_not_zero_checked+0x6e/0x180
 ipv4_neigh_lookup+0x365/0x12c0
 __neigh_update+0x1467/0x22f0
 arp_process.constprop.6+0x82e/0x1f00
 __netif_receive_skb_one_core+0xee/0x170
 process_backlog+0xe3/0x640
 net_rx_action+0x755/0xd90
 __do_softirq+0x29b/0xae7
 irq_exit+0x177/0x1c0
 smp_apic_timer_interrupt+0x164/0x5e0
 apic_timer_interrupt+0xf/0x20
 </IRQ>

Fixes: 5c9f7c1dfc2e ("ipv4: Add helpers for neigh lookup for nexthop")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Reported-by: Shalom Toledo <shalomt@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
 net/ipv4/route.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 8ea0735a6754..b2b35b38724d 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -447,7 +447,7 @@ static struct neighbour *ipv4_neigh_lookup(const struct dst_entry *dst,
 		n = ip_neigh_gw4(dev, pkey);
 	}
 
-	if (n && !refcount_inc_not_zero(&n->refcnt))
+	if (!IS_ERR(n) && !refcount_inc_not_zero(&n->refcnt))
 		n = NULL;
 
 	rcu_read_unlock_bh();
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH net] ipv4: Fix NULL pointer dereference in ipv4_neigh_lookup()
  2019-07-04 16:26 [PATCH net] ipv4: Fix NULL pointer dereference in ipv4_neigh_lookup() Ido Schimmel
@ 2019-07-04 19:24 ` David Miller
  2019-07-04 19:55   ` Ido Schimmel
  2019-07-05 15:37   ` David Ahern
  2019-07-05 23:19 ` David Miller
  1 sibling, 2 replies; 5+ messages in thread
From: David Miller @ 2019-07-04 19:24 UTC (permalink / raw)
  To: idosch; +Cc: netdev, dsahern, jiri, shalomt, mlxsw, idosch

From: Ido Schimmel <idosch@idosch.org>
Date: Thu,  4 Jul 2019 19:26:38 +0300

> Both ip_neigh_gw4() and ip_neigh_gw6() can return either a valid pointer
> or an error pointer, but the code currently checks that the pointer is
> not NULL.
 ...
> @@ -447,7 +447,7 @@ static struct neighbour *ipv4_neigh_lookup(const struct dst_entry *dst,
>  		n = ip_neigh_gw4(dev, pkey);
>  	}
>  
> -	if (n && !refcount_inc_not_zero(&n->refcnt))
> +	if (!IS_ERR(n) && !refcount_inc_not_zero(&n->refcnt))
>  		n = NULL;
>  
>  	rcu_read_unlock_bh();

Don't the callers expect only non-error pointers?

All of this stuff is so confusing and fragile...

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] ipv4: Fix NULL pointer dereference in ipv4_neigh_lookup()
  2019-07-04 19:24 ` David Miller
@ 2019-07-04 19:55   ` Ido Schimmel
  2019-07-05 15:37   ` David Ahern
  1 sibling, 0 replies; 5+ messages in thread
From: Ido Schimmel @ 2019-07-04 19:55 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, dsahern, jiri, shalomt, mlxsw, idosch

On Thu, Jul 04, 2019 at 12:24:49PM -0700, David Miller wrote:
> From: Ido Schimmel <idosch@idosch.org>
> Date: Thu,  4 Jul 2019 19:26:38 +0300
> 
> > Both ip_neigh_gw4() and ip_neigh_gw6() can return either a valid pointer
> > or an error pointer, but the code currently checks that the pointer is
> > not NULL.
>  ...
> > @@ -447,7 +447,7 @@ static struct neighbour *ipv4_neigh_lookup(const struct dst_entry *dst,
> >  		n = ip_neigh_gw4(dev, pkey);
> >  	}
> >  
> > -	if (n && !refcount_inc_not_zero(&n->refcnt))
> > +	if (!IS_ERR(n) && !refcount_inc_not_zero(&n->refcnt))
> >  		n = NULL;
> >  
> >  	rcu_read_unlock_bh();
> 
> Don't the callers expect only non-error pointers?

It is actually OK to return an error pointer here. In fact, before the
commit I cited the function returned the return value of neigh_create().

If you think it's clearer, we can do this instead:

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 8ea0735a6754..40697fcd2889 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -447,6 +447,9 @@ static struct neighbour *ipv4_neigh_lookup(const struct dst_entry *dst,
                n = ip_neigh_gw4(dev, pkey);
        }
 
+       if (IS_ERR(n))
+               n = NULL;
+
        if (n && !refcount_inc_not_zero(&n->refcnt))
                n = NULL;

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH net] ipv4: Fix NULL pointer dereference in ipv4_neigh_lookup()
  2019-07-04 19:24 ` David Miller
  2019-07-04 19:55   ` Ido Schimmel
@ 2019-07-05 15:37   ` David Ahern
  1 sibling, 0 replies; 5+ messages in thread
From: David Ahern @ 2019-07-05 15:37 UTC (permalink / raw)
  To: David Miller, idosch; +Cc: netdev, jiri, shalomt, mlxsw, idosch

On 7/4/19 1:24 PM, David Miller wrote:
> From: Ido Schimmel <idosch@idosch.org>
> Date: Thu,  4 Jul 2019 19:26:38 +0300
> 
>> Both ip_neigh_gw4() and ip_neigh_gw6() can return either a valid pointer
>> or an error pointer, but the code currently checks that the pointer is
>> not NULL.
>  ...
>> @@ -447,7 +447,7 @@ static struct neighbour *ipv4_neigh_lookup(const struct dst_entry *dst,
>>  		n = ip_neigh_gw4(dev, pkey);
>>  	}
>>  
>> -	if (n && !refcount_inc_not_zero(&n->refcnt))
>> +	if (!IS_ERR(n) && !refcount_inc_not_zero(&n->refcnt))
>>  		n = NULL;
>>  
>>  	rcu_read_unlock_bh();
> 
> Don't the callers expect only non-error pointers?
> 
> All of this stuff is so confusing and fragile...
> 

The intention was to fold the lookup and neigh_create calls into a
single helper.

The lookup can return NULL if an entry does not exist; the create can
return an ERR_PTR (variety of reasons in ___neigh_create). So the end
result is that the new helper (lookup + create) can return a valid neigh
entry or an ERR_PTR.

When I converted ipv4_neigh_lookup and folded in the refcount bump, I
missed updating the above check to account for ERR_PTR.

Ido's patch looks correct to me. Thanks, Ido.

Reviewed-by: David Ahern <dsahern@gmail.com>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net] ipv4: Fix NULL pointer dereference in ipv4_neigh_lookup()
  2019-07-04 16:26 [PATCH net] ipv4: Fix NULL pointer dereference in ipv4_neigh_lookup() Ido Schimmel
  2019-07-04 19:24 ` David Miller
@ 2019-07-05 23:19 ` David Miller
  1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2019-07-05 23:19 UTC (permalink / raw)
  To: idosch; +Cc: netdev, dsahern, jiri, shalomt, mlxsw, idosch

From: Ido Schimmel <idosch@idosch.org>
Date: Thu,  4 Jul 2019 19:26:38 +0300

> From: Ido Schimmel <idosch@mellanox.com>
> 
> Both ip_neigh_gw4() and ip_neigh_gw6() can return either a valid pointer
> or an error pointer, but the code currently checks that the pointer is
> not NULL.
> 
> Fix this by checking that the pointer is not an error pointer, as this
> can result in a NULL pointer dereference [1]. Specifically, I believe
> that what happened is that ip_neigh_gw4() returned '-EINVAL'
> (0xffffffffffffffea) to which the offset of 'refcnt' (0x70) was added,
> which resulted in the address 0x000000000000005a.
> 
> [1]
 ...
> Fixes: 5c9f7c1dfc2e ("ipv4: Add helpers for neigh lookup for nexthop")
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Reported-by: Shalom Toledo <shalomt@mellanox.com>
> Reviewed-by: Jiri Pirko <jiri@mellanox.com>

Applied, thanks.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-07-05 23:19 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-04 16:26 [PATCH net] ipv4: Fix NULL pointer dereference in ipv4_neigh_lookup() Ido Schimmel
2019-07-04 19:24 ` David Miller
2019-07-04 19:55   ` Ido Schimmel
2019-07-05 15:37   ` David Ahern
2019-07-05 23:19 ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).