Re: [PATCH net 3/6] net: bcmasp: validate minimum RX packet size in bcmasp_rx_poll()

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 (2) · 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 new minimum-size check in bcmasp_rx_poll()
  (bcmasp_intf.c:526) is `len < 2 + (intf->crc_fwd ?
- [Medium] The commit message overstates the impact.
- [Low] The new runt/invalid-length drop branch in bcmasp_rx_poll()
  (lines 526-530) increments `stats->rx_dropped`.

Pre-existing issues:
- [Medium] bcmasp_rx_poll() never checks desc->size or
  DESC_ADDR(desc->buf) against an upper limit.
- [Medium] One `struct u64_stats_sync` (`intf->stats64.syncp`) is written
  by several contexts that are not mutually exclusive: - `rx_napi`…

--

net: bcmasp: validate minimum RX packet size in bcmasp_rx_poll()

This patch adds a lower bound check on desc->size in bcmasp_rx_poll().
Descriptors smaller than the 2-byte alignment pad plus the optional FCS
are counted in rx_dropped and skipped before an skb is built.

> If the hardware reports a descriptor size smaller than the pad and CRC
> lengths (e.g. runt or corrupted frames), len -= 2 or len - ETH_FCS_LEN
> underflows u32 len, leading to out-of-bounds trimming and memory
> corruption.

[Severity: Medium]
Is "out-of-bounds trimming and memory corruption" accurate here? Both
helpers used by bcmasp_rx_poll() appear to guard against the underflowed
values:

include/linux/skbuff.h:skb_pull_inline() {
	return unlikely(len > skb->len) ? NULL : __skb_pull(skb, len);
}

net/core/skbuff.c:skb_trim() {
	if (skb->len > len)
		__skb_trim(skb, len);
}

If desc->size is below 2, skb_pull() returns NULL and pulls nothing. If
len - ETH_FCS_LEN wraps to a value near 4G, skb_trim() does nothing.

Before this patch, the visible effects seem to be two:

  - a 0-3 byte skb reaches eth_type_trans() and GRO
  - the wrapped len is added to rx_bytes, about 4 GiB per frame:

	u64_stats_add(&stats->rx_bytes, len);

Neither of these is an out-of-bounds write.

The driver also never sets UMC_CMD_RUNT_FILTER_DIS, so the UniMAC runt
filter should stay enabled. That leaves misbehaving or corrupted
descriptors as the only way to reach this path.

This carries a Fixes: tag, so it will go to stable. Could the commit
message describe the actual impact, and say whether such descriptors have
been seen on real hardware?

> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> index 9ad5a982542f5..2ad8a7eac8888 100644
> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> @@ -523,6 +523,12 @@ static int bcmasp_rx_poll(struct napi_struct *napi, int budget)
>  					DMA_FROM_DEVICE);
>  
>  		len = desc->size;
> +		if (unlikely(len < 2 + (intf->crc_fwd ? ETH_FCS_LEN : 0))) {

[Severity: High]
Should this threshold also include ETH_HLEN?

With crc_fwd set, any desc->size from 6 to 19 passes this check. With
crc_fwd clear, the range is 2 to 15. After the pad pull and the FCS trim,
the skb holds 0-13 bytes. With desc->size == 6 and crc_fwd set,
skb->len == 0.

That skb still goes to eth_type_trans(), and eth_skb_pull_mac() ignores
the failed pull:

include/linux/etherdevice.h:eth_skb_pull_mac() {
	struct ethhdr *eth = (struct ethhdr *)skb->data;

	skb_pull_inline(skb, ETH_HLEN);
	return eth;
}

eth_skb_pkt_type() then reads h_dest, and eth_type_trans() reads
h_proto, from bytes past skb->len. The memcpy() never wrote those bytes.
They are stale page_pool page contents, or the stripped CRC, and they
decide skb->pkt_type and skb->protocol.

The frame is then passed to napi_gro_receive(). If a packet socket
receives it, eth_header_parse() copies h_source into sll_addr. Can that
leak 6 stale bytes to userspace?

Sizes just above the new limit still reach this path for the runt or
corrupted descriptors the commit message describes. bcmasp_xmit() already
uses ETH_ZLEN + ETH_FCS_LEN as the minimum frame size.

Would 2 + ETH_HLEN + (crc_fwd ? ETH_FCS_LEN : 0), or a bound based on
ETH_ZLEN, be more appropriate here?

> +			u64_stats_update_begin(&stats->syncp);
> +			u64_stats_inc(&stats->rx_dropped);

[Severity: Low]
Is rx_dropped the right counter for a length error? The page_pool and
napi_build_skb() allocation failures also use it, and
bcmasp_get_stats64() exports it directly:

	stats->rx_dropped = u64_stats_read(&lstats->rx_dropped);
	stats->rx_crc_errors = u64_stats_read(&lstats->rx_crc_errs);
	stats->rx_frame_errors = u64_stats_read(&lstats->rx_sym_errs);
	stats->rx_errors = stats->rx_crc_errors + stats->rx_frame_errors;

include/uapi/linux/if_link.h describes the two counters differently:

  - rx_dropped: "e.g. due to lack of resources or unsupported protocol"
  - rx_length_errors: "Number of packets dropped due to invalid length",
    which is also counted in rx_errors

As written, ip -s link can't tell these drops apart from allocation
failures, and they never show up in rx_errors or rx_length_errors.

> +			u64_stats_update_end(&stats->syncp);

[Severity: Medium]
This is a pre-existing issue, but intf->stats64.syncp seems to have
several writers that are not mutually exclusive:

  - rx_napi, here in bcmasp_rx_poll()
  - tx_napi, via bcmasp_tx_poll()->bcmasp_tx_reclaim()
  - process context, in bcmasp_netif_deinit()

bcmasp_netif_deinit() {
    ...
	bcmasp_tx_reclaim(intf);
    ...
	napi_disable(&intf->rx_napi);
    ...
}

So bcmasp_tx_reclaim() runs while rx_napi is still enabled. The two napi
instances can also run at the same time under threaded napi or on
different CPUs.

On 32-bit kernels, could two concurrent u64_stats_update_begin()/end()
pairs lose a sequence increment and leave the seqcount odd? If so,
bcmasp_get_stats64() would spin forever in its fetch/retry loop, or
return torn values. The process context caller also runs with preemption
enabled.

This patch adds one more RX-side writer that follows the existing
pattern.

> +			goto next;
> +		}
>  
>  		/* Allocate a page pool page as the SKB data area so the
>  		 * kernel can recycle it efficiently after the packet is

[Severity: Medium]
This isn't a bug introduced by this patch, but bcmasp_rx_poll() has no
upper bound on desc->size and no range check on the descriptor address.
Should it have both?

	data = intf->rx_ring_cpu +
		(DESC_ADDR(desc->buf) - intf->rx_ring_dma);
    ...
	skb_put(skb, len);
	memcpy(skb->data, data, len);

The skb is built on a single page. Its tailroom is PAGE_SIZE minus
NET_SKB_PAD minus skb_shared_info, about 3.7 KB with 4K pages.

A desc->size larger than that would hit skb_over_panic(). An address
outside RING_BUFFER_SIZE would make memcpy() copy memory from outside the
RX ring into the skb.

umac_reset_and_init() caps well-behaved hardware at 0x800 through
UMC_FRM_LEN and UMC_RX_MAX_PKT_SZ. So this only matters for the kind of
misbehaving descriptors this patch guards against.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922221630.3864427-1-florian.fainelli%40broadcom.com