Re: [PATCH bpf v3 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:

New issues:
- [High] Time-of-Check to Time-of-Use (TOCTOU) vulnerability in transport header offset calculation leads to out-of-bounds write.

Pre-existing issues:
- [Critical] Missing validation of `iph->ihl >= 5` allows unprivileged BPF programs to trigger a kernel panic in `ip_fast_csum`.
--

commit 0be2d8b388e17b2000afd2e38ff89f5f956084ae
Author: Leon Hwang <leon.hwang@linux.dev>

bpf: Update transport_header when encapsulating UDP tunnel in lwt

This commit updates bpf_lwt_push_ip_encap() to set the skb transport
header whenever the encapsulated packet uses UDP. This ensures that
drivers relying on the transport header for hardware checksum offload
do not drop packets due to incorrect checksums.

Cc: Leon Hwang <leon.huangfu@shopee.com>

> diff --git a/net/core/lwt_bpf.c b/net/core/lwt_bpf.c
> index f71ef82a5f3d3..65d1dfbf3312b 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: Critical]
This is a pre-existing issue, but does this code properly validate the
minimum IPv4 header length?

Because len is an unsigned u32, if a BPF program provides a buffer where
iph->ihl is 0, iph->ihl * 4 will be 0. Since len < 0 is never true, the
check trivially passes without returning -EINVAL.

Later in the function, ip_fast_csum() is called with iph->ihl. On some
architectures (like PowerPC and MIPS), the ip_fast_csum() implementation
assumes ihl >= 5 and subtracts from it, causing an integer underflow in the
loop counter that leads to a deterministic kernel panic.

Should there be an explicit check that enforces iph->ihl >= 5 before the
length calculation?

> +		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,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 && is_udp_tunnel)
> +		skb_set_transport_header(skb, skb_network_offset(skb) + iph->ihl * 4);

[Severity: High]
Can iph->ihl change between the initial length validation and this call?

The hdr pointer (and thus iph) points to a buffer provided by the BPF
program, which might be concurrently modified by userspace via a shared
BPF map. Because skb_cow_head() acts as a compiler barrier, the compiler
will reload iph->ihl from this untrusted memory here.

If it is concurrently changed to a larger value (like 15) after the initial
validation, the transport header offset could be set beyond the end of the
SKB data (skb->tail).

Could this lead to out-of-bounds memory writes when network drivers later
dereference this transport header to update checksums?

> +	else if (!ipv4 && is_udp_tunnel)
> +		skb_set_transport_header(skb, skb_network_offset(skb) + sizeof(struct ipv6hdr));
>  	memcpy(skb_network_header(skb), hdr, len);

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