Re: [PATCH net-next v4 03/13] net: ethernet: oa_tc6: add OA_TC6_BROKEN_PHY quirk flag

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Heap buffer overflow in SPI TX/RX buffer preparation due to unvalidated chunk counts.
- [High] Unbounded `skb_put` calls in `oa_tc6_update_rx_skb` can cause a kernel panic due to missing frame length limits.
- [High] NULL pointer dereference in `oa_tc6_update_rx_skb` when receiving out-of-order chunks.
- [High] Use-After-Free of kthread task_struct due to premature exit on SPI error.
- [High] Use-After-Free in `lan865x_multicast_work_handler` due to incorrect driver teardown sequence.
- [High] Lost wakeup and device deadlock due to TOCTOU race on `tc6->int_flag`.
--

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

net: ethernet: oa_tc6: add OA_TC6_BROKEN_PHY quirk flag

Some MAC-PHY devices need custom MDIO bus access functions to work
around hardware issues. Add the OA_TC6_BROKEN_PHY quirk flag so drivers
can opt in to skip oa_tc6's internal PHY init and manage the PHY
themselves. When the flag is set, oa_tc6 skips MDIO bus registration,
PHY discovery and PHY connection, leaving these to the driver.

> diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c b/drivers/net/ethernet/microchip/lan865x/lan865x.c
> index 0277d9737369a..26a2761332a5a 100644
> --- a/drivers/net/ethernet/microchip/lan865x/lan865x.c
> +++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c
> @@ -346,7 +346,7 @@ static int lan865x_probe(struct spi_device *spi)
>  	spi_set_drvdata(spi, priv);
>  	INIT_WORK(&priv->multicast_work, lan865x_multicast_work_handler);

[Severity: High]
This isn't a bug introduced by this patch, but does the teardown sequence
in lan865x_remove() leave a window for a use-after-free?

In lan865x_remove():

    cancel_work_sync(&priv->multicast_work);
    unregister_netdev(priv->netdev);

If a concurrent userspace process calls ndo_set_rx_mode via the RTNL lock
after the work is cancelled but before the netdev is unregistered, could
it reschedule the multicast work? If the remove function then proceeds to
free priv, wouldn't the erroneously rescheduled work access the freed priv
pointer when it later executes?

> -	priv->tc6 = oa_tc6_init(spi, netdev);
> +	priv->tc6 = oa_tc6_init(spi, netdev, NULL);
>  	if (!priv->tc6) {
>  		ret = -ENODEV;
>  		goto free_netdev;

[ ... ]

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index baba5aad84dfa..2a72f0c4b0095 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c
> @@ -134,6 +134,7 @@ struct oa_tc6 {
>  	bool rx_buf_overflow;
>  	bool int_flag;

[Severity: High]
This isn't a bug introduced by this patch, but does the handling of
tc6->int_flag have a race condition that could lose wakeups?

In oa_tc6_try_spi_transfer(), the flag is checked and cleared non-atomically:

    if (tc6->int_flag) {
        tc6->int_flag = false;

If the hardware asserts the interrupt exactly between the check and the clear,
and the ISR sets tc6->int_flag = true, wouldn't the kthread immediately
overwrite it with false? Could this cause the kthread to miss the event flag
and hang indefinitely?

[Severity: High]
This isn't a bug introduced by this patch, but can an uninterrupted stream of
ongoing chunks with DATA_VALID=1 and END_VALID=0 continuously append up to 64
bytes to tc6->rx_skb without bounds checking?

Looking at oa_tc6_prcs_ongoing_rx_frame(), it calls:

    oa_tc6_update_rx_skb(tc6, payload, OA_TC6_CHUNK_PAYLOAD_SIZE);

This unconditionally calls skb_put():

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

If a device sends an endless stream of ongoing chunks, could this exhaust
the SKB tailroom and cause the core network stack to trigger a BUG() via
skb_over_panic()?

[Severity: High]
This isn't a bug introduced by this patch, but what happens if the MAC-PHY
sends an ongoing chunk (DATA_VALID=1, START_VALID=0, END_VALID=0) without a
preceding start chunk?

In oa_tc6_prcs_rx_chunk_payload():

    /* Process the chunk with ongoing rx frame data */
    oa_tc6_prcs_ongoing_rx_frame(tc6, data, footer);

If tc6->rx_skb is NULL because no start chunk was processed, wouldn't
oa_tc6_update_rx_skb() dereference a NULL pointer when calling skb_put(),
leading to an immediate crash?

>  	bool prot_ctrl;
> +	enum oa_tc6_quirk_flag quirk_flags;
>  };

[ ... ]

> @@ -1279,11 +1286,13 @@ static int oa_tc6_check_ctrl_protection(struct oa_tc6 *tc6)
>   * oa_tc6_init - allocates and initializes oa_tc6 structure.
>   * @spi: device with which data will be exchanged.
>   * @netdev: network device interface structure.
> + * @quirks: device specific modifiers for the OA TC6 protocol.
>   *
>   * Return: pointer reference to the oa_tc6 structure if the MAC-PHY
>   * initialization is successful otherwise NULL.
>   */
> -struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)
> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev,
> +			   struct oa_tc6_quirks *quirks)
>  {

[Severity: Critical]
This isn't a bug introduced by this patch, but is it possible for unvalidated
hardware registers to cause out-of-bounds writes in the SPI data buffers?

In oa_tc6_update_buffer_status_from_register():

    tc6->tx_credits = FIELD_GET(OA_TC6_BUFFER_STATUS_TX_CREDITS_AVAILABLE,
                                value);
    tc6->rx_chunks_available = FIELD_GET(OA_TC6_BUFFER_STATUS_RX_CHUNKS_AVAILABLE,
                                         value);

These are read directly from the hardware as 8-bit values (up to 255), but
the SPI data buffers (spi_data_tx_buf and spi_data_rx_buf) are allocated for
a maximum of 48 chunks (OA_TC6_MAX_TX_CHUNKS).

Later, in oa_tc6_prepare_spi_tx_buf_for_rx_chunks():

    needed_empty_chunks = tc6->rx_chunks_available - tx_chunks;
    oa_tc6_add_empty_chunks_to_spi_buf(tc6, needed_empty_chunks);

If a device reports unusually high values, wouldn't these loops write past
the end of the ~14KB buffer allocations?

[Severity: High]
This isn't a bug introduced by this patch, but could an early return from
oa_tc6_spi_thread_handler() lead to a use-after-free during teardown?

In oa_tc6_spi_thread_handler():

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

If an SPI error occurs, the kthread exits and its task_struct is freed.

Later, during device teardown, oa_tc6_exit() unconditionally calls:

    kthread_stop(tc6->spi_thread);

If the thread already exited, wouldn't calling kthread_stop() on the freed
task_struct cause a kernel crash?

>  	struct oa_tc6 *tc6;
>  	int ret;

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