* [PATCH 2/2] dmaengine: pxa_dma: Annotate struct pxad_desc_sw with __counted_by [not found] <c8fc5563c9593c914fde41f0f7d1489a21b45a9a.1696676782.git.christophe.jaillet@wanadoo.fr> @ 2023-10-07 11:13 ` Christophe JAILLET 2023-10-08 21:05 ` Kees Cook 0 siblings, 1 reply; 2+ messages in thread From: Christophe JAILLET @ 2023-10-07 11:13 UTC (permalink / raw) To: keescook, gustavoars, Daniel Mack, Haojian Zhuang, Robert Jarzmik, Vinod Koul, Nathan Chancellor, Nick Desaulniers, Tom Rix Cc: linux-hardening, linux-kernel, kernel-janitors, Christophe JAILLET, linux-arm-kernel, dmaengine, llvm 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 ^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH 2/2] dmaengine: pxa_dma: Annotate struct pxad_desc_sw with __counted_by 2023-10-07 11:13 ` [PATCH 2/2] dmaengine: pxa_dma: Annotate struct pxad_desc_sw with __counted_by Christophe JAILLET @ 2023-10-08 21:05 ` Kees Cook 0 siblings, 0 replies; 2+ messages in thread From: Kees Cook @ 2023-10-08 21:05 UTC (permalink / raw) To: Christophe JAILLET Cc: gustavoars, Daniel Mack, Haojian Zhuang, Robert Jarzmik, Vinod Koul, Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-hardening, linux-kernel, kernel-janitors, linux-arm-kernel, dmaengine, llvm 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 ^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-10-08 21:05 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <c8fc5563c9593c914fde41f0f7d1489a21b45a9a.1696676782.git.christophe.jaillet@wanadoo.fr>
2023-10-07 11:13 ` [PATCH 2/2] dmaengine: pxa_dma: Annotate struct pxad_desc_sw with __counted_by Christophe JAILLET
2023-10-08 21:05 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox