Re: [PATCH net-next v2 02/10] net: ethernet: oa_tc6: Handle the OA TC6 SPI protected mode

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `size` parameter in `oa_tc6_check_ctrl_write_reply` is typed as `u8`, causing silent truncation of the calculated SPI buffer size (which can exceed 1000 bytes). This can result in a massive out-of-bounds memory read in `memcmp` (leading to a kernel panic) or silently bypass the echoed data integrity check.
- [High] In `lan865x_remove`, `cancel_work_sync()` is incorrectly called before `unregister_netdev()`, allowing concurrent network operations to re-arm the work and cause a use-after-free.
--

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

net: ethernet: oa_tc6: Handle the OA TC6 SPI protected mode

This commit implements the OA TC6 standard defined protected mode for control
(register access) transactions. It adds 1's complement values of the data
field to the SPI transfer frames for both the host and the MACPHY to act as
an integrity check. The protected mode state is read from the CONFIG0 register
during MACPHY's reset.

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 91a906a7918a2..baba5aad84dfa 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c
[ ... ]
> @@ -283,10 +320,10 @@ static int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 address, u32 value[],
>  	u16 size;
>  	int ret;
>  
> -	/* Prepare control command and copy to SPI control buffer */
> -	oa_tc6_prepare_ctrl_spi_buf(tc6, address, value, length, reg_op);
> +	size = oa_tc6_calculate_ctrl_buf_size(length, tc6->prot_ctrl);
>  
> -	size = oa_tc6_calculate_ctrl_buf_size(length);
> +	/* Prepare control command and copy to SPI control buffer */
> +	oa_tc6_prepare_ctrl_spi_buf(tc6, address, value, length, reg_op, size);
>  
>  	/* Perform SPI transfer */
>  	ret = oa_tc6_spi_transfer(tc6, OA_TC6_CTRL_HEADER, size);
> @@ -301,7 +338,7 @@ static int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 address, u32 value[],
>  		return oa_tc6_check_ctrl_write_reply(tc6, size);

[Severity: High]
This isn't a bug introduced by this patch, but does this size parameter get
silently truncated?

The function oa_tc6_calculate_ctrl_buf_size() correctly computes the SPI
buffer size as a u16 (which can be up to 1032 bytes in protected mode).
However, oa_tc6_check_ctrl_write_reply() is defined as taking a u8 for
size:

static int oa_tc6_check_ctrl_write_reply(struct oa_tc6 *tc6, u8 size)
{
    ...
    if (memcmp(tx_buf, rx_buf, size - OA_TC6_CTRL_IGNORED_SIZE))
        return -EPROTO;
    ...
}

If the truncated size is less than 4 (for instance exactly 256, which
happens for a 31-register write in protected mode), could the expression
size - OA_TC6_CTRL_IGNORED_SIZE underflow to a negative integer?

Since memcmp expects a size_t, this would be promoted to an enormous
unsigned integer, leading to an immediate out-of-bounds read and a
kernel panic. If it truncates to a value >= 4 but smaller than the true
size, could it silently defeat the integrity check by only comparing a
small fraction of the buffer?

While this is a pre-existing issue, the introduction of protected mode
doubles the payload size, making a 256-byte overflow much easier to hit.


[Severity: High]
This is a pre-existing issue, but could the teardown sequence lead to a
use-after-free?

Looking at the driver teardown patterns, cancel_work_sync() is being
called before unregister_netdev(). For example, in lan865x_remove():

static void lan865x_remove(struct spi_device *spi)
{
    ...
    cancel_work_sync(&priv->multicast_work);
    unregister_netdev(priv->netdev);
    ...
}

Because the netdev is still active and registered when cancel_work_sync()
is called, could a concurrent userspace operation (such as modifying the
multicast list) invoke the ndo_set_rx_mode callback and re-schedule the
workqueue?

When unregister_netdev() subsequently proceeds and free_netdev() frees
the priv pointer, when the re-armed work eventually executes, won't it
access the freed priv and crash? Should unregister_netdev() be executed
before cancel_work_sync()?

Note that the newly introduced adin1140_remove() driver in this patch
series appears to copy this exact same flawed pattern with
cancel_work_sync(&priv->rx_mode_work), potentially introducing a new
use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527-adin1140-driver-v2-0-37e5c8d4e0a0@analog.com?part=2