2020-01-27 12:50:05

by Gilad Ben-Yossef

[permalink] [raw]
Subject: [RFC] crypto: ccree - protect against short scatterlists

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 <[email protected]>
Reported-by: Geert Uytterhoeven <[email protected]>
---
drivers/crypto/ccree/cc_buffer_mgr.c | 54 ++++++++++++++--------------
1 file changed, 26 insertions(+), 28 deletions(-)

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);
--
2.23.0


2020-01-27 12:52:53

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [RFC] crypto: ccree - protect against short scatterlists

Hi Gilad,

On Mon, Jan 27, 2020 at 1:29 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 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 <[email protected]>
> Reported-by: Geert Uytterhoeven <[email protected]>

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

--
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

2020-01-27 15:10:18

by Gilad Ben-Yossef

[permalink] [raw]
Subject: Re: [RFC] crypto: ccree - protect against short scatterlists

On Mon, Jan 27, 2020 at 2:52 PM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Gilad,
>
> On Mon, Jan 27, 2020 at 1:29 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 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 <[email protected]>
> > Reported-by: Geert Uytterhoeven <[email protected]>
>
> 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!