Re: [PATCH] net: dsa: sja1110: pull meta timestamps before reading them

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (1) · 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] In sja1110_rcv_meta() (net/dsa/tag_sja1105.c), the patch adds a
  pskb_may_pull() but leaves `buf` stale.
- [Low] The commit message is not fit to apply.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the trailer
  branch of sja1110_rcv_inband_control_extension()…

--

net: dsa: sja1110: pull meta timestamps before reading them

The patch adds a pskb_may_pull() to sja1110_rcv_meta(). The pull covers
the (n_ts + 1) ten-byte timestamp records before they are parsed. The goal
is to harden the SJA1110 meta frame parser against truncated or non-linear
skbs.

> net: dsa: sja1110: pull meta timestamps before reading them
>
> Hello,

[ ... ]

> This was found during an assisted (AI-supported) static source review of
> mainline fa4ad798f6f681031775266044ba1b0cf3c670bd (v7.4-rc1, 2026-11-07); every code path cited below was re-verified by the
> reporter against that tree. The patch below has NOT been compiled and has
> NOT been runtime tested, and no reproducer was produced - the triggering
> conditions are described above instead. Please treat this as a report for
> verification rather than a battle-tested fix.
>
> Reported-by: Noah Weinberger <noahweinberger2003@proton.me>

[Severity: Low]
This isn't a bug, but the changelog has a few problems for an applied
commit.

There is no Signed-off-by: line. The only trailer is a Reported-by: that
names the author.

The "Hello," greeting, the AI-assisted review note, the "has NOT been
compiled" disclaimer and the reference to the reporter's tree read like
cover letter text. Should they go below the --- line?

The message calls this "Longstanding code" but has no Fixes: tag. The meta
frame parsing seems to come from commit 566b18c8b752 ("net: dsa: sja1105:
implement TX timestamping for SJA1110"). The buf initialization was later
reworked by commit 5d928ff48656 ("net: dsa: create a helper for locating
EtherType DSA headers on RX").

The changelog also says the non-linear skb read is fixed. See the comment
below on buf: that claim may not hold.

> diff --git a/net/dsa/tag_sja1105.c b/net/dsa/tag_sja1105.c
> index bfe1f746f55be..caa66596a0840 100644
> --- a/net/dsa/tag_sja1105.c
> +++ b/net/dsa/tag_sja1105.c
> @@ -537,6 +537,12 @@ static struct sk_buff *sja1110_rcv_meta(struct sk_buff *skb, u16 rx_header)
>  	struct dsa_switch *ds;
>  	int i;
>  
> +	if (!pskb_may_pull(skb,
> +			   2 + (n_ts + 1) * SJA1110_META_TSTAMP_SIZE)) {
> +		kfree_skb(skb);
> +		return NULL;
> +	}
> +

[Severity: High]
Can buf end up pointing into freed memory after this pull? buf is set at
the top of sja1110_rcv_meta(), before the new check, and is never
reloaded:

	u8 *buf = dsa_etype_header_pos_rx(skb) + SJA1110_HEADER_LEN;

If the meta payload is not fully linear, pskb_may_pull() calls
__pskb_pull_tail(). When there is not enough tailroom, that function
reallocates the head:

	if (eat > 0 || skb_cloned(skb)) {
		if (pskb_expand_head(skb, 0, eat > 0 ? eat + 128 : 0,
				     GFP_ATOMIC))

pskb_expand_head() copies the data into a new head and frees the old one
with skb_free_head(). The kernel-doc for both functions says header
pointers must be reloaded after the call.

The loop further down still reads through the old pointer:

		ts_id = buf[0];
		source_port = (buf[1] & GENMASK(7, 4)) >> 4;
		dir = (buf[1] & BIT(3)) >> 3;
		tstamp = be64_to_cpu(*(__be64 *)(buf + 2));

Those values then go to tagger_data->meta_tstamp_handler().

dsa_switch_rcv() calls skb_unclone_full() before ->rcv(), so skb_cloned()
should not trigger the realloc. The eat > 0 case still can, for example
on a conduit that builds small fixed-size heads and puts the rest in
frags.

In the non-linear case the commit message describes, would this turn the
out-of-bounds read into a use-after-free read?

In sja1110_process_meta_tstamp(), a garbage ts_id that matches nothing
hits WARN_ON(!skb_match). A ts_id that does match would pass 8 bytes of
freed memory to skb_complete_tx_timestamp() as the TX timestamp.

Would it work to recompute buf after the pull succeeds, or to move the buf
initialization below the pskb_may_pull() check?

>  	cpu_dp = conduit->dsa_ptr;
>  	ds = dsa_switch_find(cpu_dp->dst->index, switch_id);
>  	if (!ds) {

[Severity: High]
This isn't a bug introduced by this patch, but the commit message's threat
model ("buggy or compromised switch silicon" and "short-skb conduit
paths") also applies to the caller, sja1110_rcv_inband_control_extension().
Its trailer branch looks like it can reach BUG() on a malformed header.

start_of_padding comes straight from the 12-bit header field and is never
checked:

	if (rx_header & SJA1110_RX_HEADER_HAS_TRAILER) {
		int start_of_padding = SJA1110_RX_HEADER_TRAILER_POS(rx_header);
		u8 *rx_trailer = skb_tail_pointer(skb) - SJA1110_RX_TRAILER_LEN;
		...
		if (pskb_trim_rcsum(skb, start_of_padding - ETH_HLEN)) {
			kfree_skb(skb);
			return NULL;
		}
	...
	/* Advance skb->data past the DSA header */
	skb_pull_rcsum(skb, SJA1110_HEADER_LEN);

If start_of_padding is between 14 and 17, the trim leaves skb->len at 0 to
3 and returns success. The unconditional skb_pull_rcsum(skb, 4) would then
hit BUG_ON(len > skb->len) in skb_pull_rcsum(). Should start_of_padding be
checked against ETH_HLEN + SJA1110_HEADER_LEN and skb->len?

The same branch reads the 13-byte trailer from skb_tail_pointer(skb) with
no check on skb->len or linearity. On a non-linear skb, would that parse
the end of the linear area instead of the real trailer?

In VLAN-filtering mode the taggers accept hardware-stripped VLAN tags. If
a conduit strips the 802.1Q tag from a front-port frame whose inner
EtherType is ETH_P_SJA1110, could rx_header contain bytes from the wire?
That has not been confirmed.

sja1105_meta_unpack() has a similar issue. It reads 8 bytes from
skb_mac_header(skb) + ETH_HLEN without a pskb_may_pull() or length check:

	u8 *buf = skb_mac_header(skb) + ETH_HLEN;

Should these sites be hardened as well?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/mnJ9l2Ebi69wyTB5k6FBEnjRESb0SmEnlqwaQAOEJ1x5qRQJ3ZMVAdX7Bn9-yWEnQ0iv_3KLoI1yUVCc88IJOVGGow9rOyG6Y54sPMeW0zQ%3D%40proton.me