Re: [PATCH net] skbuff: keep accesses through unset transport header in bounds

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · Medium: 0 · 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] The fix is incomplete because the length helpers that read the
  same fields were not changed.
- [High] The new guards in skb_transport_header() and
  skb_inner_transport_header() only fire when the field equals exactly…
- [Low] skb_inner_transport_header() checks inner_transport_header
  against ~0U.

--

skbuff: keep accesses through unset transport header in bounds

This commit changes skb_transport_header() and skb_inner_transport_header()
so that they warn under CONFIG_DEBUG_NET when the transport header offset
holds the ~0 unset value. In that case they fall back to the network header
position. The aim is to keep accesses through an unset transport header
inside the packet instead of at skb->head + 0xffff.

> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 6787fcf6ac3e4..e0e2f925f7299 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3002,6 +3002,12 @@ static inline void skb_reset_mac_len(struct sk_buff *skb)
>  static inline unsigned char *skb_inner_transport_header(const struct sk_buff
>  							*skb)
>  {
> +	if (unlikely(skb->inner_transport_header ==
> +		     (typeof(skb->inner_transport_header))~0U)) {

[Severity: Low]
This isn't a bug, but is ~0U the right unset value to test for the inner
transport header? The rest of the inner header code uses 0 to mean unset:

include/linux/skbuff.h:skb_inner_network_header_was_set() {
	return skb->inner_network_header > 0;
}

net/core/skbuff.c:skb_carve_reset_headers() {
	...
	/* Inner offsets have no "unset" marker, zero them so that
	 * skb_inner_network_header_was_set() becomes false and no
	 * consumer mistakes them for a real (and long gone) header.
	 */
	skb->inner_mac_header = 0;
	skb->inner_network_header = 0;
	skb->inner_transport_header = 0;
	...
}

The ~0U case can still happen, because skb_reset_inner_headers() copies an
unset outer transport_header into inner_transport_header. An inner
transport header that was never set (0) is not caught by this check,
though. It still returns skb->head. That pointer is in bounds, and the
behaviour is the same as before the patch.

> +		DEBUG_NET_WARN_ON_ONCE(1);
> +		return skb->head + skb->inner_network_header;
> +	}
> +
>  	return skb->head + skb->inner_transport_header;
>  }
>  
> @@ -3081,7 +3087,11 @@ static inline void skb_unset_transport_header(struct sk_buff *skb)
>  
>  static inline unsigned char *skb_transport_header(const struct sk_buff *skb)
>  {
> -	DEBUG_NET_WARN_ON_ONCE(!skb_transport_header_was_set(skb));
> +	if (unlikely(!skb_transport_header_was_set(skb))) {

[Severity: High]
Does the unset value always survive until this check? The new test only
matches 0xffff exactly. skb_headers_offset_update() shifts both transport
offsets unconditionally, while it does guard mac_header:

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

pskb_expand_head() calls it with a positive nhead, so 0xffff wraps to
nhead - 1. skb_copy_expand() calls it with newheadroom - oldheadroom. That
value is negative when the new headroom is smaller, which gives 0xffff - k.

In both cases skb_transport_header_was_set() returns true, so no warning
fires. In the second case skb_transport_header() still returns a pointer
about 64K past skb->head.

One RX path that looks like it can reach this is vl600_rx_fixup() in
drivers/net/usb/lg-vl600.c:

	s->current_rx_buf = skb_copy_expand(skb, 0,
			le32_to_cpup(&frame->len), GFP_ATOMIC);

That buffer is later passed up with usbnet_skb_return(). Because was_set()
now returns true, __netif_receive_skb_core() also skips its reset:

	if (!skb_transport_header_was_set(skb))
		skb_reset_transport_header(skb);

Should skb_headers_offset_update() leave unset transport offsets alone, as
it already does for mac_header?

> +		DEBUG_NET_WARN_ON_ONCE(1);
> +		return skb->head + skb->network_header;
> +	}

[Severity: High]
Should the length helpers that read the same fields use the same fallback?
skb_transport_offset() picks it up through skb_transport_header(), but
skb_network_header_len() and skb_inner_network_header_len() still use the
raw offsets:

include/linux/skbuff.h:
static inline u32 skb_network_header_len(const struct sk_buff *skb)
{
	DEBUG_NET_WARN_ON_ONCE(!skb_transport_header_was_set(skb));
	return skb->transport_header - skb->network_header;
}

static inline u32 skb_inner_network_header_len(const struct sk_buff *skb)
{
	return skb->inner_transport_header - skb->inner_network_header;
}

With an unset header, both still return about 0xffff minus the network
offset. skb_inner_network_header_len() has no check at all. Callers that
use these values for pulls, copies or offset arithmetic would still get a
length of about 64K.

Code that reads skb->transport_header directly, such as
virtio_net_hdr_tnl_from_skb(), also bypasses the fallback.

Does this match the commit message's "accesses stay within the packet"?

There is also a new mismatch. Before this patch,
skb_transport_header() - skb_network_header() always equalled
skb_network_header_len(). With an unset header they now differ.
ipv6_mc_may_pull() mixes the two:

include/net/addrconf.h:ipv6_mc_may_pull() {
	if (skb_transport_offset(skb) + ipv6_transport_len(skb) < len)
		return false;

	return pskb_may_pull(skb, len);
}

include/linux/ipv6.h:ipv6_transport_len() {
	return ntohs(ipv6_hdr(skb)->payload_len) + sizeof(struct ipv6hdr) -
	       skb_network_header_len(skb);
}

Before the patch the raw offsets cancelled out. Now the first term is
clamped to the network offset, but ipv6_transport_len() still subtracts the
raw ~64K value.

Can the unsigned sum wrap here and skip the advertised length check,
leaving pskb_may_pull() as the only bound?

> +
>  	return skb->head + skb->transport_header;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/0bdcbe2ab5ffc86ba7190fe25476bcebcb967310.1792844270.git.mst%40redhat.com