Skip to contentSkip to conversation

dmaengine: dw-axi-dmac: Do not dereference NULL structure

PATCH Applied Kees Cook 3 participants 5 messages Jan 27, 2023 – Feb 10, 2023

Subsystems (2)
dmaengine Synopsys DesignWare AXI DMAC driver
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 dmaengine: dw-axi-dmac: Do not dereference NULL structure

If "vdesc" is NULL, it cannot be used with vd_to_axi_desc(). Leave "bytes" unchanged at 0. Seen under GCC 13 with -Warray-bounds:

../drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c: In function 'dma_chan_tx_status':
../drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:329:46: warning: array subscript 0 is outside array bounds of 'struct virt_dma_desc[46116860184273879]' [-Warray-bounds=]
329 | bytes = vd_to_axi_desc(vdesc)->length;
| ^~

Fixes 8e55444da65c ("dmaengine: dw-axi-dmac: Support burst residue granularity")
Cc Eugeniy Paltsev <[email protected]>
Cc Vinod Koul <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
 drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c | 2 --
 1 file changed, 2 deletions(-)
drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c +0 -2
index 08af483331fd..4169e1d7d5ca 100644
@@ -325,8 +325,6 @@ dma_chan_tx_status(struct dma_chan *dchan, dma_cookie_t cookie,
 		len = vd_to_axi_desc(vdesc)->hw_desc[0].len;
 		completed_length = completed_blocks * len;
 		bytes = length - completed_length;
-	} else {
-		bytes = vd_to_axi_desc(vdesc)->length;
 	}
 
 	spin_unlock_irqrestore(&chan->vc.lock, flags);
Signature · 1 line
2.34.1
#1 lore raw
Quoted 23 lines · Kees Cook

If "vdesc" is NULL, it cannot be used with vd_to_axi_desc(). Leave "bytes" unchanged at 0. Seen under GCC 13 with -Warray-bounds:

../drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c: In function 'dma_chan_tx_status':
../drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:329:46: warning: array subscript 0 is outside array bounds of 'struct virt_dma_desc[46116860184273879]' [-Warray-bounds=]
329 | bytes = vd_to_axi_desc(vdesc)->length;
| ^~

Fixes 8e55444da65c ("dmaengine: dw-axi-dmac: Support burst residue granularity")
Cc Eugeniy Paltsev <[email protected]>
Cc Vinod Koul <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
 drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c | 2 --
 1 file changed, 2 deletions(-)
drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c +0 -0
index 08af483331fd..4169e1d7d5ca 100644

drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c dma_chan_tx_status(struct dma_chan *dchan, dma_cookie_t cookie, · line 330

Quoted 8 lines
drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c +0 -2
@@ -325,8 +325,6 @@ dma_chan_tx_status(struct dma_chan *dchan, dma_cookie_t cookie,
 		len = vd_to_axi_desc(vdesc)->hw_desc[0].len;
 		completed_length = completed_blocks * len;
 		bytes = length - completed_length;
-	} else {
-		bytes = vd_to_axi_desc(vdesc)->length;
 	}
 
 	spin_unlock_irqrestore(&chan->vc.lock, flags);

If you want to fix it properly, the code should be modified like

	status = dma_cookie_status(dchan, cookie, txstate);
	if (status == DMA_COMPLETE || !txstate) {
		if (txstate)
			goto out;
		return status;
	}

	...

out:

	dma_set_residue(txstate, bytes);

	return status;

to be in accordance with the Documentation.

**OR**

the Documentation should be fixed to tell that if status is DMA_COMPLETE, residue is undefined and assumed to be 0.

Vinod?

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#2 lore raw
Quoted 1 line · Andy Shevchenko

On Fri, Jan 27, 2023 at 02:36:27PM -0800, Kees Cook wrote:

...

Quoted 16 lines

If you want to fix it properly, the code should be modified like

	status = dma_cookie_status(dchan, cookie, txstate);
	if (status == DMA_COMPLETE || !txstate) {
		if (txstate)
			goto out;
		return status;
	}

	...

out:

	dma_set_residue(txstate, bytes);

	return status;
Quoted 8 lines

to be in accordance with the Documentation.

**OR**

the Documentation should be fixed to tell that if status is DMA_COMPLETE, residue is undefined and assumed to be 0.

Vinod?

Disregard my message. The dma_cookie_status() makes sure it's 0. Sorry for the noise.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
Reviewed-by on v1 1/1
#3 lore raw
In reply to Kees Cook
Quoted 8 lines · Kees Cook

If "vdesc" is NULL, it cannot be used with vd_to_axi_desc(). Leave "bytes" unchanged at 0. Seen under GCC 13 with -Warray-bounds:

../drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c: In function 'dma_chan_tx_status':
../drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:329:46: warning: array subscript 0 is outside array bounds of 'struct virt_dma_desc[46116860184273879]' [-Warray-bounds=]
329 | bytes = vd_to_axi_desc(vdesc)->length;
| ^~

Reviewed-by Andy Shevchenko <[email protected]>
Quoted 25 lines
Fixes 8e55444da65c ("dmaengine: dw-axi-dmac: Support burst residue granularity")
Cc Eugeniy Paltsev <[email protected]>
Cc Vinod Koul <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
 drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c | 2 --
 1 file changed, 2 deletions(-)
drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c +0 -2
index 08af483331fd..4169e1d7d5ca 100644
@@ -325,8 +325,6 @@ dma_chan_tx_status(struct dma_chan *dchan, dma_cookie_t cookie,
 		len = vd_to_axi_desc(vdesc)->hw_desc[0].len;
 		completed_length = completed_blocks * len;
 		bytes = length - completed_length;
-	} else {
-		bytes = vd_to_axi_desc(vdesc)->length;
 	}
 
 	spin_unlock_irqrestore(&chan->vc.lock, flags);
Signature · 1 line
2.34.1
Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer Applied
#4 lore raw
In reply to Kees Cook
Quoted 9 lines · Kees Cook

If "vdesc" is NULL, it cannot be used with vd_to_axi_desc(). Leave "bytes" unchanged at 0. Seen under GCC 13 with -Warray-bounds:

../drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c: In function 'dma_chan_tx_status':
../drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c:329:46: warning: array subscript 0 is outside array bounds of 'struct virt_dma_desc[46116860184273879]' [-Warray-bounds=]
329 | bytes = vd_to_axi_desc(vdesc)->length;
| ^~

Applied, thanks

Signature · 1 line
~Vinod