netdev - Re: [PATCH net] __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]

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