Re: [PATCH net v2 1/1] net: cap advertised device headroom

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch attempts to prevent 16-bit skb header offset overflows by capping `needed_headroom` to 512 at device configuration. However, this only mitigates overflows caused by a single massive headroom allocation. It fails to prevent cumulative overflows in the data path. An attacker can chain multiple tunnels and interpose a qdisc like `netem` to bypass the `dev_xmit_recursion()` limit (since `netem` defers transmission to a timer, unwinding the call stack and resetting the recursion counter). Because `needed_headroom` is capped, `skb_cow_head()` will repeatedly calculate a small `delta` and trigger `pskb_expand_head()`. Each call increments the 16-bit header offsets. After enough chained transmissions (e.g., ~1600 tunnels), the offsets will exceed 65535 and wrap around, leading to out-of-bounds reads or writes when the headers are accessed.
--

--- Patch [1]: [PATCH net v2 1/1] net: cap advertised device headroom ---
commit 9a8b47db3e094ff90f0e4cb79a3ba54d0ddcaab8
Author: Zhiling Zou <[email protected]>

net: cap advertised device headroom
    
IP tunnel and netkit devices can publish headroom values that are derived
from user-created stacked devices or directly supplied netlink attributes.
The dynamic tunnel transmit path already caps growing needed_headroom at
512.

Link: https://lore.kernel.org/all/0c6c64e9bbd71a0decc8504a384061e0e631be13.1785054561.git.zhilinz@nebusec.ai/

> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> index d708b66e55cda..85e3455cea259 100644
> --- a/include/net/ip_tunnels.h
> +++ b/include/net/ip_tunnels.h
> @@ -629,8 +629,7 @@ struct metadata_dst *iptunnel_metadata_reply(struct metadata_dst *md,
>  int skb_tunnel_check_pmtu(struct sk_buff *skb, struct dst_entry *encap_dst,
>  			  int headroom, bool reply);
>  
> -static inline void ip_tunnel_adj_headroom(struct net_device *dev,
> -					  unsigned int headroom)
> +static inline unsigned int ip_tunnel_limit_headroom(unsigned int headroom)
>  {
>  	/* we must cap headroom to some upperlimit, else pskb_expand_head
>  	 * will overflow header offsets in skb_headers_offset_update().
> @@ -640,6 +639,14 @@ static inline void ip_tunnel_adj_headroom(struct net_device *dev,
>  	if (headroom > max_allowed)
>  		headroom = max_allowed;
>  
> +	return headroom;
> +}

[Severity: High]
This is a pre-existing issue, but does capping needed_headroom to 512 here
completely mitigate the 16-bit header offset overflow in
skb_headers_offset_update()?

While this prevents a single massive allocation from overflowing the offsets,
it seems it doesn't prevent cumulative overflows in the data path. If a deep
chain of network tunnels is created and a delaying qdisc (like netem) is
attached to each to bypass the dev_xmit_recursion() limit (since netem
defers transmission to a timer, unwinding the call stack and resetting
the recursion counter), skb_cow_head() could repeatedly calculate a small
delta and trigger pskb_expand_head().

Each call increments the 16-bit header offsets in
skb_headers_offset_update():

net/core/skbuff.c:skb_headers_offset_update() {
    ...
    skb->mac_header += off;
    ...
}

After enough chained transmissions, could the offsets exceed 65535 and wrap
around, leading to out-of-bounds reads or writes when the network layers
access the wrapped header pointers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0ae4aa29223b89049727aec4d36f144bad41537e.1785476387.git.zhilinz@nebusec.ai?part=1