Re: [PATCH net v3] net: gso: validate transport header before computing segment length
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-10-25 04:16:46
Also in:
bpf, lkml
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider. Critical: 0 · High: 1 · Medium: 0 · Low: 0 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] The patch is incomplete for the producer its own commit message names. -- net: gso: validate transport header before computing segment length This patch makes the GSO segment length helpers in net/core/gso.c check the TCP header before they read it. If the transport header is unset, or the TCP header is not in the linear data, skb_gso_validate_network_len() and skb_gso_validate_mac_len() now report that the skb does not fit. It also adds two helpers, skb_inner_transport_header_was_set() and skb_transport_header_linear(), to include/linux/skbuff.h.
quoted hunk ↗ jump to hunk
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index d6894f463bf43..11c8b295bd260 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h
[ ... ]
quoted hunk ↗ jump to hunk
@@ -3250,6 +3257,29 @@ static inline int skb_transport_offset(const struct sk_buff *skb) return skb_transport_header(skb) - skb->data; } +/* Is the (inner) transport header, as specified by + * skb->transport_header/inner_transport_header, in the linear data? + */ +static inline bool skb_transport_header_linear(const struct sk_buff *skb, + unsigned int hlen) +{ + int thoff; + + if (!skb_transport_header_was_set(skb)) + return false; + + if (skb->encapsulation) { + if (!skb_inner_transport_header_was_set(skb)) + return false; + + thoff = skb_inner_transport_offset(skb); + } else { + thoff = skb_transport_offset(skb); + } + + return thoff >= 0 && thoff + hlen <= skb_headlen(skb);
[Severity: High]
The commit message names bpf_skb_adjust_room() as a way to leave a bad
transport header behind. It also says:
Qdisc length estimation has carried a guard against exactly this since
Is that guard enough for the bpf_skb_adjust_room() case? In
qdisc_pkt_len_segs_init(), the only check is for an unset transport
header. Nothing there matches the thoff >= 0 check added here:
net/core/dev.c:qdisc_pkt_len_segs_init() {
...
if (!skb->encapsulation) {
if (unlikely(!skb_transport_header_was_set(skb)))
return SKB_NOT_DROPPED_YET;
hdr_len = skb_transport_offset(skb);
} else {
hdr_len = skb_inner_transport_offset(skb);
}
/* + transport layer */
if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))) {
const struct tcphdr *th;
if (!pskb_may_pull(skb, hdr_len + sizeof(struct tcphdr)))
return SKB_DROP_REASON_SKB_BAD_GSO;
th = (const struct tcphdr *)(skb->data + hdr_len);
tlen = __tcp_hdrlen(th);
...
}
bpf_skb_net_hdr_pop() moves mac_header and network_header forward. It
leaves transport_header where it was, unless it was equal to
network_header:
net/core/filter.c:bpf_skb_net_hdr_pop() {
...
skb->mac_header += len;
skb->network_header += len;
if (trans_same)
skb->transport_header = skb->network_header;
...
}
When the DECAP_L4 or DECAP_IPXIP flags remove the last tunnel GSO bit,
bpf_skb_net_shrink() also clears skb->encapsulation.
Take a locally VXLAN-encapsulated TCP TSO skb whose transport_header
points at the outer UDP header. If a tc program calls
bpf_skb_adjust_room(skb, -50, BPF_ADJ_ROOM_MAC,
BPF_F_ADJ_ROOM_DECAP_L4_UDP | ...), the transport offset ends up at -16
and encapsulation is 0.
A simpler BPF_ADJ_ROOM_NET shrink of 40 bytes on a plain Ethernet/IPv4
TSO skb gives an offset of -6.
If the program then redirects to egress with bpf_redirect(), this path
runs before any of the validators hardened in this patch:
__bpf_tx_skb()
dev_queue_xmit()
__dev_queue_xmit()
qdisc_pkt_len_segs_init()
In that function hdr_len is an unsigned int, so -16 becomes 0xfffffff0.
hdr_len + sizeof(struct tcphdr) is then truncated to 4 when passed as the
unsigned int len argument of pskb_may_pull(), so the pull succeeds.
Wouldn't __tcp_hdrlen(th) then read about 4GB past skb->data on 64-bit?
Should qdisc_pkt_len_segs_init() get the same check, for example by
using skb_transport_header_linear()? If not, could the commit message
be changed so it does not say the qdisc path already guards against
this?
quoted hunk ↗ jump to hunk
+} + static inline u32 skb_network_header_len(const struct sk_buff *skb) { DEBUG_NET_WARN_ON_ONCE(!skb_transport_header_was_set(skb));
[ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/52b8bfe4ee848aff8ace47752352de486171d45f.1792595507.git.mst%40redhat.com