Re: [PATCH net v5 3/4] net: ethernet: oa_tc6: Remove FCS size in RX frame

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing check for `tc6->rx_skb` leads to a NULL pointer dereference when processing orphaned ongoing or end chunks.
- [High] Missing size bounds check for received chunks allows the MAC-PHY to cause a kernel panic via `skb_over_panic()`.
- [High] Unconditional SKB allocation on RX frame start overwrites existing SKBs.
--

commit 47aca7211ccbe61f3e2557dd3549950a622fcf9d
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

net: ethernet: oa_tc6: Remove FCS size in RX frame

OA TC6 MAC-PHY appends FCS to the incoming frame. It must be
removed from the frame before being passed to the stack.

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 477ceefde2c54..0727d53345a33 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c
> @@ -785,6 +785,17 @@ static int oa_tc6_process_rx_chunk_footer(struct oa_tc6 *tc6, u32 footer)
>  
>  static void oa_tc6_submit_rx_skb(struct oa_tc6 *tc6)
>  {
> +	/* MAC-PHY delivers each frame with its Ethernet FCS attached.
> +	 * Strip it before handing over to the stack, unless the user
> +	 * has asked to keep it via NETIF_F_RXFCS. Keeping the FCS
> +	 * in the frame is harmless for IP traffic, but is parsed as
> +	 * a (malformed) suffix TLV by PTP, which makes ptp4l reject
> +	 * every message with "bad message" error.
> +	 */
> +	if (!(tc6->netdev->features & NETIF_F_RXFCS) &&
> +	    tc6->rx_skb->len > ETH_FCS_LEN)
> +		skb_trim(tc6->rx_skb, tc6->rx_skb->len - ETH_FCS_LEN);
> +
>  	tc6->rx_skb->protocol = eth_type_trans(tc6->rx_skb, tc6->netdev);
>  	tc6->netdev->stats.rx_packets++;
>  	tc6->netdev->stats.rx_bytes += tc6->rx_skb->len;

While reviewing this patch, I noticed a few pre-existing issues in the
same file:

drivers/net/ethernet/oa_tc6.c:oa_tc6_prcs_rx_chunk_payload() {
...
	/* Process the chunk with only rx frame end */
	if (end_valid && !start_valid) {
		size = end_byte_offset + 1;
		oa_tc6_prcs_rx_frame_end(tc6, data, size);
		return 0;
	}
...
	/* Process the chunk with ongoing rx frame data */
	oa_tc6_prcs_ongoing_rx_frame(tc6, data, footer);
}

[Severity: High]
This isn't a bug introduced by this patch, but does this lead to a NULL
pointer dereference?

If a MAC-PHY sends an ongoing chunk or an end chunk without a preceding
start chunk, oa_tc6_prcs_rx_frame_end() or oa_tc6_prcs_ongoing_rx_frame()
will be called.

Both functions unconditionally call oa_tc6_update_rx_skb(), which blindly
calls skb_put(tc6->rx_skb, length). If tc6->rx_skb is NULL because no
start_valid was received, skb_put() will crash the system.


drivers/net/ethernet/oa_tc6.c:oa_tc6_prcs_ongoing_rx_frame() {
	oa_tc6_update_rx_skb(tc6, payload, OA_TC6_CHUNK_PAYLOAD_SIZE);
}

drivers/net/ethernet/oa_tc6.c:oa_tc6_update_rx_skb() {
	memcpy(skb_put(tc6->rx_skb, length), payload, length);
}

[Severity: High]
This isn't a bug introduced by this patch, but can this cause a kernel
panic via skb_over_panic()?

The driver allocates the SKB with a fixed tailroom
(MTU + ETH_HLEN + ETH_FCS_LEN). There is no bounds check to ensure the
accumulated length from ongoing chunks does not exceed the allocated
tailroom.

An endless stream of chunks without an end_valid bit might cause skb_put()
to exceed the tailroom and trigger a bounds check panic.


drivers/net/ethernet/oa_tc6.c:oa_tc6_prcs_rx_chunk_payload() {
...
	/* Process the chunk with only rx frame start */
	if (start_valid && !end_valid) {
		size = OA_TC6_CHUNK_PAYLOAD_SIZE - start_byte_offset;
		return oa_tc6_prcs_rx_frame_start(tc6,
						  &data[start_byte_offset],
						  size);
	}
...
}

drivers/net/ethernet/oa_tc6.c:oa_tc6_allocate_rx_skb() {
	tc6->rx_skb = netdev_alloc_skb_ip_align(tc6->netdev, tc6->netdev->mtu +
						ETH_HLEN + ETH_FCS_LEN);
...
}

[Severity: High]
This isn't a bug introduced by this patch, but could this unconditional
allocation overwrite and leak existing SKBs?

When an RX chunk with start_valid=1 is received, oa_tc6_allocate_rx_skb()
assigns a new SKB to tc6->rx_skb.

If the MAC-PHY sends consecutive chunks with start_valid=1 (or leaves a
previous frame incomplete), the prior tc6->rx_skb pointer appears to be
overwritten and permanently leaked.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611-level-trigger-v5-0-4533a9e85ce2@onsemi.com?part=3