LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/1] net: ax88796c: do not receive data in pointer
@ 2021-11-21 20:06 Nicolas Iooss
       [not found] ` <CGME20211122135238eucas1p1e46674d13b4f6f504558f4102cfe3a85@eucas1p1.samsung.com>
  2021-11-22 14:40 ` patchwork-bot+netdevbpf
  0 siblings, 2 replies; 3+ messages in thread
From: Nicolas Iooss @ 2021-11-21 20:06 UTC (permalink / raw)
  To: Łukasz Stelmach
  Cc: David S. Miller, Jakub Kicinski, netdev, linux-kernel, Nicolas Iooss

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.

Fixes: a97c69ba4f30 ("net: ax88796c: ASIX AX88796C SPI Ethernet Adapter Driver")

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---
 drivers/net/ethernet/asix/ax88796c_spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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
-- 
2.33.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH 1/1] net: ax88796c: do not receive data in pointer
       [not found] ` <CGME20211122135238eucas1p1e46674d13b4f6f504558f4102cfe3a85@eucas1p1.samsung.com>
@ 2021-11-22 13:52   ` Lukasz Stelmach
  0 siblings, 0 replies; 3+ messages in thread
From: Lukasz Stelmach @ 2021-11-22 13:52 UTC (permalink / raw)
  To: Nicolas Iooss; +Cc: David S. Miller, Jakub Kicinski, netdev, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2687 bytes --]

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

Acked-by: Łukasz Stelmach <l.stelmach@samsung.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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH 1/1] net: ax88796c: do not receive data in pointer
  2021-11-21 20:06 [PATCH 1/1] net: ax88796c: do not receive data in pointer Nicolas Iooss
       [not found] ` <CGME20211122135238eucas1p1e46674d13b4f6f504558f4102cfe3a85@eucas1p1.samsung.com>
@ 2021-11-22 14:40 ` patchwork-bot+netdevbpf
  1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-11-22 14:40 UTC (permalink / raw)
  To: Nicolas Iooss; +Cc: l.stelmach, davem, kuba, netdev, linux-kernel

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Sun, 21 Nov 2021 21:06:42 +0100 you 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:
> 
> [...]

Here is the summary with links:
  - [1/1] net: ax88796c: do not receive data in pointer
    https://git.kernel.org/netdev/net/c/f93fd0ca5e7d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-11-22 14:40 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-21 20:06 [PATCH 1/1] net: ax88796c: do not receive data in pointer Nicolas Iooss
     [not found] ` <CGME20211122135238eucas1p1e46674d13b4f6f504558f4102cfe3a85@eucas1p1.samsung.com>
2021-11-22 13:52   ` Lukasz Stelmach
2021-11-22 14:40 ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).