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 <[email protected]>
Reported-by: Geert Uytterhoeven <[email protected]>
---
drivers/crypto/ccree/cc_buffer_mgr.c | 65 +++++++++++++---------------
drivers/crypto/ccree/cc_buffer_mgr.h | 1 +
2 files changed, 31 insertions(+), 35 deletions(-)
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
};
--
2.23.0
Hi Gilad,
On Mon, Jan 27, 2020 at 4:08 PM Gilad Ben-Yossef <[email protected]> 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 <[email protected]>
> Reported-by: Geert Uytterhoeven <[email protected]>
Thank you, boots fine on Salvator-XS with R-Car H3ES2.0, and
CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=y.
Tested-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
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 <[email protected]>
> Reported-by: Geert Uytterhoeven <[email protected]>
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 <[email protected]> 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 <[email protected]>
> > Reported-by: Geert Uytterhoeven <[email protected]>
>
> Thank you, boots fine on Salvator-XS with R-Car H3ES2.0, and
> CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=y.
>
> Tested-by: Geert Uytterhoeven <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > Reported-by: Geert Uytterhoeven <[email protected]>
> >
> > Thank you, boots fine on Salvator-XS with R-Car H3ES2.0, and
> > CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=y.
> >
> > Tested-by: Geert Uytterhoeven <[email protected]>
>
> 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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, Jan 28, 2020 at 10:30 AM Geert Uytterhoeven
<[email protected]> wrote:
>
> Hi Eric,
>
> On Tue, Jan 28, 2020 at 4:01 AM Eric Biggers <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > Reported-by: Geert Uytterhoeven <[email protected]>
> > >
> > > Thank you, boots fine on Salvator-XS with R-Car H3ES2.0, and
> > > CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=y.
> > >
> > > Tested-by: Geert Uytterhoeven <[email protected]>
> >
> > 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