Re: [PATCH 1/1] net: ax88796c: do not receive data in pointer

[Lukasz Stelmach]

It was <2021-11-21 nie 21:06>, when Nicolas Iooss wrote:
> Function axspi_read_status calls:
>
>     ret = spi_write_then_read(ax_spi->spi, ax_spi->cmd_buf, 1,
>                               (u8 *)&status, 3);
>
> status is a pointer to a struct spi_status, which is 3-byte wide:
>
>     struct spi_status {
>         u16 isr;
>         u8 status;
>     };
>
> But &status is the pointer to this pointer, and spi_write_then_read does
> not dereference this parameter:
>
>     int spi_write_then_read(struct spi_device *spi,
>                             const void *txbuf, unsigned n_tx,
>                             void *rxbuf, unsigned n_rx)
>
> Therefore axspi_read_status currently receive a SPI response in the
> pointer status, which overwrites 24 bits of the pointer.
>
> Thankfully, on Little-Endian systems, the pointer is only used in
>
>     le16_to_cpus(&status->isr);
>
> ... which is a no-operation. So there, the overwritten pointer is not
> dereferenced. Nevertheless on Big-Endian systems, this can lead to
> dereferencing pointers after their 24 most significant bits were
> overwritten. And in all systems this leads to possible use of
> uninitialized value in functions calling spi_write_then_read which
> expect status to be initialized when the function returns.
>
> Moreover function axspi_read_status (and macro AX_READ_STATUS) do not
> seem to be used anywhere. So currently this seems to be dead code. Fix
> the issue anyway so that future code works properly when using function
> axspi_read_status.
>

Thank you for spotting and fixing this one.

> Fixes: a97c69ba4f30 ("net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver")
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@xxxxxxx>
> ---
>  drivers/net/ethernet/asix/ax88796c_spi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Acked-by: Łukasz Stelmach <l.stelmach@xxxxxxxxxxx>

> diff --git a/drivers/net/ethernet/asix/ax88796c_spi.c b/drivers/net/ethernet/asix/ax88796c_spi.c
> index 94df4f96d2be..0710e716d682 100644
> --- a/drivers/net/ethernet/asix/ax88796c_spi.c
> +++ b/drivers/net/ethernet/asix/ax88796c_spi.c
> @@ -34,7 +34,7 @@ int axspi_read_status(struct axspi_data *ax_spi, struct spi_status *status)
>  
>  	/* OP */
>  	ax_spi->cmd_buf[0] = AX_SPICMD_READ_STATUS;
> -	ret = spi_write_then_read(ax_spi->spi, ax_spi->cmd_buf, 1, (u8 *)&status, 3);
> +	ret = spi_write_then_read(ax_spi->spi, ax_spi->cmd_buf, 1, (u8 *)status, 3);
>  	if (ret)
>  		dev_err(&ax_spi->spi->dev, "%s() failed: ret = %d\n", __func__, ret);
>  	else

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

Attachment: signature.asc
Description: PGP signature