netdev - Re: [PATCH net] net: fix use-after-free in __netif_receive_skb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <62ad16f6-c33a-407e-2f55-9be382b7ec52@solarflare.com>
Date:   Wed, 10 Jul 2019 16:07:43 +0100
From:   Edward Cree <ecree@...arflare.com>
To:     Sabrina Dubroca <sd@...asysnail.net>, <netdev@...r.kernel.org>
CC:     Andreas Steinmetz <ast@...dv.de>
Subject: Re: [PATCH net] net: fix use-after-free in __netif_receive_skb_core

On 10/07/2019 14:52, Sabrina Dubroca wrote:
> When __netif_receive_skb_core handles a shared skb, it can be
> reallocated in a few different places:
>  - the device's rx_handler
>  - vlan_do_receive
>  - skb_vlan_untag
>
> To deal with that, rx_handlers and vlan_do_receive get passed a
> reference to the skb, and skb_vlan_untag just returns the new
> skb. This was not a problem until commit 88eb1944e18c ("net: core:
> propagate SKB lists through packet_type lookup"), which moved the
> final handling of the skb via pt_prev out of
> __netif_receive_skb_core. After this commit, when the skb is
> reallocated by __netif_receive_skb_core, KASAN reports a
> use-after-free on the old skb:
>
> BUG: KASAN: use-after-free in __netif_receive_skb_one_core+0x15c/0x180
> Call Trace:
>  <IRQ>
>  __netif_receive_skb_one_core+0x15c/0x180
>  process_backlog+0x1b5/0x630
>  ? net_rx_action+0x247/0xd00
>  net_rx_action+0x3fa/0xd00
>  ? napi_complete_done+0x360/0x360
>  __do_softirq+0x257/0xa0b
>  do_softirq_own_stack+0x2a/0x40
>  </IRQ>
>  ? __dev_queue_xmit+0x12ba/0x3120
>  do_softirq+0x5d/0x60
>  [...]
>
> Allocated by task 505:
>  __kasan_kmalloc.constprop.0+0xd6/0x140
>  kmem_cache_alloc+0xd4/0x2e0
>  skb_clone+0x106/0x300
>  deliver_clone+0x3f/0xa0
>  maybe_deliver+0x1c0/0x2b0
>  br_flood+0xd4/0x320
>  br_dev_xmit+0xbc0/0x1080
>  dev_hard_start_xmit+0x139/0x750
>  __dev_queue_xmit+0x24eb/0x3120
>  packet_sendmsg+0x1bfa/0x50e0
>  [...]
>
> Freed by task 505:
>  __kasan_slab_free+0x138/0x1e0
>  kmem_cache_free+0xa2/0x2e0
>  macsec_handle_frame+0xa24/0x2e60
>  __netif_receive_skb_core+0xe2a/0x2c90
>  __netif_receive_skb_one_core+0x96/0x180
>  process_backlog+0x1b5/0x630
>  net_rx_action+0x3fa/0xd00
>  __do_softirq+0x257/0xa0b
>
> The solution is to pass a reference to the skb to
> __netif_receive_skb_core, as we already do with the rx_handlers, so
> that its callers use the new skb.
>
> Fixes: 88eb1944e18c ("net: core: propagate SKB lists through packet_type lookup")
> Reported-by: Andreas Steinmetz <ast@...dv.de>
> Signed-off-by: Sabrina Dubroca <sd@...asysnail.net>
> ---
>  net/core/dev.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d6edd218babd..0bbf6d2a9c32 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4809,11 +4809,12 @@ static inline int nf_ingress(struct sk_buff *skb, struct packet_type **pt_prev,
>  	return 0;
>  }
>  
> -static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc,
> +static int __netif_receive_skb_core(struct sk_buff **pskb, bool pfmemalloc,
>  				    struct packet_type **ppt_prev)
>  {
>  	struct packet_type *ptype, *pt_prev;
>  	rx_handler_func_t *rx_handler;
> +	struct sk_buff *skb = *pskb;
Would it not be simpler just to change all users of skb to *pskb?
Then you avoid having to keep doing "*pskb = skb;" whenever skb changes
 (with concomitant risk of bugs if one gets missed).

-Ed