From:   Lukasz Stelmach <l.stelmach@samsung.com>
To:     Nicolas Iooss <nicolas.iooss_linux@m4x.org>
Cc:     "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] net: ax88796c: do not receive data in pointer
Date:   Mon, 22 Nov 2021 14:52:27 +0100
In-Reply-To: <20211121200642.2083316-1-nicolas.iooss_linux@m4x.org> (Nicolas
        Iooss's message of "Sun, 21 Nov 2021 21:06:42 +0100")
Message-ID: <dleftjk0h0e1r8.fsf%l.stelmach@samsung.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-="; micalg="pgp-sha256";
        protocol="application/pgp-signature"
CMS-TYPE: 201P
References: <20211121200642.2083316-1-nicolas.iooss_linux@m4x.org>
        <CGME20211122135238eucas1p1e46674d13b4f6f504558f4102cfe3a85@eucas1p1.samsung.com>
Precedence: bulk

--=-=-=
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

It was <2021-11-21 nie 21:06>, when Nicolas Iooss wrote:
> Function axspi_read_status calls:
>
>     ret =3D 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 D=
river")
>
> 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: =C5=81ukasz Stelmach <l.stelmach@samsung.com>

> diff --git a/drivers/net/ethernet/asix/ax88796c_spi.c b/drivers/net/ether=
net/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)
>=20=20
>  	/* OP */
>  	ax_spi->cmd_buf[0] =3D AX_SPICMD_READ_STATUS;
> -	ret =3D spi_write_then_read(ax_spi->spi, ax_spi->cmd_buf, 1, (u8 *)&sta=
tus, 3);
> +	ret =3D spi_write_then_read(ax_spi->spi, ax_spi->cmd_buf, 1, (u8 *)stat=
us, 3);
>  	if (ret)
>  		dev_err(&ax_spi->spi->dev, "%s() failed: ret =3D %d\n", __func__, ret);
>  	else

=2D-=20
=C5=81ukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEXpuyqjq9kGEVr9UQsK4enJilgBAFAmGboJsACgkQsK4enJil
gBBbAQf+LGRklhU4VJHDmVpF8P0R/Dqjs9+NNBmydtCQTkcJ6uuHwvE1igEJL/Ap
zQxWk143mdmbvhWThnVah83o0RH/0qArJ78iAFNyDikon+wtEqiiEWLuj/UBTplS
AwqvGYhSrkwrzErdMxSzH6M6cCUl9zUThlmExGwqUHGfsatecbo26uV2cBe9cPhs
5QoDNGjuhs2RTCOGpcYL9xL7xyqe6xh+AwAsUsGTPn+dnn13z95Ih+IxbrhYXrga
zYvjRPTmZZDKUDsQs6xdvHSw3Qgt0u8GoB7ZyDDGgBeh5zLy6zev5VPOyPp12kRE
xJjS1XqOPFZcPFJsSIHIkGx9X6BbOQ==
=u3wQ
-----END PGP SIGNATURE-----
--=-=-=--