Re: [PATCH net v3 1/1] net: gso: limit recursive IP-in-IP segmentation

[Willem de Bruijn]

Zihan Xi wrote:
> IPIP GSO/TSO support makes IP-in-IP GSO dispatch re-enter
> inet_gso_segment() or ipv6_gso_segment() for every nested IP header. The
> only state that tracks this nesting is encap_level, which records header
> bytes and has no recursion bound. A sufficiently deep chain can consume the
> kernel stack before a transport GSO callback is reached.
> 
> The unbounded callback nesting was introduced when inet_gso_segment() was
> made stackable by "ipv4: gso: make inet_gso_segment() stackable". GRE GSO
> support predated that change, and IP-in-IP GSO/TSO support later made the
> affected path reachable.
> 
> The corresponding IPv6 stackable path was introduced separately by
> "ipv6: gso: make ipv6_gso_segment() stackable". This patch uses the same
> budget for IPv6, but the Fixes tag covers the IPv4 root cause only.
> 
> Limit the cumulative header budget for a GSO operation to GSO_MAX_HEADER
> (256 bytes). Keep the consumed budget in skb_gso_cb and charge each header
> before dispatching the next GSO callback. The callback wrapper checks the
> same state, so direct IP handler re-entry and nested tunnel dispatch share
> one monotonic budget. GRE and UDP context resets cannot restart it before
> an inner GSO callback; other GSO tunnel and extension handlers charge their
> stripped headers before inner dispatch as well.
> 
> GSO_MAX_HEADER is a practical header budget, not a measured stack-overflow
> threshold or an architecture-independent stack-safety proof. With a zero
> initial offset, 12 minimum-sized IPv4 headers consume 240 bytes; the next
> header is rejected. IPv6 base headers, extension headers, and tunnel
> headers use the budget faster. Validation of the preceding
> implementation on x86_64 used a 16 KiB task stack and completed without a
> stack-guard fault, but this does not establish a uniform margin for
> architectures with smaller stacks.
> 
> Fixes: 3347c9602955 ("ipv4: gso: make inet_gso_segment() stackable")
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: Vega <vega@xxxxxxxxxx>
> Assisted-by: LLM
> Co-developed-by: Luxing Yin <root@xxxxxxxxxx>
> Signed-off-by: Luxing Yin <root@xxxxxxxxxx>
> Signed-off-by: Zihan Xi <zihanx@xxxxxxxxxx>
> 
> ---
> changes in v3:
>   - Treat an exhausted 256-byte budget as a callback-entry failure, including
>     the zero-length check used by the common callback wrapper and IP handlers.
>   - v2 Link: https://lore.kernel.org/all/cover.1789618203.git.zihanx@xxxxxxxxxx/
> changes in v2:
>   - Replace the callback counter with a cumulative 256-byte header budget
>     carried in skb_gso_cb.
>   - Apply the budget at common callback entry and across IP, GRE, UDP, MPLS,
>     NSH, ESP, and IPv6 extension dispatch, including GRE/UDP context resets.
>   - Rebase the UDP hunk onto selected revision c9151088f167 and rerun both
>     IPv4 PoCs; use the decoded crash evidence from unpatched 88c17de85ddb.
>   - v1 Link: https://lore.kernel.org/all/cover.1789302084.git.zihanx@xxxxxxxxxx/
>  include/net/gso.h       | 29 +++++++++++++++++++++++++++++
>  net/core/gso.c          |  7 +++++--
>  net/ipv4/af_inet.c      |  7 ++++++-
>  net/ipv4/esp4_offload.c |  9 +++++++--
>  net/ipv4/gre_offload.c  |  2 ++
>  net/ipv4/udp_offload.c  |  4 +++-
>  net/ipv6/esp6_offload.c |  9 +++++++--
>  net/ipv6/ip6_offload.c  | 11 ++++++++++-
>  net/mpls/mpls_gso.c     |  2 ++
>  net/nsh/nsh.c           |  2 ++
>  10 files changed, 73 insertions(+), 9 deletions(-)

This is a lot of code change compared to v1, a simple recursion
counter. Wang already suggested a simplication.

If the previous approach could be tested at only the two
network header callbacks, then this likely can too. By just bounding
skb_network_header - skb_mac_header? Or skb->data. Each pass through
these functions does an skb_pull.