Re: [PATCH 00/11] crypto: ccree - fixes and cleanups

Re: [PATCH 00/11] crypto: ccree - fixes and cleanups

[Geert Uytterhoeven]

Hi Gilad,

On Thu, Jan 16, 2020 at 11:25 AM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> A bunch of fixes and code cleanups for the ccree driver

Thank you!

I wanted to give this a try, but it looks like CCREE is no longer working
on R-Car H3, both with/without this series.

E.g. with renesas-devel[*] and renesas_defconfig +
CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=n, I get the following crash:

ccree e6601000.crypto: ARM CryptoCell 630P Driver: HW version
0xAF400001/0xDCC63000, Driver version 5.0
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)
------------[ cut here ]------------
kernel BUG at kernel/dma/swiotlb.c:497!
Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
CPU: 7 PID: 189 Comm: cryptomgr_test Not tainted 5.5.0-rc7-arm64-renesas #463
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 : ffff800012313430
x29: ffff800012313430 x28: 0000000000000000
x27: 0000000000000000 x26: 0000000738e7e000
x25: ffff0006fa5f8010 x24: 0000000000000000
x23: ffff800011aed000 x22: 0000000000000000
x21: 0000000000000000 x20: 00000000000e8000
x19: ffff80001105e000 x18: ffffffffffffffff
x17: 0000000000000007 x16: 0000000000000001
x15: ffff800010f5f908 x14: ffff800092313cf7
x13: ffff0006ff0b4000 x12: 0000000000000001
x11: 0000000000000003 x10: 0000000000200000
x9 : 0000000000000000 x8 : 0000000000000001
x7 : ffff800011aed9e0 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+0x10c/0x1a8
 cc_map_aead_request+0x160/0x990
 cc_proc_aead+0x140/0xef8
 cc_aead_encrypt+0x48/0x68
 crypto_aead_encrypt+0x20/0x30
 test_aead_vec_cfg+0x20c/0x848
 test_aead+0xb8/0x140
 alg_test_aead+0x94/0x178
 alg_test+0x108/0x3f8
 cryptomgr_test+0x40/0x48
 kthread+0x11c/0x120
 ret_from_fork+0x10/0x18
Code: f9402fbc 17ffffa0 f9000bb3 f9002fbc (d4210000)
---[ end trace 272124cd4e3fd6f0 ]---
note: cryptomgr_test[189] exited with preempt_count 1
------------[ cut here ]------------

FWIW, the same happens on R-Car H3 ES1.0.
I haven't tried investigating when it stopped working.
I stopped running the crypto manager tests when they were broken by
CONFIG_HARDENED_USERCOPY_PAGESPAN=y.

Do you have a clue?
Thanks!

[*] https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-devel.git/

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 00/11] crypto: ccree - fixes and cleanups

[Gilad Ben-Yossef]

Hi,

On Wed, Jan 22, 2020 at 6:51 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Gilad,
>
> On Thu, Jan 16, 2020 at 11:25 AM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> > A bunch of fixes and code cleanups for the ccree driver
>
> Thank you!
>
> I wanted to give this a try, but it looks like CCREE is no longer working
> on R-Car H3, both with/without this series.
>
> E.g. with renesas-devel[*] and renesas_defconfig +
> CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=n, I get the following crash:
>

Thank you for the bug report Geert!

My R-Car board is on loan at the moment to another project. I didn't
see this on our internal test board.
I will track down my R-Car board and reproduce this - hopefully
beginning of next week and will get back to you.

Thanks again,
Gilad

> ccree e6601000.crypto: ARM CryptoCell 630P Driver: HW version
> 0xAF400001/0xDCC63000, Driver version 5.0
> 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)
> ------------[ cut here ]------------
> kernel BUG at kernel/dma/swiotlb.c:497!
> Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
> CPU: 7 PID: 189 Comm: cryptomgr_test Not tainted 5.5.0-rc7-arm64-renesas #463
> 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 : ffff800012313430
> x29: ffff800012313430 x28: 0000000000000000
> x27: 0000000000000000 x26: 0000000738e7e000
> x25: ffff0006fa5f8010 x24: 0000000000000000
> x23: ffff800011aed000 x22: 0000000000000000
> x21: 0000000000000000 x20: 00000000000e8000
> x19: ffff80001105e000 x18: ffffffffffffffff
> x17: 0000000000000007 x16: 0000000000000001
> x15: ffff800010f5f908 x14: ffff800092313cf7
> x13: ffff0006ff0b4000 x12: 0000000000000001
> x11: 0000000000000003 x10: 0000000000200000
> x9 : 0000000000000000 x8 : 0000000000000001
> x7 : ffff800011aed9e0 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+0x10c/0x1a8
>  cc_map_aead_request+0x160/0x990
>  cc_proc_aead+0x140/0xef8
>  cc_aead_encrypt+0x48/0x68
>  crypto_aead_encrypt+0x20/0x30
>  test_aead_vec_cfg+0x20c/0x848
>  test_aead+0xb8/0x140
>  alg_test_aead+0x94/0x178
>  alg_test+0x108/0x3f8
>  cryptomgr_test+0x40/0x48
>  kthread+0x11c/0x120
>  ret_from_fork+0x10/0x18
> Code: f9402fbc 17ffffa0 f9000bb3 f9002fbc (d4210000)
> ---[ end trace 272124cd4e3fd6f0 ]---
> note: cryptomgr_test[189] exited with preempt_count 1
> ------------[ cut here ]------------
>
> FWIW, the same happens on R-Car H3 ES1.0.
> I haven't tried investigating when it stopped working.
> I stopped running the crypto manager tests when they were broken by
> CONFIG_HARDENED_USERCOPY_PAGESPAN=y.
>
> Do you have a clue?
> Thanks!
>
> [*] https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-devel.git/
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> 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



-- 
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!

Re: [PATCH 00/11] crypto: ccree - fixes and cleanups

[Geert Uytterhoeven]

Hi Gilad

On Thu, Jan 23, 2020 at 12:44 PM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> On Wed, Jan 22, 2020 at 6:51 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, Jan 16, 2020 at 11:25 AM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> > > A bunch of fixes and code cleanups for the ccree driver
> >
> > Thank you!
> >
> > I wanted to give this a try, but it looks like CCREE is no longer working
> > on R-Car H3, both with/without this series.
> >
> > E.g. with renesas-devel[*] and renesas_defconfig +
> > CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=n, I get the following crash:
>
> Thank you for the bug report Geert!
>
> My R-Car board is on loan at the moment to another project. I didn't
> see this on our internal test board.
> I will track down my R-Car board and reproduce this - hopefully
> beginning of next week and will get back to you.

In the mean time, I've bisected this failure to commit cdfee5623290bc89
("driver core: initialize a default DMA mask for platform device").
However, this looks like a red herring, and seems to be only an exposer
of an underlying problem.

What's happening is that cc_map_aead_request() receives a request with
cryptlen = 0.  Due to DRV_CRYPTO_DIRECTION_ENCRYPT, the length to map is
increased by 8.  This seems to works fine if there is sufficient space
in the request's scatterlist.  However, if the scatterlist has only a
single entry of size zero, cc_map_sg() tries to map a zero-length DMA
buffer, and the BUG)() is triggered.

I noticed commits 04e6d25c5bb244c1 ("crypto: caam - fix zero-length
buffer DMA mapping") and 07586d3ddf284dd7 ("crypto: caam/qi2 - fix
zero-length buffer DMA mapping") fixed other issues related to
zero-length DMA buffers.  But this one seems to be different, and a bit
more complex.

Adding "if (!req->cryptlen) return 0;" to the top of cc_proc_aead() fixes
the crash, but makes the tests fail:

    cc_proc_aead:1946: cryptlen is zero!
    alg: aead: ccm-aes-ccree encryption test failed (wrong result) on
test vector 9, cfg="in-place"
    cc_proc_aead:1946: cryptlen is zero!
    alg: aead: gcm-aes-ccree encryption test failed (wrong result) on
test vector 0, cfg="in-place"

Do you have a clue?
Thanks!

> > ccree e6601000.crypto: ARM CryptoCell 630P Driver: HW version
> > 0xAF400001/0xDCC63000, Driver version 5.0
> > 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)
> > ------------[ cut here ]------------
> > kernel BUG at kernel/dma/swiotlb.c:497!
> > Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
> > CPU: 7 PID: 189 Comm: cryptomgr_test Not tainted 5.5.0-rc7-arm64-renesas #463
> > 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 : ffff800012313430
> > x29: ffff800012313430 x28: 0000000000000000
> > x27: 0000000000000000 x26: 0000000738e7e000
> > x25: ffff0006fa5f8010 x24: 0000000000000000
> > x23: ffff800011aed000 x22: 0000000000000000
> > x21: 0000000000000000 x20: 00000000000e8000
> > x19: ffff80001105e000 x18: ffffffffffffffff
> > x17: 0000000000000007 x16: 0000000000000001
> > x15: ffff800010f5f908 x14: ffff800092313cf7
> > x13: ffff0006ff0b4000 x12: 0000000000000001
> > x11: 0000000000000003 x10: 0000000000200000
> > x9 : 0000000000000000 x8 : 0000000000000001
> > x7 : ffff800011aed9e0 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+0x10c/0x1a8
> >  cc_map_aead_request+0x160/0x990
> >  cc_proc_aead+0x140/0xef8
> >  cc_aead_encrypt+0x48/0x68
> >  crypto_aead_encrypt+0x20/0x30
> >  test_aead_vec_cfg+0x20c/0x848
> >  test_aead+0xb8/0x140
> >  alg_test_aead+0x94/0x178
> >  alg_test+0x108/0x3f8
> >  cryptomgr_test+0x40/0x48
> >  kthread+0x11c/0x120
> >  ret_from_fork+0x10/0x18
> > Code: f9402fbc 17ffffa0 f9000bb3 f9002fbc (d4210000)
> > ---[ end trace 272124cd4e3fd6f0 ]---
> > note: cryptomgr_test[189] exited with preempt_count 1
> > ------------[ cut here ]------------
> >
> > FWIW, the same happens on R-Car H3 ES1.0.
> > I haven't tried investigating when it stopped working.
> > I stopped running the crypto manager tests when they were broken by
> > CONFIG_HARDENED_USERCOPY_PAGESPAN=y.
> >
> > Do you have a clue?
> > Thanks!
> >
> > [*] https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-devel.git/

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 00/11] crypto: ccree - fixes and cleanups

[Gilad Ben-Yossef]

On Thu, Jan 23, 2020 at 5:46 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Gilad
>
> On Thu, Jan 23, 2020 at 12:44 PM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> > On Wed, Jan 22, 2020 at 6:51 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Thu, Jan 16, 2020 at 11:25 AM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> > > > A bunch of fixes and code cleanups for the ccree driver
> > >
> > > Thank you!
> > >
> > > I wanted to give this a try, but it looks like CCREE is no longer working
> > > on R-Car H3, both with/without this series.
> > >
> > > E.g. with renesas-devel[*] and renesas_defconfig +
> > > CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=n, I get the following crash:
> >
> > Thank you for the bug report Geert!
> >
> > My R-Car board is on loan at the moment to another project. I didn't
> > see this on our internal test board.
> > I will track down my R-Car board and reproduce this - hopefully
> > beginning of next week and will get back to you.
>
> In the mean time, I've bisected this failure to commit cdfee5623290bc89
> ("driver core: initialize a default DMA mask for platform device").
> However, this looks like a red herring, and seems to be only an exposer
> of an underlying problem.

Thank you for continue digging into this.

> What's happening is that cc_map_aead_request() receives a request with
> cryptlen = 0.  Due to DRV_CRYPTO_DIRECTION_ENCRYPT, the length to map is
> increased by 8.  This seems to works fine if there is sufficient space
> in the request's scatterlist.  However, if the scatterlist has only a
> single entry of size zero, cc_map_sg() tries to map a zero-length DMA
> buffer, and the BUG)() is triggered.
>

OK, this does rings a bell - can you verify please if
CONFIG_CRYPTO_MANAGER_EXTRA_TESTS is enabled and if it does can you
see if it happens if it is turned off?

There was an issue I've seen happen only with
CONFIG_CRYPTO_MANAGER_EXTRA_TESTS enabled and only after commit
49763fc6b1af4 ("crypto: testmgr - generate inauthentic AEAD test
vectors") which I am chasing. I was half starting to believe it
was an issue in the testmgr commit and not the ccree driver.

However, the stack trace doesn't look exactly the same but the
description of the issue does. It seems you are seeing the same issue
in another code path.

Thank you - this is very helpful!

I now have a better direction to look into...

Gilad

Re: [PATCH 00/11] crypto: ccree - fixes and cleanups

[Geert Uytterhoeven]

Hi Gilad,

On Thu, Jan 23, 2020 at 7:19 PM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> On Thu, Jan 23, 2020 at 5:46 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, Jan 23, 2020 at 12:44 PM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> > > On Wed, Jan 22, 2020 at 6:51 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Thu, Jan 16, 2020 at 11:25 AM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> > > > > A bunch of fixes and code cleanups for the ccree driver
> > > >
> > > > Thank you!
> > > >
> > > > I wanted to give this a try, but it looks like CCREE is no longer working
> > > > on R-Car H3, both with/without this series.
> > > >
> > > > E.g. with renesas-devel[*] and renesas_defconfig +
> > > > CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=n, I get the following crash:
> > >
> > > Thank you for the bug report Geert!
> > >
> > > My R-Car board is on loan at the moment to another project. I didn't
> > > see this on our internal test board.
> > > I will track down my R-Car board and reproduce this - hopefully
> > > beginning of next week and will get back to you.
> >
> > In the mean time, I've bisected this failure to commit cdfee5623290bc89
> > ("driver core: initialize a default DMA mask for platform device").
> > However, this looks like a red herring, and seems to be only an exposer
> > of an underlying problem.
>
> Thank you for continue digging into this.
>
> > What's happening is that cc_map_aead_request() receives a request with
> > cryptlen = 0.  Due to DRV_CRYPTO_DIRECTION_ENCRYPT, the length to map is
> > increased by 8.  This seems to works fine if there is sufficient space
> > in the request's scatterlist.  However, if the scatterlist has only a
> > single entry of size zero, cc_map_sg() tries to map a zero-length DMA
> > buffer, and the BUG)() is triggered.
> >
>
> OK, this does rings a bell - can you verify please if
> CONFIG_CRYPTO_MANAGER_EXTRA_TESTS is enabled and if it does can you
> see if it happens if it is turned off?

No, I didn't have that option enabled.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 00/11] crypto: ccree - fixes and cleanups

[Gilad Ben-Yossef]

On Thu, Jan 23, 2020 at 10:09 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Gilad,
>
> On Thu, Jan 23, 2020 at 7:19 PM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> > On Thu, Jan 23, 2020 at 5:46 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Thu, Jan 23, 2020 at 12:44 PM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> > > > On Wed, Jan 22, 2020 at 6:51 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Thu, Jan 16, 2020 at 11:25 AM Gilad Ben-Yossef <gilad@benyossef.com> wrote:
> > > > > > A bunch of fixes and code cleanups for the ccree driver
> > > > >
> > > > > Thank you!
> > > > >
> > > > > I wanted to give this a try, but it looks like CCREE is no longer working
> > > > > on R-Car H3, both with/without this series.
> > > > >
> > > > > E.g. with renesas-devel[*] and renesas_defconfig +
> > > > > CONFIG_CRYPTO_MANAGER_DISABLE_TESTS=n, I get the following crash:
> > > >
> > > > Thank you for the bug report Geert!
> > > >
> > > > My R-Car board is on loan at the moment to another project. I didn't
> > > > see this on our internal test board.
> > > > I will track down my R-Car board and reproduce this - hopefully
> > > > beginning of next week and will get back to you.
> > >
> > > In the mean time, I've bisected this failure to commit cdfee5623290bc89
> > > ("driver core: initialize a default DMA mask for platform device").
> > > However, this looks like a red herring, and seems to be only an exposer
> > > of an underlying problem.
> >
> > Thank you for continue digging into this.
> >
> > > What's happening is that cc_map_aead_request() receives a request with
> > > cryptlen = 0.  Due to DRV_CRYPTO_DIRECTION_ENCRYPT, the length to map is
> > > increased by 8.  This seems to works fine if there is sufficient space
> > > in the request's scatterlist.  However, if the scatterlist has only a
> > > single entry of size zero, cc_map_sg() tries to map a zero-length DMA
> > > buffer, and the BUG)() is triggered.
> > >
> >
> > OK, this does rings a bell - can you verify please if
> > CONFIG_CRYPTO_MANAGER_EXTRA_TESTS is enabled and if it does can you
> > see if it happens if it is turned off?
>
> No, I didn't have that option enabled.

OK, I still did not get reunited with my R-Cat board, but I think I
have a direction.

I'm about to send an RFC patch which while  probably does not address
the root cause
will stop the crash if the issue is what I think it is and so will let
me know what you are
seeing is what I think you are seeing.

I'd be delighted if you can give it a spin...

Thanks,
Gilad