Re: [PATCH bpf v2 1/4] bpf: Fix TOCTOU issue in lwt

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Using plain `memcpy()` to read concurrently mutable BPF map memory causes a data race.

Pre-existing issues:
- [High] Modifying `iph->check` after `skb_postpush_rcsum()` corrupts `skb->csum` for `CHECKSUM_COMPLETE` packets.
- [High] `handle_gso_encap()` hardcodes IPv4 header length to 20 bytes, ignoring IP options and leading to corrupted GSO parsing.
- [High] The pushed header length `len` is not validated for 4-byte alignment, causing unaligned memory accesses and kernel panics.
--

commit 9a2f99096b35970231963f30d1371e5ea1824572
Author: Leon Hwang <leon.hwang@linux.dev>

bpf: Fix TOCTOU issue in lwt

This commit fixes a TOCTOU issue where the header pointer passed to
bpf_lwt_push_ip_encap() could point to concurrently mutable memory like
a BPF map value. It resolves this by copying the header into a local
buffer before validating and pushing it.

Link: https://lore.kernel.org/bpf/20260525150010.CDEBA1F000E9@smtp.kernel.org/

> diff --git a/net/core/lwt_bpf.c b/net/core/lwt_bpf.c
> index f71ef82a5f3d3..8009e427851f1 100644
> --- a/net/core/lwt_bpf.c
> +++ b/net/core/lwt_bpf.c
> @@ -599,6 +599,7 @@ static int handle_gso_encap(struct sk_buff *skb, bool ipv4, int encap_len)
>  
>  int bpf_lwt_push_ip_encap(struct sk_buff *skb, void *hdr, u32 len, bool ingress)
>  {
> +	u8 buff[LWT_BPF_MAX_HEADROOM];
>  	struct iphdr *iph;
>  	bool ipv4;
>  	int err;
> @@ -606,8 +607,10 @@ int bpf_lwt_push_ip_encap(struct sk_buff *skb, void *hdr, u32 len, bool ingress)
>  	if (unlikely(len < sizeof(struct iphdr) || len > LWT_BPF_MAX_HEADROOM))
>  		return -EINVAL;
>  

[Severity: High]
This is a pre-existing issue, but is it possible for the len argument to be
unaligned here? If len is not a multiple of 4 (e.g. 21 bytes), skb_push()
later will shift skb->data by len bytes, making skb->data unaligned.

When ip_fast_csum() is called, it performs 32-bit memory accesses on the
pointer. On architectures that do not support unaligned memory accesses
natively (like older ARM, MIPS, or SPARC), would this trigger a fatal
alignment exception and crash the kernel?

> +	memcpy(buff, hdr, len);

[Severity: Medium]
Since hdr can point to BPF map memory that is concurrently mutated by user
space or other CPUs, could this plain memcpy() cause a data race? The
compiler might emit multiple loads or tear the reads, potentially resulting
in an inconsistent header state and triggering KCSAN warnings.

Should data_race() or a lockless safe memory copy mechanism be used here?

> +
>  	/* validate protocol and length */
> -	iph = (struct iphdr *)hdr;
> +	iph = (struct iphdr *)buff;

[ ... ]

> @@ -637,7 +640,7 @@ 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);
> -	memcpy(skb_network_header(skb), hdr, len);
> +	memcpy(skb_network_header(skb), buff, len);
>  	bpf_compute_data_pointers(skb);
>  	skb_clear_hash(skb);

[Severity: High]
This isn't a bug introduced by this patch, but does modifying iph->check
after skb_postpush_rcsum() corrupt skb->csum for CHECKSUM_COMPLETE packets?

When an IPv4 header is pushed on an ingress packet, skb_postpush_rcsum()
adds the checksum of the pushed header to skb->csum. If the BPF program
provides an IP header with iph->check == 0, skb->csum is updated using this
zeroed checksum.

Later in this function, we have:

	if (ipv4) {
		skb->protocol = htons(ETH_P_IP);
		iph = ip_hdr(skb);

		if (!iph->check)
			iph->check = ip_fast_csum((unsigned char *)iph,
						  iph->ihl);

This calculates and overwrites iph->check in the SKB, making the IP header
mathematically sum to 0. However, skb->csum is not updated to reflect this
modification, leaving it inflated by the partial sum of the IP header.

Could this break CHECKSUM_COMPLETE synchronization, causing valid packets to
be dropped later in the network stack?


[Severity: High]
This is a pre-existing issue, but does handle_gso_encap() properly handle
IPv4 headers with IP options?

At the end of bpf_lwt_push_ip_encap(), handle_gso_encap() is called:

	if (skb_is_gso(skb))
		return handle_gso_encap(skb, ipv4, len);

bpf_lwt_push_ip_encap() allows BPF programs to push IPv4 headers with IP
options (where iph->ihl > 5). However, if the packet is a GSO packet,
handle_gso_encap() hardcodes the IPv4 header length to sizeof(struct iphdr)
instead of using ip_hdr(skb)->ihl * 4.

Would this cause the function to point next_hdr into the middle of the IP
options rather than the encapsulated inner protocol header (e.g., GRE or
UDP), leading to corrupted GSO parsing?

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