linux-kernel - Re: [PATCH 2/2] dmaengine: pxa_dma: Annotate struct pxad_desc_sw with __counted

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202310081404.382AE20@keescook>
Date:   Sun, 8 Oct 2023 14:05:10 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     gustavoars@...nel.org, Daniel Mack <daniel@...que.org>,
        Haojian Zhuang <haojian.zhuang@...il.com>,
        Robert Jarzmik <robert.jarzmik@...e.fr>,
        Vinod Koul <vkoul@...nel.org>,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Tom Rix <trix@...hat.com>, linux-hardening@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, dmaengine@...r.kernel.org,
        llvm@...ts.linux.dev
Subject: Re: [PATCH 2/2] dmaengine: pxa_dma: Annotate struct pxad_desc_sw
 with __counted_by

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@...adoo.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@...omium.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