Message ID | 20200127150822.12126-1-gilad@benyossef.com (mailing list archive) |
---|---|
State | RFC |
Delegated to: | Herbert Xu |
Headers | show |
Series | [RFC,v3] crypto: ccree - protect against short scatterlists | expand |
Hi Gilad, On Mon, Jan 27, 2020 at 4:08 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 due to > attempt to map empty (but not NULL) scatterlists with none > zero lengths. > > Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> Thank you, boots fine on Salvator-XS with R-Car H3ES2.0, and CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=y. Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
On Mon, Jan 27, 2020 at 05:08:21PM +0200, Gilad Ben-Yossef wrote: > Deal gracefully with the event of being handed a scatterlist > which is shorter than expected. > > This mitigates a crash in some cases due to > attempt to map empty (but not NULL) scatterlists with none > zero lengths. > > Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> It's definitely wrong use of the crypto API to pass a scatterlist that's too short. Note that this is *not* what the test code is doing. So I don't think you should be hacking around it here. It is possible the bug is actually in cc_aead_chain_data()? It looks like it's adding the authentication tag size to the source data size for encryption, which is not correct. The authentication tag is part of the destination only. size_for_map += (direct == DRV_CRYPTO_DIRECTION_ENCRYPT) ? authsize : 0; src_mapped_nents = cc_get_sgl_nents(dev, req->src, size_for_map, &src_last_bytes); - Eric
On Mon, Jan 27, 2020 at 04:22:53PM +0100, Geert Uytterhoeven wrote: > Hi Gilad, > > On Mon, Jan 27, 2020 at 4:08 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 due to > > attempt to map empty (but not NULL) scatterlists with none > > zero lengths. > > > > Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com> > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > Thank you, boots fine on Salvator-XS with R-Car H3ES2.0, and > CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=y. > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Gr{oetje,eeting}s, > Note that you need to *unset* CONFIG_CRYPTO_MANAGER_DISABLE_TESTS to enable the self-tests. So to run the full tests, the following is needed: # CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y - Eric
Hi Eric, On Tue, Jan 28, 2020 at 4:01 AM Eric Biggers <ebiggers@kernel.org> wrote: > On Mon, Jan 27, 2020 at 04:22:53PM +0100, Geert Uytterhoeven wrote: > > On Mon, Jan 27, 2020 at 4:08 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 due to > > > attempt to map empty (but not NULL) scatterlists with none > > > zero lengths. > > > > > > Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com> > > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > > Thank you, boots fine on Salvator-XS with R-Car H3ES2.0, and > > CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=y. > > > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Note that you need to *unset* CONFIG_CRYPTO_MANAGER_DISABLE_TESTS to enable the > self-tests. Sorry, that's what I meant (too used to type "=y" to enable something ;-) > So to run the full tests, the following is needed: > > # CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set With just this, it no longer crashes. > CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y With the extra tests enabled, it still crashes :-( +alg: hash: skipping comparison tests for ghash-neon because ghash-generic is unavailable +alg: hash: skipping comparison tests for ghash-ce because ghash-generic is unavailable +alg: aead: skipping comparison tests for gcm-aes-ce because gcm_base(ctr(aes-generic),ghash-generic) is unavailable +alg: aead: skipping comparison tests for ccm-aes-ce because ccm_base(ctr(aes-generic),cbcmac(aes-generic)) is unavailable +alg: hash: skipping comparison tests for cmac-aes-ce because cmac(aes-generic) is unavailable +alg: hash: skipping comparison tests for xcbc-aes-ce because xcbc(aes-generic) is unavailable +alg: hash: skipping comparison tests for cbcmac-aes-ce because cbcmac(aes-generic) is unavailable +alg: skcipher: skipping comparison tests for cts-cbc-aes-ce because cts(cbc(aes-generic)) is unavailable +alg: skcipher: skipping comparison tests for essiv-cbc-aes-sha256-ce because essiv(cbc(aes-generic),sha256-generic) is unavailable [...] ccree e6601000.crypto: ARM CryptoCell 630P Driver: HW version 0xAF400001/0xDCC63000, Driver version 5.0 +alg: skcipher: blocksize for xts-aes-ccree (1) doesn't match generic impl (16) +alg: skcipher: skipping comparison tests for ofb-aes-ccree because ofb(aes-generic) is unavailable +alg: skcipher: skipping comparison tests for cts-cbc-aes-ccree because cts(cbc(aes-generic)) is unavailable +alg: skcipher: skipping comparison tests for cbc-3des-ccree because cbc(des3_ede-generic) is unavailable +alg: skcipher: skipping comparison tests for ecb-3des-ccree because ecb(des3_ede-generic) is unavailable +alg: skcipher: skipping comparison tests for cbc-des-ccree because cbc(des-generic) is unavailable +alg: skcipher: skipping comparison tests for ecb-des-ccree because ecb(des-generic) is unavailable +random: crng init done +alg: hash: skipping comparison tests for xcbc-aes-ccree because xcbc(aes-generic) is unavailable +alg: hash: skipping comparison tests for cmac-aes-ccree because cmac(aes-generic) is unavailable +alg: aead: blocksize for authenc-hmac-sha1-cbc-aes-ccree (0) doesn't match generic impl (16) +alg: aead: skipping comparison tests for authenc-hmac-sha1-cbc-des3-ccree because authenc(hmac(sha1-generic),cbc(des3_ede-generic)) is unavailable +alg: aead: blocksize for authenc-hmac-sha256-cbc-aes-ccree (0) doesn't match generic impl (16) +alg: aead: skipping comparison tests for authenc-hmac-sha256-cbc-des3-ccree because authenc(hmac(sha256-generic),cbc(des3_ede-generic)) is unavailable alg: No test for authenc(xcbc(aes),cbc(aes)) (authenc-xcbc-aes-cbc-aes-ccree) alg: No test for authenc(xcbc(aes),rfc3686(ctr(aes))) (authenc-xcbc-aes-rfc3686-ctr-aes-ccree) -ccree e6601000.crypto: ARM ccree device initialized [...] +------------[ cut here ]------------ +kernel BUG at kernel/dma/swiotlb.c:497! +Internal error: Oops - BUG: 0 [#1] PREEMPT SMP +CPU: 1 PID: 270 Comm: cryptomgr_test Not tainted 5.5.0-rc6-arm64-renesas-00814-g967bcc92bb54b957 #525 +Hardware name: Renesas Salvator-X 2nd version board based on r8a77951 (DT) +pstate: 80000005 (Nzcv daif -PAN -UAO) +pc : swiotlb_tbl_map_single+0x30c/0x380 +lr : swiotlb_map+0xb0/0x300 +sp : ffff80000a7e3500 +x29: ffff80000a7e3500 x28: 0000000000000000 +x27: 0000000000000000 x26: 0000800048000000 +x25: ffff0006fa648010 x24: 0000000000000000 +x23: ffff800009b1d000 x22: 0000000000000000 +x21: 0000000000000000 x20: 00000000000e8000 +x19: ffff80000908f000 x18: ffffffffffffffff +x17: 0000000000000007 x16: 0000000000000001 +x15: ffff800008f8f908 x14: 019a33cc65fe9730 +x13: c962fb942dc65ff8 x12: 912ac35cf58e27c0 +x11: 59f28b24bd56ef88 x10: 0000000000200000 +x9 : 0000000000000000 x8 : 0000000000000001 +x7 : ffff800009b1d9e0 x6 : 0000000000000000 +x5 : 0000000000000000 x4 : 0000000000000000 +x3 : 0000000000000000 x2 : 0000000000000000 +x1 : 0000000074000000 x0 : 0000000000000000 +Call trace: + swiotlb_tbl_map_single+0x30c/0x380 + swiotlb_map+0xb0/0x300 + dma_direct_map_page+0xb8/0x140 + dma_direct_map_sg+0x78/0xe0 + cc_map_sg+0x7c/0xd8 + cc_map_aead_request+0x160/0x990 + cc_proc_aead+0x140/0xeb0 + cc_aead_encrypt+0x48/0x68 + crypto_aead_encrypt+0x20/0x30 + generate_aead_message+0x158/0x338 + generate_random_aead_testvec.constprop.43+0x110/0x1c8 + alg_test_aead+0x350/0x400 + alg_test+0x108/0x410 + cryptomgr_test+0x40/0x48 + kthread+0x11c/0x120 + ret_from_fork+0x10/0x18 +Code: f9402fbc 17ffffa0 f9000bb3 f9002fbc (d4210000) +---[ end trace 42a5d23b5191edbc ]--- +note: cryptomgr_test[270] exited with preempt_count 1 +------------[ cut here ]------------ Gr{oetje,eeting}s, Geert
On Tue, Jan 28, 2020 at 10:30 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Eric, > > On Tue, Jan 28, 2020 at 4:01 AM Eric Biggers <ebiggers@kernel.org> wrote: > > On Mon, Jan 27, 2020 at 04:22:53PM +0100, Geert Uytterhoeven wrote: > > > On Mon, Jan 27, 2020 at 4:08 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 due to > > > > attempt to map empty (but not NULL) scatterlists with none > > > > zero lengths. > > > > > > > > Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com> > > > > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > > > > Thank you, boots fine on Salvator-XS with R-Car H3ES2.0, and > > > CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=y. > > > > > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > Note that you need to *unset* CONFIG_CRYPTO_MANAGER_DISABLE_TESTS to enable the > > self-tests. > > Sorry, that's what I meant (too used to type "=y" to enable something ;-) > > > So to run the full tests, the following is needed: > > > > # CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set > > With just this, it no longer crashes. > > > CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y > > With the extra tests enabled, it still crashes :-( > That's fine - this is the second issue I was talking about. The patch I sent before was not really a fix - more a stop gap designed to see if I understand the issue you are seeing. I will send out a patch fixing the root cause of both and this should resolve both issues. I just want to run it through some internal tests to make sure it did not break something else first. Thanks! Gilad
diff --git a/drivers/crypto/ccree/cc_buffer_mgr.c b/drivers/crypto/ccree/cc_buffer_mgr.c index a72586eccd81..c5d58becb66d 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,25 @@ 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) { - *nents = 0; - dev_err(dev, "dma_map_sg() sg buffer failed\n"); - return -ENOMEM; - } + 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; } + 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 %d\n", ret); + return -ENOMEM; + } + + *mapped_nents = ret; + return 0; } @@ -555,11 +550,11 @@ void cc_unmap_aead_request(struct device *dev, struct aead_request *req) sg_virt(req->src), areq_ctx->src.nents, areq_ctx->assoc.nents, areq_ctx->assoclen, req->cryptlen); - dma_unmap_sg(dev, req->src, sg_nents(req->src), DMA_BIDIRECTIONAL); + dma_unmap_sg(dev, req->src, areq_ctx->src.mapped_nents, DMA_BIDIRECTIONAL); if (req->src != req->dst) { dev_dbg(dev, "Unmapping dst sgl: req->dst=%pK\n", sg_virt(req->dst)); - dma_unmap_sg(dev, req->dst, sg_nents(req->dst), + dma_unmap_sg(dev, req->dst, areq_ctx->dst.mapped_nents, DMA_BIDIRECTIONAL); } if (drvdata->coherent && @@ -881,7 +876,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); @@ -908,7 +903,7 @@ static int cc_aead_chain_data(struct cc_drvdata *drvdata, size_for_map += crypto_aead_ivsize(tfm); rc = cc_map_sg(dev, req->dst, size_for_map, DMA_BIDIRECTIONAL, - &areq_ctx->dst.nents, + &areq_ctx->dst.mapped_nents, LLI_MAX_NUM_OF_DATA_ENTRIES, &dst_last_bytes, &dst_mapped_nents); if (rc) @@ -921,7 +916,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); @@ -1123,7 +1118,7 @@ int cc_map_aead_request(struct cc_drvdata *drvdata, struct aead_request *req) if (is_gcm4543) size_to_map += crypto_aead_ivsize(tfm); rc = cc_map_sg(dev, req->src, size_to_map, DMA_BIDIRECTIONAL, - &areq_ctx->src.nents, + &areq_ctx->src.mapped_nents, (LLI_MAX_NUM_OF_ASSOC_DATA_ENTRIES + LLI_MAX_NUM_OF_DATA_ENTRIES), &dummy, &mapped_nents); diff --git a/drivers/crypto/ccree/cc_buffer_mgr.h b/drivers/crypto/ccree/cc_buffer_mgr.h index af434872c6ff..827b6cb1236e 100644 --- a/drivers/crypto/ccree/cc_buffer_mgr.h +++ b/drivers/crypto/ccree/cc_buffer_mgr.h @@ -25,6 +25,7 @@ enum cc_sg_cpy_direct { struct cc_mlli { cc_sram_addr_t sram_addr; + unsigned int mapped_nents; unsigned int nents; //sg nents unsigned int mlli_nents; //mlli nents might be different than the above };
Deal gracefully with the event of being handed a scatterlist which is shorter than expected. This mitigates a crash in some cases due to attempt to map empty (but not NULL) scatterlists with none zero lengths. Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org> --- drivers/crypto/ccree/cc_buffer_mgr.c | 65 +++++++++++++--------------- drivers/crypto/ccree/cc_buffer_mgr.h | 1 + 2 files changed, 31 insertions(+), 35 deletions(-)