Re: [PATCH net v4 4/7] net: stmmac: selftests: Capture all packets for vlan checks

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

Critical: 0 · High: 1 · Medium: 2 · Low: 0

- [High] The "Means filter did not work" branch that this patch newly
  makes reachable encodes a detected filtering failure the same way the…
- [Medium] Making the VLAN selftest handler an ETH_P_ALL tap exposes the
  test's unsynchronized shared state to arbitrary concurrently received…
- [Medium] The VLAN validator now parses untrusted frames with an
  attacker-controlled offset and an insufficient length guard.

--

net: stmmac: selftests: Capture all packets for vlan checks

This reuses the DSA proxy mechanism for the VLAN selftests, registering an
ETH_P_ALL handler so that frames marked OTHERHOST still reach
stmmac_test_vlan_validate(). The destination address check inside the
validation function is moved earlier since external frames can now be seen.

[ ... ]

[Severity: High]

With the ETH_P_ALL registration the vid comparison further down in
stmmac_test_vlan_validate() becomes reachable on non-DSA devices, since
the ptype_all walk in __netif_receive_skb_core() happens before
__vlan_hwaccel_clear_tag() and skb->vlan_proto is no longer zero:

	if (tpriv->vlan_id) {
		if (skb->vlan_proto != htons(proto))
			goto out;
		if (skb->vlan_tci != tpriv->vlan_id) {
			/* Means filter did not work. */
			tpriv->ok = false;
			complete(&tpriv->comp);
			goto out;
		}
	}

Is the ok = false encoding here distinguishable from "nothing was
received"?  The callers in __stmmac_test_vlanfilt() and
__stmmac_test_dvlanfilt() do:

		wait_for_completion_timeout(&tpriv->comp, STMMAC_LB_TIMEOUT);
		ret = tpriv->ok ? 0 : -ETIMEDOUT;
		if (ret && !i) {
			goto vlan_del;
		} else if (!ret && i) {
			ret = -EINVAL;
			goto vlan_del;
		} else {
			ret = 0;
		}

For iterations i = 1..3 the frame is sent with VID tpriv->vlan_id + i and
is expected to be dropped by the hardware filter.  If the filter leaks the
frame, its tci cannot equal tpriv->vlan_id, so it can only take the
"filter did not work" branch, which yields ret = -ETIMEDOUT, and the else
arm then rewrites that to ret = 0.

Does this mean a real VLAN filter leak is reported as a pass for every
i > 0?

Conversely, at i == 0 any tagged frame addressed to our MAC that passes
the hash filter (the in-code comment above acknowledges false positives)
also lands in that branch and aborts the test with -ETIMEDOUT.

Would a distinct state, for example leaving ok = true or adding a separate
"unexpected frame" flag consumed by the !ret && i arm, be better here?

 		}
 	}
 
-	ehdr = (struct ethhdr *)skb_mac_header(skb);
-	if (!ether_addr_equal_unaligned(ehdr->h_dest, tpriv->packet->dst))
-		goto out;
-
 	ihdr = ip_hdr(skb);
 	if (tpriv->double_vlan)
 		ihdr = (struct iphdr *)(skb_network_header(skb) + 4);

[Severity: Medium]

Now that frames from the wire reach this parsing code, can the offset
arithmetic below read past the validated data?

	uhdr = (struct udphdr *)((u8 *)ihdr + 4 * ihdr->ihl);
	if (uhdr->dest != htons(tpriv->packet->dport))
		goto out;

	shdr = (struct stmmachdr *)((u8 *)uhdr + sizeof(*uhdr));
	if (shdr->magic != cpu_to_be64(STMMAC_TEST_PKT_MAGIC))

The only length guard is:

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

and STMMAC_TEST_PKT_SIZE is:

#define STMMAC_TEST_PKT_SIZE (sizeof(struct ethhdr) + sizeof(struct iphdr) + \
			      sizeof(struct stmmachdr))

so it guarantees 33 bytes after the MAC header, with no allowance for
struct udphdr and none for the extra 4 bytes of the double_vlan path.
ihdr->ihl comes from the wire and can be up to 15, so uhdr can sit up to
60 bytes past ihdr and shdr->magic can be read roughly 84 bytes past the
network header.

The proxy itself does no validation either, it clones after an ethertype
comparison only:

	struct ethhdr *hdr = eth_hdr(skb);

	if (hdr->h_proto == tpriv->packet_type) {
		struct sk_buff *nskb = skb_clone(skb, GFP_ATOMIC);

A 60-byte frame with ihl = 15 and protocol = UDP addressed to our MAC gives
skb_headlen() == 46, passes the 33-byte check, and then reads about 30
bytes beyond skb->tail.  Should a pskb_may_pull() or an explicit
remaining-length check against 4 * ihdr->ihl + sizeof(*uhdr) +
sizeof(*shdr) be added before these dereferences?

The same unchecked ihl arithmetic already exists in
stmmac_test_loopback_validate(), so that part is not new, but this change
is what exposes the VLAN validator to external frames.

[Severity: Medium]

Does the 4-iteration loop in __stmmac_test_vlanfilt() need a
reinit_completion()?  init_completion() is called once before the loop and
the loop only resets the verdict:

		tpriv->ok = false;
	}

With the promiscuous tap, unrelated received frames addressed to our MAC
can now call complete(&tpriv->comp) from the vid-mismatch branch of
stmmac_test_vlan_validate().  Every surplus complete() leaves done > 0, so
the next iteration's wait_for_completion_timeout() returns immediately
without ever waiting for its own frame.

Also, tpriv->ok is a plain int written from NAPI softirq context in
stmmac_test_vlan_validate() and written/read from process context in the
loop, with no common lock and no READ_ONCE/WRITE_ONCE.  If a softirq write
of ok = true lands after the process-context reset, the caller can observe
ok == true in an iteration where a timeout is expected and turn that into
a bogus -EINVAL.

[Severity: Medium]

The same two points apply to __stmmac_test_dvlanfilt(): the loop resets
tpriv->ok without re-arming the completion, and the verdict encoding
question above applies to its identical ret = tpriv->ok ? 0 : -ETIMEDOUT
handling.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917215339.2022523-1-maxime.chevallier%40bootlin.com