[PATCH] spi: bcm2835: fix NULL pointer deref for OOB transfers.

[PATCH] spi: bcm2835: fix NULL pointer deref for OOB transfers.

[Dani Sanz]

From: Dani Sanz <sbrk.modules@gmail.com>

There was a problem when calling ioctl(fd, SPI_IOC_ENABLE_OOB_MODE,
oob_spi_setup) to use spidev in OOB mode after setting up the spidev.

The kernel crashed because on function
bcm2835_spi_start_oob_transfer(struct spi_controller *ctlr, struct
spi_oob_transfer *xfer), it tried to access bs->slv->prepare_cs,
however bs->slv was NULL.

This patch sets the bs->slv value to the correct bcm2835_spidev object
during bcm2835_spi_setup(struct spi_device *spi), so the NULL pointer
dereference that happened later is avoided.

Signed-off-by: Dani Sanz <sbrk.modules@gmail.com>
---
 drivers/spi/spi-bcm2835.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 2626abfc0a5b..1051482df945 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -1244,6 +1244,8 @@ static int bcm2835_spi_setup(struct spi_device *spi)
 
 		spi_set_ctldata(spi, slv);
 
+		bs->slv = slv;
+
 		ret = bcm2835_spi_setup_dma(ctlr, spi, bs, slv);
 		if (ret)
 			goto err_cleanup;
-- 
2.25.1

Re: [PATCH] spi: bcm2835: fix NULL pointer deref for OOB transfers.

[Florian Bezdeka]

On Wed, 2023-04-12 at 19:09 +0200, Dani Sanz wrote:
> From: Dani Sanz <sbrk.modules@gmail.com>
> 
> There was a problem when calling ioctl(fd, SPI_IOC_ENABLE_OOB_MODE,
> oob_spi_setup) to use spidev in OOB mode after setting up the spidev.
> 
> The kernel crashed because on function
> bcm2835_spi_start_oob_transfer(struct spi_controller *ctlr, struct
> spi_oob_transfer *xfer), it tried to access bs->slv->prepare_cs,
> however bs->slv was NULL.
> 
> This patch sets the bs->slv value to the correct bcm2835_spidev object
> during bcm2835_spi_setup(struct spi_device *spi), so the NULL pointer
> dereference that happened later is avoided.
> 
> Signed-off-by: Dani Sanz <sbrk.modules@gmail.com>
> ---
>  drivers/spi/spi-bcm2835.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
> index 2626abfc0a5b..1051482df945 100644
> --- a/drivers/spi/spi-bcm2835.c
> +++ b/drivers/spi/spi-bcm2835.c

Hi!

Thanks for your patch, but his is the wrong mailing list for this
issue. The patch should be submitted upstream (Linux) and we will fetch
it during the next update cycle of Dovetail or IPIPE.

[1] should provide the necessary details on how to submit such a patch.

It's common practice to include the kernel stack trace in the commit
message so others can search for the same issue and will hopefully be
directed to your patch.

Best regards,
Florian

[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html

> @@ -1244,6 +1244,8 @@ static int bcm2835_spi_setup(struct spi_device *spi)
>  
>  		spi_set_ctldata(spi, slv);
>  
> +		bs->slv = slv;
> +
>  		ret = bcm2835_spi_setup_dma(ctlr, spi, bs, slv);
>  		if (ret)
>  			goto err_cleanup;

Re: [PATCH] spi: bcm2835: fix NULL pointer deref for OOB transfers.

[Philippe Gerum]

Florian Bezdeka <florian.bezdeka@siemens.com> writes:

> On Wed, 2023-04-12 at 19:09 +0200, Dani Sanz wrote:
>> From: Dani Sanz <sbrk.modules@gmail.com>
>> 
>> There was a problem when calling ioctl(fd, SPI_IOC_ENABLE_OOB_MODE,
>> oob_spi_setup) to use spidev in OOB mode after setting up the spidev.
>> 
>> The kernel crashed because on function
>> bcm2835_spi_start_oob_transfer(struct spi_controller *ctlr, struct
>> spi_oob_transfer *xfer), it tried to access bs->slv->prepare_cs,
>> however bs->slv was NULL.
>> 
>> This patch sets the bs->slv value to the correct bcm2835_spidev object
>> during bcm2835_spi_setup(struct spi_device *spi), so the NULL pointer
>> dereference that happened later is avoided.
>> 
>> Signed-off-by: Dani Sanz <sbrk.modules@gmail.com>
>> ---
>>  drivers/spi/spi-bcm2835.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
>> index 2626abfc0a5b..1051482df945 100644
>> --- a/drivers/spi/spi-bcm2835.c
>> +++ b/drivers/spi/spi-bcm2835.c
>
> Hi!
>
> Thanks for your patch, but his is the wrong mailing list for this
> issue. The patch should be submitted upstream (Linux) and we will fetch
> it during the next update cycle of Dovetail or IPIPE.
>

Actually, this patch applies specifically to Dovetail, upstream refers
to bs->slv only when TX via DMA is enabled, so there is no issue in this
case. However, Dovetail extends this assumption to any transfer
direction, but fails to init this field accordingly, hence the breakage.

-- 
Philippe.

Re: [PATCH] spi: bcm2835: fix NULL pointer deref for OOB transfers.

[Philippe Gerum]

Dani Sanz <sbrk.modules@gmail.com> writes:

> From: Dani Sanz <sbrk.modules@gmail.com>
>
> There was a problem when calling ioctl(fd, SPI_IOC_ENABLE_OOB_MODE,
> oob_spi_setup) to use spidev in OOB mode after setting up the spidev.
>
> The kernel crashed because on function
> bcm2835_spi_start_oob_transfer(struct spi_controller *ctlr, struct
> spi_oob_transfer *xfer), it tried to access bs->slv->prepare_cs,
> however bs->slv was NULL.
>
> This patch sets the bs->slv value to the correct bcm2835_spidev object
> during bcm2835_spi_setup(struct spi_device *spi), so the NULL pointer
> dereference that happened later is avoided.
>
> Signed-off-by: Dani Sanz <sbrk.modules@gmail.com>
> ---
>  drivers/spi/spi-bcm2835.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
> index 2626abfc0a5b..1051482df945 100644
> --- a/drivers/spi/spi-bcm2835.c
> +++ b/drivers/spi/spi-bcm2835.c
> @@ -1244,6 +1244,8 @@ static int bcm2835_spi_setup(struct spi_device *spi)
>  
>  		spi_set_ctldata(spi, slv);
>  
> +		bs->slv = slv;
> +
>  		ret = bcm2835_spi_setup_dma(ctlr, spi, bs, slv);
>  		if (ret)
>  			goto err_cleanup;

This bug has been there for quite some time it seems. Merged into
5.15.y, 6.1.y and 6.3, dovetail and evl trees. Thanks for looking into
this.

-- 
Philippe.