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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <dleftjk0h0e1r8.fsf%l.stelmach@samsung.com>
Date:   Mon, 22 Nov 2021 14:52:27 +0100
From:   Lukasz Stelmach <l.stelmach@...sung.com>
To:     Nicolas Iooss <nicolas.iooss_linux@....org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] net: ax88796c: do not receive data in pointer

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@....org>
> ---
>  drivers/net/ethernet/asix/ax88796c_spi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

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

> 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

Download attachment "signature.asc" of type "application/pgp-signature" (488 bytes)