Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · 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).
- [Medium] The re-segmentation path in validate_xmit_skb() /
skb_gso_output_max_segs() lets untrusted SKB_GSO_DODGY TCP skbs reach…
--
net: core: re-segment oversized TCP GSO skbs
With this patch, validate_xmit_skb() no longer segments an unencapsulated
TCP GSO skb into individual packets when it exceeds the egress device's
gso_max_size or gso_max_segs. It re-segments the skb into smaller GSO skbs
instead. A new helper, skb_gso_output_max_segs(), works out how many MSS
segments each output skb may carry and passes that to __skb_gso_segment()
as max_segs.
> diff --git a/net/core/dev.c b/net/core/dev.c
> index a6213c9ed5e72..ff9bce2506f67 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3977,6 +3977,53 @@ __netif_skb_features(struct sk_buff *skb, bool
> check_gso_limits)
> }
> EXPORT_SYMBOL(__netif_skb_features);
>
> +static unsigned int
> +skb_gso_output_max_segs(struct sk_buff *skb, struct net_device *dev)
> +{
> + unsigned int mss = skb_shinfo(skb)->gso_size;
> + unsigned int gso_max_size, hdr_len, max_segs;
> + netdev_features_t features;
> + struct tcphdr _tcph, *th;
> +
> + if (!skb_is_gso(skb) || !skb_is_gso_tcp(skb) ||
> + skb->encapsulation || skb_has_frag_list(skb))
> + return 0;
[ ... ]
> + gso_max_size = netif_get_gso_max_size(dev, skb);
> + gso_max_size = min(gso_max_size, GSO_LEGACY_MAX_SIZE);
> +
> + /*
> + * gso_within_dev_limits() accepts gso_segs == gso_max_segs but
> + * rejects skb->len >= gso_max_size, so only the size limit needs the
> + * - 1; the inner min() keeps that subtraction from wrapping.
> + */
> + max_segs = (gso_max_size - min(gso_max_size, hdr_len + 1)) / mss;
> + max_segs = min(max_segs, READ_ONCE(dev->gso_max_segs));
> +
> + return max_segs;
> +}
[Severity: Medium]
Can untrusted SKB_GSO_DODGY skbs now reach the DEBUG_NET_WARN_ON_ONCE() in
skb_segment()?
When max_segs is non-zero, skb_segment() skips the frag_list block. It then
runs the assertion against the full payload length:
net/core/skbuff.c:skb_segment() {
...
if (sg && csum && !gso_by_frags) {
if (!max_segs && !(features & NETIF_F_GSO_PARTIAL)) {
...
if (!list_skb ||
!net_gso_ok(features,
skb_shinfo(head_skb)->gso_type))
goto normal;
...
}
...
DEBUG_NET_WARN_ON_ONCE(len / mss > GSO_MAX_SEGS);
...
}
Two things keep that assertion true today. First,
__virtio_net_hdr_to_skb() clamps TCP gso_size to at least TCP_MIN_GSO_SIZE,
guarded by:
BUILD_BUG_ON(TCP_MIN_GSO_SIZE * GSO_MAX_SEGS < GSO_MAX_SIZE);
Second, the grouping path was only used for skbs that passed
gso_within_dev_limits(), so len was below GSO_MAX_SIZE. The new path runs
only for skbs that fail those limits, so their length is no longer bounded
by GSO_MAX_SIZE.
With PACKET_VNET_HDR, packet_snd() does not check the length against the
MTU when a gso_type is set. packet_alloc_skb() accepts up to
MAX_SKB_FRAGS * (PAGE_SIZE << PAGE_ALLOC_COSTLY_ORDER) bytes of frags, plus
a larger linear part.
So a CAP_NET_RAW user, including user namespace root, can build a
TCPv4/v6 GSO skb with NEEDS_CSUM, gso_size 8 and a payload of 524288 bytes
or more. tun/tap goes through the same virtio_net_hdr_to_skb() helper.
For such an skb the path looks like this:
dev_queue_xmit()
validate_xmit_skb()
gso_within_dev_limits() fails
skb_gso_output_max_segs() returns about (65536 - hdr_len - 1) / 8
__skb_gso_segment(skb, features, true, max_segs)
tcp_gso_segment()
skb_segment()
DEBUG_NET_WARN_ON_ONCE(len / mss > GSO_MAX_SEGS)
Every check in the helper passes here. csum_start equals the transport
header set by skb_partial_csum_set(), and net_gso_ok() succeeds on a TSO
device.
Before this patch the same skb had max_segs == 0 and no GSO_PARTIAL. It
took the goto normal path and never reached the assertion.
The output segments should still be correct, since
partial_segs = min(len / mss, max_segs). But with CONFIG_DEBUG_NET and
panic_on_warn, the new warning would panic the kernel.
Could skb_gso_output_max_segs() return 0 when (skb->len - hdr_len) / mss
exceeds GSO_MAX_SEGS?
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930111526.2183107-1-wang.zhan%40smartx.com
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
