Re: [PATCH net-next 1/3] net: selftests: use skb_share_check() in the loopback receive hook

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0 (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 series fixes only part of this bug class.

Pre-existing issues:
- [Low] Pre-existing weak length validation in
  net_test_loopback_validate() (net/core/selftests.c).

--

net: selftests: use skb_share_check() in the loopback receive hook

This patch replaces skb_unshare() with skb_share_check() in
net_test_loopback_validate(). With the change, a shared but non-cloned skb
is cloned before skb_linearize() runs. Without it, a non-linear frame that
reaches the hook while another ETH_P_IP handler is bound to the device can
hit BUG_ON(skb_shared()).

> diff --git a/net/core/selftests.c b/net/core/selftests.c
> index 36b949ae520b3..80029a67b2d2b 100644
> --- a/net/core/selftests.c
> +++ b/net/core/selftests.c
> @@ -170,7 +170,7 @@ static int net_test_loopback_validate(struct sk_buff *skb,
>  	struct tcphdr *thdr;
>  	struct iphdr *ihdr;
>  
> -	skb = skb_unshare(skb, GFP_ATOMIC);
> +	skb = skb_share_check(skb, GFP_ATOMIC);
>  	if (!skb)
>  		goto out;
>  

[Severity: High]
This change looks correct for net_test_loopback_validate(). The same
pattern still seems to be present in stmmac_test_vlan_validate() in
drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c, though.

A later commit in this series, "net: stmmac: selftests: use
skb_share_check() in the loopback receive hook", converts
stmmac_test_loopback_validate(). The second copy of this handler in the
same file is left as:

stmmac_test_vlan_validate() {
    ...
	skb = skb_unshare(skb, GFP_ATOMIC);
	if (!skb)
		goto out;

	if (skb_linearize(skb))
		goto out;
    ...
}

__stmmac_test_vlanfilt() and the other VLAN tests register this handler as
a device-bound packet_type. The type is ETH_P_IP, ETH_P_8021Q, or either
one depending on svlan:

	tpriv->pt.type = htons(ETH_P_IP);
	tpriv->pt.func = stmmac_test_vlan_validate;
	tpriv->pt.dev = priv->dev;

Suppose another handler for the same type sits later in the device's
ptype list, for example an AF_PACKET socket bound to ETH_P_IP on that
interface. deliver_skb() then increments skb->users, so the hook gets an
skb that is shared but not cloned. skb_unshare() only checks
skb_cloned(), so it returns that skb unchanged.

For a non-linear frame with less tailroom than data_len, this path
follows:

skb_linearize()
  __pskb_pull_tail()
    pskb_expand_head()
      BUG_ON(skb_shared(skb));

Should stmmac_test_vlan_validate() get the same skb_share_check()
conversion, so the series covers every copy of this hook?

The xgbe sibling, xgbe_test_loopback_validate(), does not seem to be
affected. It already calls skb_share_check() before it expands a shared
skb.

[Severity: Low]
This is a pre-existing issue that this patch did not introduce. The
length check further down in net_test_loopback_validate() looks too
short for what the function parses afterwards:

	if (skb_headlen(skb) < (NET_TEST_PKT_SIZE - ETH_HLEN))
		goto out;

NET_TEST_PKT_SIZE in include/net/selftests.h covers only ethhdr, iphdr
and netsfhdr. That guarantees 33 bytes past the network header and does
not include the TCP/UDP header.

The function then reads:

	thdr = (struct tcphdr *)((u8 *)ihdr + 4 * ihdr->ihl);
	...
	if (shdr->magic != cpu_to_be64(NET_TEST_PKT_MAGIC))

It never checks ihl >= 5, the IP version, tot_len or the L4 header
length, and double_vlan adds another 4 bytes.

Could an IPv4 frame that arrives during ethtool -t make the hook read up
to about 64 bytes past skb->tail? Examples would be a minimum-size TCP
segment or a frame with ihl=15.

The over-read seems to stay inside the skb head allocation, because
skb_shared_info follows skb->end. If so, the only effect is a possibly
wrong selftest result.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007-nb-net-selftest-shared-net-v1-0-458369bfa1ba%40tipi-net.de