Subject: Re: [PATCH] net: fix NULL pointer in skb_segment_list
To:     Yan Zhai <yan@cloudflare.com>, netdev@vger.kernel.org
Cc:     edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        davem@davemloft.net, asml.silence@gmail.com, imagedong@tencent.com,
        keescook@chromium.org, jbenc@redhat.com, richardbgobert@gmail.com,
        willemb@google.com, steffen.klassert@secunet.com,
        linux-kernel@vger.kernel.org
References: <Y9gt5EUizK1UImEP@debian>
From:   Daniel Borkmann <daniel@iogearbox.net>
Message-ID: <0e41673f-457d-1685-ea47-0166ca71ff97@iogearbox.net>
Date:   Mon, 30 Jan 2023 22:05:25 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.2
MIME-Version: 1.0
In-Reply-To: <Y9gt5EUizK1UImEP@debian>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 1/30/23 9:51 PM, Yan Zhai wrote:
> Commit 3a1296a38d0c ("net: Support GRO/GSO fraglist chaining.")
> introduced UDP listifyed GRO. The segmentation relies on frag_list being
> untouched when passing through the network stack. This assumption can be
> broken sometimes, where frag_list itself gets pulled into linear area,
> leaving frag_list being NULL. When this happens it can trigger
> following NULL pointer dereference, and panic the kernel. Reverse the
> test condition should fix it.
> 
> [19185.577801][    C1] BUG: kernel NULL pointer dereference, address:
> ...
> [19185.663775][    C1] RIP: 0010:skb_segment_list+0x1cc/0x390
> ...
> [19185.834644][    C1] Call Trace:
> [19185.841730][    C1]  <TASK>
> [19185.848563][    C1]  __udp_gso_segment+0x33e/0x510
> [19185.857370][    C1]  inet_gso_segment+0x15b/0x3e0
> [19185.866059][    C1]  skb_mac_gso_segment+0x97/0x110
> [19185.874939][    C1]  __skb_gso_segment+0xb2/0x160
> [19185.883646][    C1]  udp_queue_rcv_skb+0xc3/0x1d0
> [19185.892319][    C1]  udp_unicast_rcv_skb+0x75/0x90
> [19185.900979][    C1]  ip_protocol_deliver_rcu+0xd2/0x200
> [19185.910003][    C1]  ip_local_deliver_finish+0x44/0x60
> [19185.918757][    C1]  __netif_receive_skb_one_core+0x8b/0xa0
> [19185.927834][    C1]  process_backlog+0x88/0x130
> [19185.935840][    C1]  __napi_poll+0x27/0x150
> [19185.943447][    C1]  net_rx_action+0x27e/0x5f0
> [19185.951331][    C1]  ? mlx5_cq_tasklet_cb+0x70/0x160 [mlx5_core]
> [19185.960848][    C1]  __do_softirq+0xbc/0x25d
> [19185.968607][    C1]  irq_exit_rcu+0x83/0xb0
> [19185.976247][    C1]  common_interrupt+0x43/0xa0
> [19185.984235][    C1]  asm_common_interrupt+0x22/0x40
> ...
> [19186.094106][    C1]  </TASK>
> 
> Fixes: 3a1296a38d0c ("net: Support GRO/GSO fraglist chaining.")
> Suggested-by: Daniel Borkmann <daniel@iogearbox.net>
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Yan Zhai <yan@cloudflare.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

>   net/core/skbuff.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 4a0eb5593275..a31ff4d83ecc 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4100,7 +4100,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
>   
>   	skb_shinfo(skb)->frag_list = NULL;
>   
> -	do {
> +	while (list_skb) {
>   		nskb = list_skb;
>   		list_skb = list_skb->next;
>   
> @@ -4146,8 +4146,7 @@ struct sk_buff *skb_segment_list(struct sk_buff *skb,
>   		if (skb_needs_linearize(nskb, features) &&
>   		    __skb_linearize(nskb))
>   			goto err_linearize;
> -
> -	} while (list_skb);
> +	}
>   
>   	skb->truesize = skb->truesize - delta_truesize;
>   	skb->data_len = skb->data_len - delta_len;
>