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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing IPv4 header length (`ihl`) minimum bounds check in `bpf_lwt_push_ip_encap` allows denial of service via `ip_fast_csum`.
- [High] Time-of-Check to Time-of-Use (TOCTOU) vulnerability in `bpf_lwt_push_ip_encap` due to re-reading from eBPF map memory.
--

commit 8004c2f9aa8d7cbb7b762ab79a4e3272c20af453
Author: Leon Hwang <leon.hwang@linux.dev>

bpf: Update transport_header when encapsulating UDP tunnel in lwt

This commit updates skb->transport_header in bpf_lwt_push_ip_encap() when
encapsulating a packet that uses UDP. This ensures that checksum offloading
works correctly when a driver reuses the transport header.

> diff --git a/net/core/lwt_bpf.c b/net/core/lwt_bpf.c
> index f71ef82a5f3d3..bf588f508b79e 100644
> --- a/net/core/lwt_bpf.c
> +++ b/net/core/lwt_bpf.c
> @@ -612,10 +613,16 @@ int bpf_lwt_push_ip_encap(struct sk_buff *skb, void *hdr, u32 len, bool ingress)
>  		ipv4 = true;
>  		if (unlikely(len < iph->ihl * 4))
>  			return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does bpf_lwt_push_ip_encap() need to verify
that iph->ihl is at least 5?

An eBPF program can pass an IPv4 header with ihl = 0 and len = 20. This
bypasses the length validation because 20 < 0 is false. Later in the function,
this unvalidated ihl is passed to ip_fast_csum(iph, iph->ihl) with a length
of 0. On architectures like MIPS and PowerPC, could this cause the
ip_fast_csum implementation to blindly decrement or loop until reaching the
end pointer, triggering an infinite loop or kernel panic?

[Severity: High]
This isn't a bug introduced by this patch, but could this validation suffer
from a Time-of-Check to Time-of-Use (TOCTOU) race?

The hdr argument can point to mutable eBPF map memory. A concurrent
modification by userspace or another CPU could alter iph->ihl between this
validation and its later uses. For example, if iph->ihl is mutated to 0 after
this check, it bypasses validation and the subsequent memcpy() copies the
mutated ihl to the SKB, leading to the same ip_fast_csum() kernel panic
described above.

Should the function copy the header into a local stack buffer or the SKB
first, and then perform all validations on that immutable copy?

> +		is_udp_tunnel = iph->protocol == IPPROTO_UDP;
> +		if (unlikely(is_udp_tunnel && len < iph->ihl * 4 + sizeof(struct udphdr)))
> +			return -EINVAL;
>  	} else if (iph->version == 6) {

[ ... ]

> @@ -637,6 +644,11 @@ 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 (is_udp_tunnel) {
> +		size_t iph_sz = ipv4 ? iph->ihl * 4 : sizeof(struct ipv6hdr);
> +
> +		skb_set_transport_header(skb, skb_network_offset(skb) + iph_sz);
> +	}

[Severity: High]
This is a pre-existing issue, but does this newly added code introduce an
additional TOCTOU race by re-reading iph->ihl?

Because iph->ihl is read again here from mutable eBPF map memory to calculate
iph_sz, a concurrent modification could result in an incorrect size calculation.
Could this allow skb_set_transport_header() to be set to a malicious
out-of-bounds offset?

>  	memcpy(skb_network_header(skb), hdr, len);
>  	bpf_compute_data_pointers(skb);
>  	skb_clear_hash(skb);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602150931.49629-1-leon.hwang@linux.dev?part=1