* Re: [PATCH net] esp: fix out-of-bounds transport-header read after exthdr walk
2026-10-09 13:52 [PATCH net] esp: fix out-of-bounds transport-header read after exthdr walk Henry Martin
@ 2026-10-09 13:54 ` netdev-bot+sinfo
2026-10-10 14:32 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sinfo @ 2026-10-09 13:54 UTC (permalink / raw)
To: Henry Martin
Cc: Steffen Klassert, Herbert Xu, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman, netdev, linux-kernel
Hi!
This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:
- Whether the issue was actually triggered, or is only theoretical
(e.g. found by code inspection). If it was triggered please include
the symptoms, like the stack trace or error messages.
Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.
The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net] esp: fix out-of-bounds transport-header read after exthdr walk
2026-10-09 13:52 [PATCH net] esp: fix out-of-bounds transport-header read after exthdr walk Henry Martin
2026-10-09 13:54 ` netdev-bot+sinfo
@ 2026-10-10 14:32 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-10-10 14:32 UTC (permalink / raw)
To: bsdhenrymartin
Cc: steffen.klassert, herbert, davem, edumazet, kuba, pabeni, horms,
netdev, linux-kernel
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 1 · Low: 1
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] Both new bounds checks compare a negative, data-relative offset
against skb->len using unsigned arithmetic, so the check is always…
- [Medium] Where the computed offset is non-negative, the new checks
bound it only by skb->len (total length including page frags and…
- [Low] The new comments and the commit message do not describe the main
receive paths accurately.
--
esp: fix out-of-bounds transport-header read after exthdr walk
This adds a bounds check to esp_input_done2() and to esp6_input_done2()
before the 2-byte source port is read from the encap transport header.
If the computed offset plus two bytes is past skb->len, the packet is
rejected with -EINVAL.
> diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
> index e76db5817e78e..aaff5a2b659db 100644
> --- a/net/ipv4/esp4.c
> +++ b/net/ipv4/esp4.c
> @@ -744,6 +744,18 @@ int esp_input_done2(struct sk_buff *skb, int err)
> struct udphdr *uh = (void *)(skb_network_header(skb) + ihl);
> __be16 source;
>
> + /*
> + * ihl is taken from the decrypted inner packet and is not
> + * re-validated after esp_remove_trailer() shrank the skb;
> + * make sure the 2-byte source port we read below is within
> + * the packet.
> + */
[Severity: Low]
Is this comment accurate for the UDP encap path?
In esp_input_done2(), ihl comes from:
iph = ip_hdr(skb);
ihl = iph->ihl * 4;
At this point that is the outer, unencrypted IPv4 header that
ip_rcv_core() validated. The inner network header is only reset later,
by the mode input callback.
The UDP header at network_header + ihl was already linearized by
__xfrm4_udp_encap_rcv():
if (!pskb_may_pull(skb, sizeof(struct udphdr) + min(len, 8)))
That header sits before skb->data, so esp_remove_trailer() trimming the
tail can't move it out of range.
The esp6 comment has the same problem. On the UDP path,
ipv6_skip_exthdr() walks outer extension headers that IPv6 input has
already validated, stops at the UDP header, and returns a negative
offset relative to skb->data.
The scenario might really happen on espintcp paths where the network
header is stale. tcp_collapse() leaves network_header at 0 for a new
head, and alloc_skb_for_msg() copies first->network_header into a
0-byte head through __copy_skb_header(). That would fit the Fixes: tag,
but neither the comments nor the commit message describe it.
Could the comments and the commit message describe how the offsets
actually work, and which path produces the bad header?
> + if (skb_network_offset(skb) + ihl + sizeof(__be16) >
> + skb->len) {
> + err = -EINVAL;
> + goto out;
> + }
[Severity: High]
Can this check ever pass for a valid encapsulated packet?
When esp_input_done2() runs, skb->data points at the ESP header. The
outer IP header and the UDP or TCP encap header both sit before
skb->data. For UDP, __xfrm4_udp_encap_rcv() pulls the UDP header off:
net/ipv4/xfrm4_input.c:__xfrm4_udp_encap_rcv() {
...
if (pull) {
__skb_pull(skb, len);
skb_reset_transport_header(skb);
...
}
For espintcp, espintcp_rcv() does:
if (!pskb_pull(skb, rxm->offset + 2)) {
So with UDP encap, skb_network_offset(skb) is -(ihl + 8), and
skb_network_offset(skb) + ihl is -8. Adding sizeof(__be16) converts
that to size_t, so the left side becomes a huge unsigned value and the
comparison is always true.
Wouldn't that drop every UDP_ENCAP_ESPINUDP and TCP_ENCAP_ESPINTCP
packet with -EINVAL after a successful decrypt? That would break both
NAT-T and espintcp receive.
The new check in esp6_input_done2() looks like it has the same problem:
if (offset + sizeof(__be16) > skb->len) {
offset starts at skb_network_offset(skb) + sizeof(*ip6h).
ipv6_skip_exthdr() stops at the UDP or TCP header, which gives -8 for
UDP. The existing code already depends on offset being negative there:
hdr_len += offset;
Also, skb->len counts bytes from skb->data onward, but the port being
read is before skb->data. Even a signed comparison against skb->len
would not bound this read.
Would something like skb_header_pointer_careful(), or a signed check
against the headroom and headlen, be needed instead of extending this
check?
> +
> switch (x->encap->encap_type) {
> case TCP_ENCAP_ESPINTCP:
> source = th->source;
> diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
> index b1c9b36f76dc4..89e8cdf5a26b2 100644
> --- a/net/ipv6/esp6.c
> +++ b/net/ipv6/esp6.c
> @@ -782,6 +782,17 @@ int esp6_input_done2(struct sk_buff *skb, int err)
> goto out;
> }
>
> + /*
> + * ipv6_skip_exthdr() can advance past the packet end when the
> + * last extension header claims an oversized length; the
> + * transport header below would then be read out of bounds.
> + * Make sure there is room for the 2-byte source port we read.
> + */
> + if (offset + sizeof(__be16) > skb->len) {
> + err = -EINVAL;
> + goto out;
> + }
> +
> uh = (void *)(skb->data + offset);
> th = (void *)(skb->data + offset);
> hdr_len += offset;
[Severity: Medium]
When offset is non-negative, does this check bound the read that
follows?
skb->len includes page frags and the frag_list. The port, however, is
read straight from the linear buffer through skb->data + offset. Any
offset with skb_headlen(skb) <= offset + 2 <= skb->len passes the
check, and the read then lands past the linear data, possibly past the
end of the head allocation.
Non-negative offsets seem possible on the espintcp paths where the
network header is stale. alloc_skb_for_msg() does:
n = alloc_skb(0, GFP_ATOMIC);
...
skb_shinfo(n)->frag_list = first;
__copy_skb_header(n, first);
and tcp_collapse() leaves network_header at 0. In both cases
ipv6_hdr() points at bytes that are not a real IPv6 header.
ipv6_skip_exthdr() walks with skb_header_pointer(), which can read
frags, and each step can advance up to 2048 bytes. esp6_input() does
not linearize the skb, so the offset can end up well inside skb->len
but past skb_headlen().
The esp4 check has the same skb->len bound in front of the direct
th->source and uh->source reads at skb_network_header(skb) + ihl.
I haven't confirmed the whole path end to end. Would reading the port
with skb_header_pointer_careful() handle both the negative-offset case
and the frag case?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009135233.4084239-1-bsdhenrymartin%40gmail.com
^ permalink raw reply [flat|nested] 3+ messages in thread