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: <8166b82f-8430-1441-32e7-540c1829754e@solarflare.com>
Date:   Fri, 12 Jul 2019 16:29:48 +0100
From:   Edward Cree <ecree@...arflare.com>
To:     Sabrina Dubroca <sd@...asysnail.net>
CC:     <netdev@...r.kernel.org>, Andreas Steinmetz <ast@...dv.de>
Subject: Re: [PATCH net] net: fix use-after-free in __netif_receive_skb_core

On 10/07/2019 23:47, Sabrina Dubroca wrote:
> 2019-07-10, 16:07:43 +0100, Edward Cree wrote:
>> On 10/07/2019 14:52, Sabrina Dubroca wrote:
>>> -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).
> Yes, that would be less risky. I wrote a version of the patch that did
> exactly that, but found it really too ugly (both the patch and the
> resulting code).
If you've still got that version (or can dig it out of your reflog), can
 you post it so we can see just how ugly it turns out?

>  We have more than 50 occurences of skb, including
> things like:
>
>     atomic_long_inc(&skb->dev->rx_dropped);
Ooh, yes, I can see why that ends up looking funny...

Here's a thought, how about switching round the return-vs-pass-by-pointer
 and writing:

static struct sk_buff *__netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc,
                                                struct packet_type **ppt_prev, int *ret)
?
(Although, you might want to rename 'ret' in that case.)

Does that make things any less ugly?
-Ed