Re: [PATCH net-next v4 06/13] net: ethernet: oa_tc6: Add the OA_TC6_ prefix to standard registers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing SKB check when receiving an end-valid or ongoing chunk without a prior start-valid chunk causes a NULL pointer dereference.
- [High] Unbounded SKB tail extension allows a malicious or faulty MAC-PHY to trigger `skb_over_panic()`.
--

commit f42a7b7b1c5ba6bcf876685d2831cb4138c5ac3a
Author: Ciprian Regus <ciprian.regus@analog.com>

net: ethernet: oa_tc6: Add the OA_TC6_ prefix to standard registers

The OA TC6 standard registers are currently exported in a header file.
Add the OA_TC6_ prefix to the register address and subfield mask macros
to avoid future naming conflicts.

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 97df38207827d..92da5bb74cc76 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c

[ ... ]

> @@ -687,25 +687,25 @@ static int oa_tc6_process_extended_status(struct oa_tc6 *tc6)
>  		return ret;
>  	}
>  
> -	if (FIELD_GET(STATUS0_RX_BUFFER_OVERFLOW_ERROR, value)) {
> +	if (FIELD_GET(OA_TC6_STATUS0_RX_BUFFER_OVERFLOW_ERROR, value)) {
>  		tc6->rx_buf_overflow = true;
>  		oa_tc6_cleanup_ongoing_rx_skb(tc6);
>  		net_err_ratelimited("%s: Receive buffer overflow error\n",
>  				    tc6->netdev->name);
>  		return -EAGAIN;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the driver's
handling of rx_skb, I noticed a potential NULL pointer dereference in
oa_tc6_prcs_rx_chunk_payload().

If the function processes a chunk with end_valid set but start_valid clear:

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;
    }
    ...
}

Both this branch and the fallback to oa_tc6_prcs_ongoing_rx_frame()
eventually call oa_tc6_update_rx_skb(), which executes:

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

If tc6->rx_skb is NULL (which happens before any start_valid chunk is
processed), will skb_put() dereference a NULL pointer and cause a panic?

[Severity: High]
This is also a pre-existing issue, but I noticed another concern with
skb_put() in oa_tc6_prcs_ongoing_rx_frame().

When chunks arrive with both start_valid and end_valid cleared,
oa_tc6_prcs_rx_chunk_payload() calls oa_tc6_prcs_ongoing_rx_frame(), which
appends data without any bounds checking on the total accumulated length:

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

If a faulty MAC-PHY or compromised bus sends an unbounded sequence of ongoing
chunks without an end_valid signal, does this exhaust the SKB's tailroom and
trigger an skb_over_panic()?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-adin1140-driver-v4-0-0753e28ee004@analog.com?part=6