Message ID | 20200127122939.6952-1-gilad@benyossef.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Herbert Xu |
Headers | show |
Series | [RFC] crypto: ccree - protect against short scatterlists | expand |
Hi Gilad, On Mon, Jan 27, 2020 at 1:29 PM Gilad Ben-Yossef <gilad@benyossef.com> wrote: > Deal gracefully with the event of being handed a scatterlist > which is shorter than expected. > > This mitigates a crash in some cases of Crypto API calls due with > scatterlists with a NULL first buffer, despite the aead.h > forbidding doing so. > > Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Thanks for your patch! Unable to handle kernel paging request at virtual address fffeffffc0000000 Mem abort info: ESR = 0x96000144 EC = 0x25: DABT (current EL), IL = 32 bits SET = 0, FnV = 0 EA = 0, S1PTW = 0 Data abort info: ISV = 0, ISS = 0x00000144 CM = 1, WnR = 1 [fffeffffc0000000] address between user and kernel address ranges Internal error: Oops: 96000144 [#1] PREEMPT SMP CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.5.0-rc6-arm64-renesas-00813-g0ada3a94aab4dd18-dirty #520 Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT) pstate: 40000005 (nZcv daif -PAN -UAO) pc : __dma_inv_area+0x40/0x58 lr : arch_sync_dma_for_cpu+0x18/0x20 sp : ffff800008003cb0 x29: ffff800008003cb0 x28: ffff0006f89f0000 x27: ffff800008e44fa8 x26: 0000000000800000 x25: ffff0006f89f0000 x24: 0000000000000000 x23: 0000000000000000 x22: 0000000000000000 x21: 0000000000000000 x20: ffff0006fa648010 x19: 0000000000000000 x18: 0000000000000014 x17: 000000005111eab2 x16: 000000006cc48a27 x15: 41075e541c170702 x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000001 x11: 0000000000000002 x10: 0000000000000000 x9 : 0000000000000000 x8 : 0000000040000000 x7 : 0000000000000000 x6 : 0000000000000000 x5 : ffff80000825590c x4 : 0000000000000000 x3 : 000000000000003f x2 : 0000000000000040 x1 : fffeffffc0000000 x0 : fffeffffc0000000 Call trace: __dma_inv_area+0x40/0x58 dma_direct_sync_single_for_cpu+0x84/0x88 dma_direct_unmap_page+0x84/0x88 dma_direct_unmap_sg+0x54/0x80 cc_unmap_aead_request+0x160/0x408 cc_aead_complete+0x2c/0xf8 comp_handler+0x174/0x398 tasklet_action_common.isra.16+0xa8/0x190 tasklet_action+0x24/0x30 efi_header_end+0x110/0x560 irq_exit+0x13c/0x148 __handle_domain_irq+0x60/0xb0 gic_handle_irq+0x58/0xa8 el1_irq+0xbc/0x180 arch_cpu_idle+0x34/0x230 default_idle_call+0x1c/0x38 do_idle+0x1e0/0x2c0 cpu_startup_entry+0x24/0x28 rest_init+0x1a0/0x270 arch_call_rest_init+0xc/0x14 start_kernel+0x488/0x4b4 Code: 8a230000 54000060 d50b7e20 14000002 (d5087620) ---[ end trace 843cb2d928c7bf8b ]--- Kernel panic - not syncing: Fatal exception in interrupt SMP: stopping secondary CPUs Kernel Offset: disabled CPU features: 0x10002,21006004 Memory Limit: none ---[ end Kernel panic - not syncing: Fatal exception in interrupt ]--- Gr{oetje,eeting}s, Geert
On Mon, Jan 27, 2020 at 2:52 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Gilad, > > On Mon, Jan 27, 2020 at 1:29 PM Gilad Ben-Yossef <gilad@benyossef.com> wrote: > > Deal gracefully with the event of being handed a scatterlist > > which is shorter than expected. > > > > This mitigates a crash in some cases of Crypto API calls due with > > scatterlists with a NULL first buffer, despite the aead.h > > forbidding doing so. > > > > Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com> > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > Thanks for your patch! > > Unable to handle kernel paging request at virtual address fffeffffc0000000 OK, this is a progress of a sort. We now crash during unmap, not map. Sent another go. If this doesn't work I'll wait till I reunite with the board. Blind debugging is hard... Thanks again! Gilad values of β will give rise to dom!
diff --git a/drivers/crypto/ccree/cc_buffer_mgr.c b/drivers/crypto/ccree/cc_buffer_mgr.c index a72586eccd81..62a0dfb0b0b6 100644 --- a/drivers/crypto/ccree/cc_buffer_mgr.c +++ b/drivers/crypto/ccree/cc_buffer_mgr.c @@ -87,6 +87,11 @@ static unsigned int cc_get_sgl_nents(struct device *dev, { unsigned int nents = 0; + *lbytes = 0; + + if (!sg_list || !sg_list->length) + goto out; + while (nbytes && sg_list) { nents++; /* get the number of bytes in the last entry */ @@ -95,6 +100,8 @@ static unsigned int cc_get_sgl_nents(struct device *dev, nbytes : sg_list->length; sg_list = sg_next(sg_list); } + +out: dev_dbg(dev, "nents %d last bytes %d\n", nents, *lbytes); return nents; } @@ -290,37 +297,28 @@ static int cc_map_sg(struct device *dev, struct scatterlist *sg, unsigned int nbytes, int direction, u32 *nents, u32 max_sg_nents, u32 *lbytes, u32 *mapped_nents) { - if (sg_is_last(sg)) { - /* One entry only case -set to DLLI */ - if (dma_map_sg(dev, sg, 1, direction) != 1) { - dev_err(dev, "dma_map_sg() single buffer failed\n"); - return -ENOMEM; - } - dev_dbg(dev, "Mapped sg: dma_address=%pad page=%p addr=%pK offset=%u length=%u\n", - &sg_dma_address(sg), sg_page(sg), sg_virt(sg), - sg->offset, sg->length); - *lbytes = nbytes; - *nents = 1; - *mapped_nents = 1; - } else { /*sg_is_last*/ - *nents = cc_get_sgl_nents(dev, sg, nbytes, lbytes); - if (*nents > max_sg_nents) { - *nents = 0; - dev_err(dev, "Too many fragments. current %d max %d\n", - *nents, max_sg_nents); - return -ENOMEM; - } - /* In case of mmu the number of mapped nents might - * be changed from the original sgl nents - */ - *mapped_nents = dma_map_sg(dev, sg, *nents, direction); - if (*mapped_nents == 0) { + int ret = 0; + + *nents = cc_get_sgl_nents(dev, sg, nbytes, lbytes); + if (*nents > max_sg_nents) { + *nents = 0; + dev_err(dev, "Too many fragments. current %d max %d\n", + *nents, max_sg_nents); + return -ENOMEM; + } + + if (nents) { + + ret = dma_map_sg(dev, sg, *nents, direction); + if (dma_mapping_error(dev, ret)) { *nents = 0; - dev_err(dev, "dma_map_sg() sg buffer failed\n"); + dev_err(dev, "dma_map_sg() sg buffer failed %d\n", ret); return -ENOMEM; } } + *mapped_nents = ret; + return 0; } @@ -881,7 +879,7 @@ static int cc_aead_chain_data(struct cc_drvdata *drvdata, &src_last_bytes); sg_index = areq_ctx->src_sgl->length; //check where the data starts - while (sg_index <= size_to_skip) { + while (src_mapped_nents && (sg_index <= size_to_skip)) { src_mapped_nents--; offset -= areq_ctx->src_sgl->length; sgl = sg_next(areq_ctx->src_sgl); @@ -921,7 +919,7 @@ static int cc_aead_chain_data(struct cc_drvdata *drvdata, offset = size_to_skip; //check where the data starts - while (sg_index <= size_to_skip) { + while (dst_mapped_nents && sg_index <= size_to_skip) { dst_mapped_nents--; offset -= areq_ctx->dst_sgl->length; sgl = sg_next(areq_ctx->dst_sgl);
Deal gracefully with the event of being handed a scatterlist which is shorter than expected. This mitigates a crash in some cases of Crypto API calls due with scatterlists with a NULL first buffer, despite the aead.h forbidding doing so. Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> --- drivers/crypto/ccree/cc_buffer_mgr.c | 54 ++++++++++++++-------------- 1 file changed, 26 insertions(+), 28 deletions(-)