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 <[email protected]>
Thanks! Yeah, this looks like a sensible refactor to handle the increment before array assignment without losing error checking. Reviewed-by: Kees Cook <[email protected]> -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
