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: <77B7C763-CB82-43D7-8379-0A758BA216FA@gmail.com>
Date:   Fri, 12 Jul 2019 10:06:13 -0700
From:   "Jonathan Lemon" <jonathan.lemon@...il.com>
To:     "Edward Cree" <ecree@...arflare.com>
Cc:     "Sabrina Dubroca" <sd@...asysnail.net>, netdev@...r.kernel.org,
        "Andreas Steinmetz" <ast@...dv.de>
Subject: Re: [PATCH net] net: fix use-after-free in __netif_receive_skb_core



On 12 Jul 2019, at 8:29, Edward Cree wrote:

> 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?

That was actually my first thought as well - this seems to fit well with 
the
other APIS which can return a different skb.
-- 
Jonathan

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ