Re: [PATCH bpf 1/2] bpf: Update transport_header when encapsulating UDP tunnel in lwt

[Leon Hwang <leon.hwang@linux.dev>]

On 2026/5/25 23:00, sashiko-bot@kernel.org wrote:
[...]
>> diff --git a/net/core/lwt_bpf.c b/net/core/lwt_bpf.c
>> index f71ef82a5f3d3..cbb2803367c6d 100644
>> --- a/net/core/lwt_bpf.c
>> +++ b/net/core/lwt_bpf.c
>> @@ -637,6 +637,10 @@ int bpf_lwt_push_ip_encap(struct sk_buff *skb, void *hdr, u32 len, bool ingress)
>>  	if (ingress)
>>  		skb_postpush_rcsum(skb, iph, len);
>>  	skb_reset_network_header(skb);
>> +	if (ipv4 && iph->protocol == IPPROTO_UDP /* UDP tunnel */)
>> +		skb_set_transport_header(skb, skb_network_offset(skb) + iph->ihl * 4);
> 
> [Severity: High]
> Can a concurrent modification to the BPF-provided header cause an
> out-of-bounds transport header offset?
> 
> The hdr pointer passed to bpf_lwt_push_ip_encap() can point to concurrently
> mutable memory such as a BPF map value. Reading iph->protocol and iph->ihl
> directly from hdr here, after length validation has already occurred,
> appears to introduce a Time-of-Check to Time-of-Use race condition.
> 
> If a concurrent modification changes iph->ihl, the calculated transport
> header could point well past the end of the pushed IP header. For small
> SKBs, this could cause skb_transport_header to point out-of-bounds into
> skb_shared_info.
> 
> Would it be safer to perform these reads after copying the header into the
> SKB, using the copied, stable values instead?
> 

Agreed.

Will move skb_set_transport_header() after memcpy().

> [Severity: Critical]
> This is a pre-existing issue and was not introduced by the patch under
> review, but is there a missing validation for the minimum IPv4 header
> length before ip_fast_csum() is called?
> 
> Earlier in the function, the length validation checks:
> 
>     if (unlikely(len < iph->ihl * 4))
>         return -EINVAL;
> 
> If a BPF program provides an IP header with ihl = 0, the check len < 0
> evaluates to false because len is at least 20.
> 
> Later, ip_fast_csum() is called with ihl = 0. On architectures like MIPS,
> the ip_fast_csum() implementation reads 16 bytes and then uses a loop:
> 
>     do { ... word++ } while (word != stop);
> 
> where stop = iph + ihl. If ihl = 0, stop is physically behind word,
> causing the loop to endlessly increment through the address space until
> it page faults.
> 
> Does this code need a check to ensure iph->ihl >= 5?
> 

I'm not sure about this case.

But, can update "len < iph->ihl * 4" to
"iph->ihl < 5 || len < iph->ihl * 4".

> [Severity: Medium]
> Does this code verify that the packet length actually contains the full UDP
> header?
> 
> This updates the transport header for UDP tunnels based solely on the IP
> protocol, but it doesn't seem to validate that len actually encompasses
> the full struct udphdr.
> 
> If an IP header is pushed with IPPROTO_UDP but a truncated length (like
> len = 20), the transport header is set to the end of the IP header. A NIC
> driver might then attempt to parse the subsequent memory as a UDP header,
> which could lead to broken offloads or out-of-bounds reads.
> 

Agreed.

Will check len must include UDP header.

Thanks,
Leon

>> +	else if (!ipv4 && ((struct ipv6hdr *)hdr)->nexthdr == NEXTHDR_UDP /* UDP tunnel */)
>> +		skb_set_transport_header(skb, skb_network_offset(skb) + sizeof(struct ipv6hdr));
>>  	memcpy(skb_network_header(skb), hdr, len);
>>  	bpf_compute_data_pointers(skb);
>>  	skb_clear_hash(skb);
>