All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net] ipv4: route: fix inet_rtm_getroute induced crash
@ 2017-08-13 22:52 Florian Westphal
  2017-08-14  4:34 ` David Ahern
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Florian Westphal @ 2017-08-13 22:52 UTC (permalink / raw)
  To: netdev; +Cc: Florian Westphal, Roopa Prabhu, David Ahern

"ip route get $daddr iif eth0 from $saddr" causes:
 BUG: KASAN: use-after-free in ip_route_input_rcu+0x1535/0x1b50
 Call Trace:
  ip_route_input_rcu+0x1535/0x1b50
  ip_route_input_noref+0xf9/0x190
  tcp_v4_early_demux+0x1a4/0x2b0
  ip_rcv+0xbcb/0xc05
  __netif_receive_skb+0x9c/0xd0
  netif_receive_skb_internal+0x5a8/0x890

Problem is that inet_rtm_getroute calls either ip_route_input_rcu (if an
iif was provided) or ip_route_output_key_hash_rcu.

But ip_route_input_rcu, unlike ip_route_output_key_hash_rcu, already
associates the dst_entry with the skb.  This clears the SKB_DST_NOREF
bit (i.e. skb_dst_drop will release/free the entry while it should not).

Thus only set the dst if we called ip_route_output_key_hash_rcu().

I tested this patch by running:
 while true;do ip r get 10.0.1.2;done > /dev/null &
 while true;do ip r get 10.0.1.2 iif eth0  from 10.0.1.1;done > /dev/null &
... and saw no crash or memory leak.

Cc: Roopa Prabhu <roopa@cumulusnetworks.com>
Cc: David Ahern <dsahern@gmail.com>
Fixes: ba52d61e0ff ("ipv4: route: restore skb_dst_set in inet_rtm_getroute")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/ipv4/route.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 0383e66f59bc..7effa62beed3 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -2750,12 +2750,13 @@ static int inet_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
 		err = 0;
 		if (IS_ERR(rt))
 			err = PTR_ERR(rt);
+		else
+			skb_dst_set(skb, &rt->dst);
 	}
 
 	if (err)
 		goto errout_free;
 
-	skb_dst_set(skb, &rt->dst);
 	if (rtm->rtm_flags & RTM_F_NOTIFY)
 		rt->rt_flags |= RTCF_NOTIFY;
 
-- 
2.13.0

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

* Re: [PATCH net] ipv4: route: fix inet_rtm_getroute induced crash
  2017-08-13 22:52 [PATCH net] ipv4: route: fix inet_rtm_getroute induced crash Florian Westphal
@ 2017-08-14  4:34 ` David Ahern
  2017-08-14  5:20   ` Florian Westphal
  2017-08-14 15:42 ` Eric Dumazet
  2017-08-14 18:09 ` David Miller
  2 siblings, 1 reply; 5+ messages in thread
From: David Ahern @ 2017-08-14  4:34 UTC (permalink / raw)
  To: Florian Westphal, netdev; +Cc: Roopa Prabhu

On 8/13/17 4:52 PM, Florian Westphal wrote:
> "ip route get $daddr iif eth0 from $saddr" causes:
>  BUG: KASAN: use-after-free in ip_route_input_rcu+0x1535/0x1b50
>  Call Trace:
>   ip_route_input_rcu+0x1535/0x1b50
>   ip_route_input_noref+0xf9/0x190
>   tcp_v4_early_demux+0x1a4/0x2b0
>   ip_rcv+0xbcb/0xc05
>   __netif_receive_skb+0x9c/0xd0
>   netif_receive_skb_internal+0x5a8/0x890
> 
> Problem is that inet_rtm_getroute calls either ip_route_input_rcu (if an
> iif was provided) or ip_route_output_key_hash_rcu.
> 
> But ip_route_input_rcu, unlike ip_route_output_key_hash_rcu, already
> associates the dst_entry with the skb.  This clears the SKB_DST_NOREF
> bit (i.e. skb_dst_drop will release/free the entry while it should not).
> 
> Thus only set the dst if we called ip_route_output_key_hash_rcu().
> 
> I tested this patch by running:
>  while true;do ip r get 10.0.1.2;done > /dev/null &
>  while true;do ip r get 10.0.1.2 iif eth0  from 10.0.1.1;done > /dev/null &
> ... and saw no crash or memory leak.
> 
> Cc: Roopa Prabhu <roopa@cumulusnetworks.com>
> Cc: David Ahern <dsahern@gmail.com>
> Fixes: ba52d61e0ff ("ipv4: route: restore skb_dst_set in inet_rtm_getroute")
> Signed-off-by: Florian Westphal <fw@strlen.de>

Have looked at the change in detail, but are you sure that is the
correct Fixes?

Running these:
  while true;do ip r get 10.1.1.3;done > /dev/null &
  while true;do ip r get 10.1.1.3 iif eth0  from 192.16.1.1;done >
/dev/null &

at various commits:
  ffe95ecf3a2 - KASAN backtraces
  374d801522f - works fine
  ba52d61e0ff - negative refcnt messages
  a5e2ee5da47 - works fine

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

* Re: [PATCH net] ipv4: route: fix inet_rtm_getroute induced crash
  2017-08-14  4:34 ` David Ahern
@ 2017-08-14  5:20   ` Florian Westphal
  0 siblings, 0 replies; 5+ messages in thread
From: Florian Westphal @ 2017-08-14  5:20 UTC (permalink / raw)
  To: David Ahern; +Cc: Florian Westphal, netdev, Roopa Prabhu

David Ahern <dsahern@gmail.com> wrote:
> On 8/13/17 4:52 PM, Florian Westphal wrote:
> > "ip route get $daddr iif eth0 from $saddr" causes:
> >  BUG: KASAN: use-after-free in ip_route_input_rcu+0x1535/0x1b50
> >  Call Trace:
> >   ip_route_input_rcu+0x1535/0x1b50
> >   ip_route_input_noref+0xf9/0x190
> >   tcp_v4_early_demux+0x1a4/0x2b0
> >   ip_rcv+0xbcb/0xc05
> >   __netif_receive_skb+0x9c/0xd0
> >   netif_receive_skb_internal+0x5a8/0x890
> > 
> > Problem is that inet_rtm_getroute calls either ip_route_input_rcu (if an
> > iif was provided) or ip_route_output_key_hash_rcu.
> > 
> > But ip_route_input_rcu, unlike ip_route_output_key_hash_rcu, already
> > associates the dst_entry with the skb.  This clears the SKB_DST_NOREF
> > bit (i.e. skb_dst_drop will release/free the entry while it should not).
> > 
> > Thus only set the dst if we called ip_route_output_key_hash_rcu().
> > 
> > I tested this patch by running:
> >  while true;do ip r get 10.0.1.2;done > /dev/null &
> >  while true;do ip r get 10.0.1.2 iif eth0  from 10.0.1.1;done > /dev/null &
> > ... and saw no crash or memory leak.
> > 
> > Cc: Roopa Prabhu <roopa@cumulusnetworks.com>
> > Cc: David Ahern <dsahern@gmail.com>
> > Fixes: ba52d61e0ff ("ipv4: route: restore skb_dst_set in inet_rtm_getroute")
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> 
> Have looked at the change in detail, but are you sure that is the
> correct Fixes?

I'm reasonably sure, yes:

if (iif) {
  ip_route_input_rcu // 1 might get NOREF dst
} else {
  ip_route_output_key_hash_rcu // 2 always takes dst ref
}
skb_dst_set /* 3 loses NOREF in case of 1) */

> Running these:
>   while true;do ip r get 10.1.1.3;done > /dev/null &
>   while true;do ip r get 10.1.1.3 iif eth0  from 192.16.1.1;done >
> /dev/null &
> 
> at various commits:
>   ffe95ecf3a2 - KASAN backtraces

Right, this is broken state (has both ba52d61e0ff and 3765d35ed8b9)

>   374d801522f - works fine

This is fine, it lacks 3765d35ed8b9:
both branches take a reference on dst so '3' above has no side effect.

>   ba52d61e0ff - negative refcnt messages

AFAICS this is before dst gc removal, I guess (but did not
check) that KASAN vs. refcount just comes from this.

>   a5e2ee5da47 - works fine

Should cause a memory leak when iif is not given (ref on dst is
taken but not released in case of 2), ba52d61e0ff cured this but
adds the problem described here).

Does that make it clearer?

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

* Re: [PATCH net] ipv4: route: fix inet_rtm_getroute induced crash
  2017-08-13 22:52 [PATCH net] ipv4: route: fix inet_rtm_getroute induced crash Florian Westphal
  2017-08-14  4:34 ` David Ahern
@ 2017-08-14 15:42 ` Eric Dumazet
  2017-08-14 18:09 ` David Miller
  2 siblings, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2017-08-14 15:42 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netdev, Roopa Prabhu, David Ahern

On Mon, 2017-08-14 at 00:52 +0200, Florian Westphal wrote:
> "ip route get $daddr iif eth0 from $saddr" causes:
>  BUG: KASAN: use-after-free in ip_route_input_rcu+0x1535/0x1b50
>  Call Trace:
>   ip_route_input_rcu+0x1535/0x1b50
>   ip_route_input_noref+0xf9/0x190
>   tcp_v4_early_demux+0x1a4/0x2b0
>   ip_rcv+0xbcb/0xc05
>   __netif_receive_skb+0x9c/0xd0
>   netif_receive_skb_internal+0x5a8/0x890
> 
> Problem is that inet_rtm_getroute calls either ip_route_input_rcu (if an
> iif was provided) or ip_route_output_key_hash_rcu.
> 
> But ip_route_input_rcu, unlike ip_route_output_key_hash_rcu, already
> associates the dst_entry with the skb.  This clears the SKB_DST_NOREF
> bit (i.e. skb_dst_drop will release/free the entry while it should not).
> 
> Thus only set the dst if we called ip_route_output_key_hash_rcu().
> 
> I tested this patch by running:
>  while true;do ip r get 10.0.1.2;done > /dev/null &
>  while true;do ip r get 10.0.1.2 iif eth0  from 10.0.1.1;done > /dev/null &
> ... and saw no crash or memory leak.
> 
> Cc: Roopa Prabhu <roopa@cumulusnetworks.com>
> Cc: David Ahern <dsahern@gmail.com>
> Fixes: ba52d61e0ff ("ipv4: route: restore skb_dst_set in inet_rtm_getroute")
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---

Ouch

Acked-by: Eric Dumazet <edumazet@google.com>

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

* Re: [PATCH net] ipv4: route: fix inet_rtm_getroute induced crash
  2017-08-13 22:52 [PATCH net] ipv4: route: fix inet_rtm_getroute induced crash Florian Westphal
  2017-08-14  4:34 ` David Ahern
  2017-08-14 15:42 ` Eric Dumazet
@ 2017-08-14 18:09 ` David Miller
  2 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2017-08-14 18:09 UTC (permalink / raw)
  To: fw; +Cc: netdev, roopa, dsahern

From: Florian Westphal <fw@strlen.de>
Date: Mon, 14 Aug 2017 00:52:58 +0200

> "ip route get $daddr iif eth0 from $saddr" causes:
>  BUG: KASAN: use-after-free in ip_route_input_rcu+0x1535/0x1b50
>  Call Trace:
>   ip_route_input_rcu+0x1535/0x1b50
>   ip_route_input_noref+0xf9/0x190
>   tcp_v4_early_demux+0x1a4/0x2b0
>   ip_rcv+0xbcb/0xc05
>   __netif_receive_skb+0x9c/0xd0
>   netif_receive_skb_internal+0x5a8/0x890
> 
> Problem is that inet_rtm_getroute calls either ip_route_input_rcu (if an
> iif was provided) or ip_route_output_key_hash_rcu.
> 
> But ip_route_input_rcu, unlike ip_route_output_key_hash_rcu, already
> associates the dst_entry with the skb.  This clears the SKB_DST_NOREF
> bit (i.e. skb_dst_drop will release/free the entry while it should not).
> 
> Thus only set the dst if we called ip_route_output_key_hash_rcu().
> 
> I tested this patch by running:
>  while true;do ip r get 10.0.1.2;done > /dev/null &
>  while true;do ip r get 10.0.1.2 iif eth0  from 10.0.1.1;done > /dev/null &
> ... and saw no crash or memory leak.
> 
> Cc: Roopa Prabhu <roopa@cumulusnetworks.com>
> Cc: David Ahern <dsahern@gmail.com>
> Fixes: ba52d61e0ff ("ipv4: route: restore skb_dst_set in inet_rtm_getroute")
> Signed-off-by: Florian Westphal <fw@strlen.de>

Applied, thanks Florian.

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

end of thread, other threads:[~2017-08-14 18:09 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-13 22:52 [PATCH net] ipv4: route: fix inet_rtm_getroute induced crash Florian Westphal
2017-08-14  4:34 ` David Ahern
2017-08-14  5:20   ` Florian Westphal
2017-08-14 15:42 ` Eric Dumazet
2017-08-14 18:09 ` David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.