Re: [PATCH net-next v4 08/16] net: ethernet: oa_tc6: Remove FCS size in RX frame

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing length validation before pulling headers from the received packet can lead to out-of-bounds reads and integer underflow on `skb->len`.
- [High] Dereferencing `skb->data` as a 32-bit integer causes unaligned memory accesses, leading to alignment faults on some architectures.

Pre-existing issues:
- [High] Missing bounds check on `skb_put()` during RX chunk processing allows a malfunctioning MAC-PHY to cause a kernel panic via `skb_over_panic()`.
- [High] A transient memory allocation failure permanently kills the driver's SPI worker thread, rendering the interface permanently unusable.
- [High] Processing intermediate or end-only RX chunks when no SKB is allocated causes a NULL pointer dereference.
--

commit 84d3033631dd42f209b8cf8f2057dce10291f539
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

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

This commit modifies the RX path to strip the Ethernet FCS from incoming
frames before passing them to the networking stack. The stripping is skipped
if the user has requested to keep the FCS via NETIF_F_RXFCS.

> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6.c b/drivers/net/ethernet/oa_tc6/oa_tc6.c
> index 2f9ace655e81c..26033373f16fc 100644
> --- a/drivers/net/ethernet/oa_tc6/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6.c
> @@ -782,12 +782,22 @@ static void oa_tc6_submit_rx_skb(struct oa_tc6 *tc6)
>  {
>  	oa_tc6_update_ts_in_rx_skb(tc6);

[Severity: High]
Can an undersized frame cause an integer underflow on skb->len?

Looking at oa_tc6_update_ts_in_rx_skb() in drivers/net/ethernet/oa_tc6/oa_tc6.c:

    ts[0] = be32_to_cpu(*((u32 *)(skb->data)));
    ts[1] = be32_to_cpu(*((u32 *)(skb->data) + 1));
    ...
    skb_pull(skb, sizeof(ts));

This sequence reads 8 bytes from skb->data and pulls them without verifying if
skb->len is at least 8. 

Later, eth_type_trans() will pull ETH_HLEN. If a small or corrupted frame is
received, could these pulls subtract past zero and wrap skb->len to a huge
value, leading to out-of-bounds accesses?

[Severity: High]
Is there a risk of unaligned memory access faults here?

In oa_tc6_update_ts_in_rx_skb():

    ts[0] = be32_to_cpu(*((u32 *)(skb->data)));

The receive SKB is typically allocated with netdev_alloc_skb_ip_align(), which
offsets skb->data by 2 bytes to align the IP header. Casting this unaligned
pointer to a 32-bit integer and dereferencing it can trigger alignment faults
on architectures lacking hardware support for unaligned accesses. 

Would using get_unaligned_be32(skb->data) prevent this issue?

> +	/* MAC-PHY delivers each frame with its Ethernet FCS attached.
[ ... ]
> +	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++;
[ ... ]

Looking at oa_tc6_update_rx_skb() in drivers/net/ethernet/oa_tc6/oa_tc6.c:

    memcpy(skb_put(tc6->rx_skb, length), payload, length);

[Severity: High]
This is a pre-existing issue, but does this skb_put() call need a bounds
check?

If a malfunctioning MAC-PHY sends an unexpectedly large frame or continuously
sends intermediate chunks (where start_valid and end_valid are false), the
accumulated length could exceed the allocated SKB buffer size. 

Will this trigger skb_over_panic() and crash the kernel?


Looking at the interaction between oa_tc6_allocate_rx_skb() and
oa_tc6_spi_thread_handler() in drivers/net/ethernet/oa_tc6/oa_tc6.c:

oa_tc6_allocate_rx_skb():
    if (!tc6->rx_skb) {
        tc6->netdev->stats.rx_dropped++;
        return -ENOMEM;
    }

oa_tc6_spi_thread_handler():
    ret = oa_tc6_try_spi_transfer(tc6);
    if (ret)
        return ret;

[Severity: High]
This is a pre-existing issue, but how does the driver recover if memory
allocation fails?

If oa_tc6_allocate_rx_skb() returns -ENOMEM, the error propagates through
oa_tc6_try_spi_transfer() to oa_tc6_spi_thread_handler(), which then exits
its loop and terminates the kthread. 

Since this thread handles all communication with the MAC-PHY, does a transient
out-of-memory condition cause a permanent denial of service for the interface
until the module is reloaded?


Looking at oa_tc6_prcs_ongoing_rx_frame() and oa_tc6_update_rx_skb() in
drivers/net/ethernet/oa_tc6/oa_tc6.c:

oa_tc6_prcs_ongoing_rx_frame():
    oa_tc6_update_rx_skb(tc6, payload, OA_TC6_CHUNK_PAYLOAD_SIZE);

oa_tc6_update_rx_skb():
    memcpy(skb_put(tc6->rx_skb, length), payload, length);

[Severity: High]
This is a pre-existing issue, but what happens if an intermediate or end-only
RX chunk arrives when no SKB is allocated?

If a chunk arrives with start_valid=false (for example, due to dropped
previous chunks or an SPI transport error) and tc6->rx_skb is NULL, the driver
unconditionally calls oa_tc6_update_rx_skb(). 

Could calling skb_put() on a NULL tc6->rx_skb result in an immediate kernel
panic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-s2500-mac-phy-support-v4-0-de0fbc13c6d8@onsemi.com?part=8