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  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]
Date:   Mon, 18 May 2020 17:20:13 +0100
From:   Edward Cree <ecree@...arflare.com>
To:     Boris Sukholitko <boris.sukholitko@...adcom.com>,
        <netdev@...r.kernel.org>
Subject: Re: [PATCH net] __netif_receive_skb_core: pass skb by reference

On 18/05/2020 10:01, Boris Sukholitko wrote:
> __netif_receive_skb_core may change the skb pointer passed into it (e.g.
> in rx_handler). The original skb may be freed as a result of this
> operation.
>
> The callers of __netif_receive_skb_core may further process original skb
> by using pt_prev pointer returned by __netif_receive_skb_core thus
> leading to unpleasant effects.
>
> The solution is to pass skb by reference into __netif_receive_skb_core.
>
> Signed-off-by: Boris Sukholitko <boris.sukholitko@...adcom.com>
I think this patch is correct, but there's a couple of things I'd like
 to see before adding my Ack.
Firstly, please add a Fixes: tag; I expect the relevant commit will be
 88eb1944e18c ("net: core: propagate SKB lists through packet_type lookup")
 but I'm not 100% sure so do check that yourself.
Secondly:
> @@ -5174,6 +5177,7 @@ static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc,
>  	}
>  
>  out:
> +	*pskb = skb;
>  	return ret;
>  }
Could we have some sort of WARN_ONs (maybe under #ifdef DEBUG) to check
 that we never have a NULL skb with a non-NULL pt_prev?  Or at least a
 comment at the top of the function stating this part of its contract
 with callers?  I've gone through and convinced myself that it never
 happens currently, but that depends on some fairly subtle details.

-ed

Powered by blists - more mailing lists