Re: [PATCH bpf v2 2/4] bpf: Add check iph->ihl < 5 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] bpf_lwt_push_ip_encap updates skb->csum incorrectly for CHECKSUM_COMPLETE packets by calling skb_postpush_rcsum() before calculating the IP header checksum.
- [High] handle_gso_encap() hardcodes the inner IPv4 header length to 20 bytes, ignoring IP options, which leads to reading garbage data as transport headers.
--

commit f2ce106c7be32fdcff1b6d3eb89a4fe4496a86d1
Author: Leon Hwang <leon.hwang@linux.dev>
Subject: bpf: Add check iph->ihl < 5 in lwt

This commit adds a validation check for the IP header length (ihl) in the BPF
lightweight tunnel IP encapsulation path. It ensures that the header length is
at least 5 (20 bytes) to prevent an infinite loop in ip_fast_csum() on certain
architectures like MIPS.

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 8009e427851f1..c306120e11d29 100644
> --- a/net/core/lwt_bpf.c
> +++ b/net/core/lwt_bpf.c
> @@ -613,7 +613,7 @@ int bpf_lwt_push_ip_encap(struct sk_buff *skb, void *hdr, u32 len, bool ingress)
>  	iph = (struct iphdr *)buff;
>  	if (iph->version == 4) {
>  		ipv4 = true;
> -		if (unlikely(len < iph->ihl * 4))
> +		if (unlikely(iph->ihl < 5 || len < iph->ihl * 4))

[Severity: High]
This isn't a bug introduced by this patch, but since we are validating the IP
header here, could this lead to an incorrect skb->csum for CHECKSUM_COMPLETE
packets later in this function?

When a BPF program leaves iph->check = 0, skb_postpush_rcsum() is called:

    if (ingress)
        skb_postpush_rcsum(skb, iph, len);

This calculates the partial checksum of the IP header and adds it to
skb->csum. Later, the correct checksum is calculated and overwrites
iph->check:

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

However, skb->csum is not updated to reflect this modification. Because a
valid IP header's checksum zeroes out its 16-bit one's complement sum, does
the addition in skb_postpush_rcsum() incorrectly add the header's
unchecksummed sum to skb->csum, causing packets to fail CHECKSUM_COMPLETE
validation and drop?

[Severity: High]
This is a pre-existing issue, but since this patch explicitly permits IP
headers with options (where iph->ihl > 5), does handle_gso_encap() handle
them correctly at the end of this function?

If the packet requires GSO, handle_gso_encap() hardcodes the IPv4 header
length to 20 bytes:

net/core/lwt_bpf.c:handle_gso_encap() {
    ...
    next_hdr_offset = sizeof(struct iphdr);
    ...
}

If IP options are present, won't handle_gso_encap() mistakenly read the IP
options as the transport header's protocol flags (e.g., GRE flags or UDP
checksum)? 

Will this misconfigure GSO and result in corrupted segmented packets or
hardware drops?

>  			return -EINVAL;
>  	} else if (iph->version == 6) {
>  		ipv4 = false;

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