Re: [PATCH net-next 2/9] net: skbuff: don't BUG() on bad arguments to pskb_expand_head()

[Willem de Bruijn]

Josef Bacik wrote:
> pskb_expand_head() BUG()s if it is handed a negative nhead or a shared
> skb.  Both are bugs in the caller, and both keep getting hit: commit
> 2cbb259ec4f8 ("bpf: Reject negative head_room in __bpf_skb_change_head")
> and commit 64e6a754d33d ("llc: do not use skb_get() before
> dev_queue_xmit()") each fixed a crash here that took down the whole box.
> 
> The function already returns an error that every caller has to handle,
> and at this point it hasn't touched the skb.  Warn once and return
> -EINVAL instead of crashing.  Anybody running with panic_on_warn, which
> includes syzbot, still stops right here.
> 
> Assisted-by: LLM
> Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> ---
>  net/core/skbuff.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 512ff9cfa269..5d856948cef9 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2303,9 +2303,11 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
>  	u8 *data;
>  	int i;
>  
> -	BUG_ON(nhead < 0);
> +	if (WARN_ON_ONCE(nhead < 0))
> +		return -EINVAL;

Another option besides WARN_ON_ONCE or even DEBUG_NET_WARN_ON_ONCE
when returning an error is a net_warn_ratelimited for such cases.

>  
> -	BUG_ON(skb_shared(skb));
> +	if (WARN_ON_ONCE(skb_shared(skb)))
> +		return -EINVAL;
>  
>  	skb_zcopy_downgrade_managed(skb);
>  
> 
> -- 
> 2.55.0
>