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