[PATCH 1/2] dmaengine: pxa_dma: Remove an erroneous BUG_ON() in pxad_free_desc()

[PATCH 1/2] dmaengine: pxa_dma: Remove an erroneous BUG_ON() in pxad_free_desc()

[Christophe JAILLET]

If pxad_alloc_desc() fails on the first dma_pool_alloc() call, then
sw_desc->nb_desc is zero.
In such a case pxad_free_desc() is called and it will BUG_ON().

Remove this erroneous BUG_ON().

It is also useless, because if "sw_desc->nb_desc == 0", then, on the first
iteration of the for loop, i is -1 and the loop will not be executed.
(both i and sw_desc->nb_desc are 'int')

Fixes: a57e16cf0333 ("dmaengine: pxa: add pxa dmaengine driver")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/dma/pxa_dma.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
index 3c574dc0613b..94cef2905940 100644
--- a/drivers/dma/pxa_dma.c
+++ b/drivers/dma/pxa_dma.c
@@ -722,7 +722,6 @@ static void pxad_free_desc(struct virt_dma_desc *vd)
 	dma_addr_t dma;
 	struct pxad_desc_sw *sw_desc = to_pxad_sw_desc(vd);
 
-	BUG_ON(sw_desc->nb_desc == 0);
 	for (i = sw_desc->nb_desc - 1; i >= 0; i--) {
 		if (i > 0)
 			dma = sw_desc->hw_desc[i - 1]->ddadr;
-- 
2.34.1

[PATCH 2/2] dmaengine: pxa_dma: Annotate struct pxad_desc_sw with __counted_by

[Christophe JAILLET]

Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).

To do so, the code needs a little shuffling related to how hw_desc is used
and nb_desc incremented.

The one by one increment is needed for the error handling path, calling
pxad_free_desc(), to work correctly.

So, add a new intermediate variable, desc, to store the result of the
dma_pool_alloc() call.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch is part of a work done in parallel of what is currently worked
on by Kees Cook.

My patches are only related to corner cases that do NOT match the
semantic of his Coccinelle script[1].

[1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
---
 drivers/dma/pxa_dma.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
index 94cef2905940..c6e2862896e3 100644
--- a/drivers/dma/pxa_dma.c
+++ b/drivers/dma/pxa_dma.c
@@ -91,7 +91,8 @@ struct pxad_desc_sw {
 	bool			cyclic;
 	struct dma_pool		*desc_pool;	/* Channel's used allocator */
 
-	struct pxad_desc_hw	*hw_desc[];	/* DMA coherent descriptors */
+	struct pxad_desc_hw	*hw_desc[] __counted_by(nb_desc);
+						/* DMA coherent descriptors */
 };
 
 struct pxad_phy {
@@ -739,6 +740,7 @@ pxad_alloc_desc(struct pxad_chan *chan, unsigned int nb_hw_desc)
 {
 	struct pxad_desc_sw *sw_desc;
 	dma_addr_t dma;
+	void *desc;
 	int i;
 
 	sw_desc = kzalloc(struct_size(sw_desc, hw_desc, nb_hw_desc),
@@ -748,20 +750,21 @@ pxad_alloc_desc(struct pxad_chan *chan, unsigned int nb_hw_desc)
 	sw_desc->desc_pool = chan->desc_pool;
 
 	for (i = 0; i < nb_hw_desc; i++) {
-		sw_desc->hw_desc[i] = dma_pool_alloc(sw_desc->desc_pool,
-						     GFP_NOWAIT, &dma);
-		if (!sw_desc->hw_desc[i]) {
+		desc = dma_pool_alloc(sw_desc->desc_pool, GFP_NOWAIT, &dma);
+		if (!desc) {
 			dev_err(&chan->vc.chan.dev->device,
 				"%s(): Couldn't allocate the %dth hw_desc from dma_pool %p\n",
 				__func__, i, sw_desc->desc_pool);
 			goto err;
 		}
 
+		sw_desc->nb_desc++;
+		sw_desc->hw_desc[i] = desc;
+
 		if (i == 0)
 			sw_desc->first = dma;
 		else
 			sw_desc->hw_desc[i - 1]->ddadr = dma;
-		sw_desc->nb_desc++;
 	}
 
 	return sw_desc;
-- 
2.34.1

Re: [PATCH 2/2] dmaengine: pxa_dma: Annotate struct pxad_desc_sw with __counted_by

[Kees Cook]

On Sat, Oct 07, 2023 at 01:13:10PM +0200, Christophe JAILLET wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
> 
> To do so, the code needs a little shuffling related to how hw_desc is used
> and nb_desc incremented.
> 
> The one by one increment is needed for the error handling path, calling
> pxad_free_desc(), to work correctly.
> 
> So, add a new intermediate variable, desc, to store the result of the
> dma_pool_alloc() call.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Thanks! Yeah, this looks like a sensible refactor to handle the
increment before array assignment without losing error checking.

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
> This patch is part of a work done in parallel of what is currently worked
> on by Kees Cook.
> 
> My patches are only related to corner cases that do NOT match the
> semantic of his Coccinelle script[1].
> 
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
> ---
>  drivers/dma/pxa_dma.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
> index 94cef2905940..c6e2862896e3 100644
> --- a/drivers/dma/pxa_dma.c
> +++ b/drivers/dma/pxa_dma.c
> @@ -91,7 +91,8 @@ struct pxad_desc_sw {
>  	bool			cyclic;
>  	struct dma_pool		*desc_pool;	/* Channel's used allocator */
>  
> -	struct pxad_desc_hw	*hw_desc[];	/* DMA coherent descriptors */
> +	struct pxad_desc_hw	*hw_desc[] __counted_by(nb_desc);
> +						/* DMA coherent descriptors */
>  };
>  
>  struct pxad_phy {
> @@ -739,6 +740,7 @@ pxad_alloc_desc(struct pxad_chan *chan, unsigned int nb_hw_desc)
>  {
>  	struct pxad_desc_sw *sw_desc;
>  	dma_addr_t dma;
> +	void *desc;
>  	int i;
>  
>  	sw_desc = kzalloc(struct_size(sw_desc, hw_desc, nb_hw_desc),
> @@ -748,20 +750,21 @@ pxad_alloc_desc(struct pxad_chan *chan, unsigned int nb_hw_desc)
>  	sw_desc->desc_pool = chan->desc_pool;
>  
>  	for (i = 0; i < nb_hw_desc; i++) {
> -		sw_desc->hw_desc[i] = dma_pool_alloc(sw_desc->desc_pool,
> -						     GFP_NOWAIT, &dma);
> -		if (!sw_desc->hw_desc[i]) {
> +		desc = dma_pool_alloc(sw_desc->desc_pool, GFP_NOWAIT, &dma);
> +		if (!desc) {
>  			dev_err(&chan->vc.chan.dev->device,
>  				"%s(): Couldn't allocate the %dth hw_desc from dma_pool %p\n",
>  				__func__, i, sw_desc->desc_pool);
>  			goto err;
>  		}
>  
> +		sw_desc->nb_desc++;
> +		sw_desc->hw_desc[i] = desc;
> +
>  		if (i == 0)
>  			sw_desc->first = dma;
>  		else
>  			sw_desc->hw_desc[i - 1]->ddadr = dma;
> -		sw_desc->nb_desc++;
>  	}
>  
>  	return sw_desc;
> -- 
> 2.34.1
> 

-- 
Kees Cook

Re: [PATCH 1/2] dmaengine: pxa_dma: Remove an erroneous BUG_ON() in pxad_free_desc()

[Kees Cook]

On Sat, Oct 07, 2023 at 01:13:09PM +0200, Christophe JAILLET wrote:
> If pxad_alloc_desc() fails on the first dma_pool_alloc() call, then
> sw_desc->nb_desc is zero.
> In such a case pxad_free_desc() is called and it will BUG_ON().
> 
> Remove this erroneous BUG_ON().

Perhaps it could be changed to a WARN_ON()?

> It is also useless, because if "sw_desc->nb_desc == 0", then, on the first
> iteration of the for loop, i is -1 and the loop will not be executed.
> (both i and sw_desc->nb_desc are 'int')

Agreed.

> 
> Fixes: a57e16cf0333 ("dmaengine: pxa: add pxa dmaengine driver")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/dma/pxa_dma.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
> index 3c574dc0613b..94cef2905940 100644
> --- a/drivers/dma/pxa_dma.c
> +++ b/drivers/dma/pxa_dma.c
> @@ -722,7 +722,6 @@ static void pxad_free_desc(struct virt_dma_desc *vd)
>  	dma_addr_t dma;
>  	struct pxad_desc_sw *sw_desc = to_pxad_sw_desc(vd);
>  
> -	BUG_ON(sw_desc->nb_desc == 0);
>  	for (i = sw_desc->nb_desc - 1; i >= 0; i--) {
>  		if (i > 0)
>  			dma = sw_desc->hw_desc[i - 1]->ddadr;
> -- 
> 2.34.1
> 

-- 
Kees Cook

Re: [PATCH 1/2] dmaengine: pxa_dma: Remove an erroneous BUG_ON() in pxad_free_desc()

[Vinod Koul]

On Sat, 07 Oct 2023 13:13:09 +0200, Christophe JAILLET wrote:
> If pxad_alloc_desc() fails on the first dma_pool_alloc() call, then
> sw_desc->nb_desc is zero.
> In such a case pxad_free_desc() is called and it will BUG_ON().
> 
> Remove this erroneous BUG_ON().
> 
> It is also useless, because if "sw_desc->nb_desc == 0", then, on the first
> iteration of the for loop, i is -1 and the loop will not be executed.
> (both i and sw_desc->nb_desc are 'int')
> 
> [...]

Applied, thanks!

[1/2] dmaengine: pxa_dma: Remove an erroneous BUG_ON() in pxad_free_desc()
      commit: 83c761f568733277ce1f7eb9dc9e890649c29a8c
[2/2] dmaengine: pxa_dma: Annotate struct pxad_desc_sw with __counted_by
      commit: 0481291f0ccbc5147635cf0eb108f9fe5a05ee7d

Best regards,
-- 
~Vinod