Re: [PATCH net-next v5 07/15] net: ethernet: oa_tc6: Support for vendor specific MMS

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing state validation in MAC-PHY RX chunk processing leads to a NULL pointer dereference if a start-of-frame chunk is missed.
- [High] The RX path does not limit the accumulated length of ongoing chunks, allowing a malicious hardware device to trigger an `skb_over_panic`.
- [High] Repeated start-of-frame chunks without corresponding end-of-frame chunks will leak newly allocated socket buffers.
--

commit ad9ba89ed1440b885945a45d429d49b23e68bbdf
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

net: ethernet: oa_tc6: Support for vendor specific MMS

OPEN Alliance 10BASE-T1x Serial Interface specification, table 6
allows vendors to use any memory map select (MMS) value between
10 and 15. This new API interface enables vendor to map one of
thes MMS values to MDIO_MMD_VEND1.

> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6.c b/drivers/net/ethernet/oa_tc6/oa_tc6.c
> index 9410cecfdc2aa..fab7cb84df712 100644
> --- a/drivers/net/ethernet/oa_tc6/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6.c
> @@ -202,6 +202,18 @@ int oa_tc6_ioctl(struct oa_tc6 *tc6, struct ifreq *rq, int cmd)
>  }
>  EXPORT_SYMBOL_GPL(oa_tc6_ioctl);
>  
> +/**
> + * oa_tc6_set_vend1_mms - Add vendor specific MDIO_MMD to OA TC6 MMS
> + * mapper value.
> + * @tc6: oa_tc6 struct.
> + * @mms: vendor defined MMS value for VEND1 mdio device.
> + */
> +void oa_tc6_set_vend1_mms(struct oa_tc6 *tc6, int mms)
> +{
> +	tc6->vend1_mms = mms;
> +}
> +EXPORT_SYMBOL_GPL(oa_tc6_set_vend1_mms);
> +
>  static __be32 oa_tc6_prepare_ctrl_header(u32 addr, u8 length,

[Severity: High]
This is a pre-existing issue, but I noticed a potential NULL pointer
dereference during RX chunk processing. If the MAC-PHY sends an RX chunk with
end_valid set and start_valid clear as the first chunk:

drivers/net/ethernet/oa_tc6/oa_tc6.c:oa_tc6_prcs_rx_chunk_payload()
    if (end_valid && !start_valid) {
        size = end_byte_offset + 1;
        oa_tc6_prcs_rx_frame_end(tc6, data, size);
        return 0;
    }

This function unconditionally calls oa_tc6_update_rx_skb(), which invokes
skb_put() on tc6->rx_skb:

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

Since tc6->rx_skb is only allocated when a chunk with a valid start is
received, it will be NULL in this case. Can a malfunctioning or malicious
MAC-PHY trigger a kernel panic here?

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a missing
length check when accumulating ongoing RX chunks. If the MAC-PHY streams an
unbroken series of ongoing chunks (start_valid = 0 and end_valid = 0),
oa_tc6_prcs_ongoing_rx_frame() repeatedly appends data:

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);

Since the SKB allocated in oa_tc6_allocate_rx_skb() has a fixed capacity
based on the MTU, could a stream of ongoing chunks eventually exceed the
skb's tailroom and trigger an skb_over_panic()?

[Severity: High]
This is a pre-existing issue, but repeated start-of-frame chunks might leak
socket buffers. When oa_tc6_prcs_rx_chunk_payload() receives a new start
chunk without the previous frame ending, it allocates a new SKB:

drivers/net/ethernet/oa_tc6/oa_tc6.c:oa_tc6_prcs_rx_chunk_payload()
    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, footer);
    }

This eventually calls oa_tc6_allocate_rx_skb() which overwrites the
tc6->rx_skb pointer:

drivers/net/ethernet/oa_tc6/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 +
                                            OA_TC6_TSTAMP_SZ);

If tc6->rx_skb was already populated from a previously interrupted frame,
the old pointer is overwritten without being freed. Can this be used to
exhaust kernel memory by continuously sending start chunks?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260614-s2500-mac-phy-support-v5-0-89874b72f725@onsemi.com?part=7