Re: [PATCH v2 3/6] crypto: caam - use devres to de-initialize the RNG
On 10/22/2019 6:30 PM, Andrey Smirnov wrote: > Use devres to de-initialize the RNG and drop explicit de-initialization > code in caam_remove(). > > Signed-off-by: Andrey Smirnov > Cc: Chris Healy > Cc: Lucas Stach > Cc: Horia Geantă > Cc: Herbert Xu > Cc: Iuliana Prodan > Cc: linux-crypto@vger.kernel.org > Cc: linux-ker...@vger.kernel.org Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH v2 6/6] crypto: caam - populate platform devices last
On 10/22/2019 6:30 PM, Andrey Smirnov wrote: > Move the call to devm_of_platform_populate() at the end of > caam_probe(), so we won't try to add any child devices until all of > the initialization is finished successfully. > > Signed-off-by: Andrey Smirnov > Cc: Chris Healy > Cc: Lucas Stach > Cc: Horia Geantă > Cc: Herbert Xu > Cc: Iuliana Prodan > Cc: linux-crypto@vger.kernel.org > Cc: linux-ker...@vger.kernel.org Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH 16/25] crypto: mxs - switch to skcipher API
On 10/14/2019 3:19 PM, Ard Biesheuvel wrote:
> Commit 7a7ffe65c8c5 ("crypto: skcipher - Add top-level skcipher interface")
> dated 20 august 2015 introduced the new skcipher API which is supposed to
> replace both blkcipher and ablkcipher. While all consumers of the API have
> been converted long ago, some producers of the ablkcipher remain, forcing
> us to keep the ablkcipher support routines alive, along with the matching
> code to expose [a]blkciphers via the skcipher API.
>
> So switch this driver to the skcipher API, allowing us to finally drop the
> blkcipher code in the near future.
>
> Cc: Shawn Guo
> Cc: Sascha Hauer
> Cc: Pengutronix Kernel Team
> Cc: Fabio Estevam
> Cc: NXP Linux Team
> Signed-off-by: Ard Biesheuvel
Tested-by: Horia Geantă
Thanks,
Horia
Re: [PATCH v2] crypto: caam - use mapped_{src,dst}_nents for descriptor
On 9/26/2019 3:26 PM, Iuliana Prodan wrote:
> The mapped_{src,dst}_nents _returned_ from the dma_map_sg
> call (which could be less than src/dst_nents) have to be
> used to generate the job descriptors.
>
> Signed-off-by: Iuliana Prodan
Reviewed-by: Horia Geantă
Thanks,
Horia
Re: [PATCH] crypto: caam - use mapped_{src,dst}_nents for descriptor
On 9/25/2019 4:04 PM, Iuliana Prodan wrote:
> @@ -428,17 +433,18 @@ static int set_rsa_priv_f1_pdb(struct akcipher_request
> *req,
> return -ENOMEM;
> }
>
> - if (edesc->src_nents > 1) {
> + if (edesc->mapped_src_nents > 1) {
> pdb->sgf |= RSA_PRIV_PDB_SGF_G;
> pdb->g_dma = edesc->sec4_sg_dma;
> - sec4_sg_index += edesc->src_nents;
> + sec4_sg_index += edesc->mapped_src_nents;
> +
> } else {
> struct caam_rsa_req_ctx *req_ctx = akcipher_request_ctx(req);
>
> pdb->g_dma = sg_dma_address(req_ctx->fixup_src);
> }
>
> - if (edesc->dst_nents > 1) {
> + if (edesc->mapped_dst_nents > 1) {
> pdb->sgf |= RSA_PRIV_PDB_SGF_F;
> pdb->f_dma = edesc->sec4_sg_dma +
>sec4_sg_index * sizeof(struct sec4_sg_entry);
AFAICS there are a few other places besides set_rsa_priv_f1_pdb
that should be updated:
set_rsa_pub_pdb
set_rsa_priv_f2_pdb
set_rsa_priv_f3_pdb
Horia
Re: [PATCH] crypto: caam - use the same jr for rng init/exit
On 9/18/2019 9:01 AM, Andrey Smirnov wrote: > On Wed, Sep 11, 2019 at 2:35 AM Horia Geanta wrote: >> >> On 9/4/2019 5:35 AM, Andrey Smirnov wrote: >>> In order to allow caam_jr_enqueue() to lock underlying JR's >>> device (via device_lock(), see commit that follows) we need to make >>> sure that no code calls caam_jr_enqueue() as a part of caam_jr_probe() >>> to avoid a deadlock. Unfortunately, current implementation of caamrng >>> code does exactly that in caam_init_buf(). >>> > > I don't think your patch addresses the above, so it can probably be > cut out of the message. > No, it does not, it addresses the issue right below. Not sure what you mean by "cut out of the message". If you look carefully in the original message, at some pointer there is a scissors line. >>> Another big problem with original caamrng initialization is a >>> circular reference in the form of: >>> >>> 1. caam_rng_init() aquires JR via caam_jr_alloc(). Freed only by >>> caam_rng_exit() >>> >>> 2. caam_rng_exit() is only called by unregister_algs() once last JR >>> is shut down >>> >>> 3. caam_jr_remove() won't call unregister_algs() for last JR >>> until tfm_count reaches zero, which can only happen via >>> unregister_algs() -> caam_rng_exit() call chain. >>> >>> To avoid all of that, convert caamrng code to a platform device driver >>> and extend caam_probe() to create corresponding platform device. >>> >>> Additionally, this change also allows us to remove any access to >>> struct caam_drv_private in caamrng.c as well as simplify resource >>> ownership/deallocation via devres. >>> >> I would avoid adding platform devices that don't have >> corresponding DT nodes. >> >> There's some generic advice here: >> https://www.kernel.org/doc/html/latest/driver-api/driver-model/platform.html#legacy-drivers-device-probing >> >> and there's also previous experience in caam driver: >> 6b175685b4a1 crypto: caam/qi - don't allocate an extra platform device >> > > Hmm, I am not sure how that experience relates to the case we have > with hwrng, but OK, I'm going to assume that platform driver approach > is a no-go. > Not specific to hwrng, but platform drivers in general: [...] SMMU. A platform device allocated using platform_device_register_full() is not completely set up - most importantly .dma_configure() is not called. Horia
Re: [PATCH 10/12] crypto: caam - populate platform devices last
On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> @@ -906,6 +900,13 @@ static int caam_probe(struct platform_device *pdev)
> debugfs_create_blob("tdsk", S_IRUSR | S_IRGRP | S_IROTH, ctrlpriv->ctl,
> &ctrlpriv->ctl_tdsk_wrap);
> #endif
> +
> + ret = devm_of_platform_populate(dev);
> + if (ret) {
> + dev_err(dev, "JR platform devices creation error\n");
> + return ret;
> + }
> +
> return 0;
> }
This is a bit better:
if (ret)
dev_err(dev, "JR platform devices creation error\n");
return ret;
Horia
Re: [PATCH 09/12] crypto: caam - user devres to populate platform devices
On 9/4/2019 5:35 AM, Andrey Smirnov wrote: > Use devres to de-initialize the RNG and drop explicit de-initialization > code in caam_remove(). > > Signed-off-by: Andrey Smirnov > Cc: Chris Healy > Cc: Lucas Stach > Cc: Horia Geantă > Cc: Herbert Xu > Cc: Iuliana Prodan > Cc: linux-crypto@vger.kernel.org > Cc: linux-ker...@vger.kernel.org Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH 08/12] crypto: caam - use devres to de-initialize QI
On 9/4/2019 5:35 AM, Andrey Smirnov wrote: > Use devres to de-initialize the QI and drop explicit de-initialization > code in caam_remove(). > > Signed-off-by: Andrey Smirnov > Cc: Chris Healy > Cc: Lucas Stach > Cc: Horia Geantă > Cc: Herbert Xu > Cc: Iuliana Prodan > Cc: linux-crypto@vger.kernel.org > Cc: linux-ker...@vger.kernel.org Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH 12/12] crypto: caam - change JR device ownership scheme
On 9/18/2019 6:13 AM, Andrey Smirnov wrote:
>> I think you need to do some form of slow wait loop in jrpriv until
>> jrpriv->tfm_count reaches zero.
> Hmm, what do we do if it never does? Why do you think it would be
> better than cancelling all outstanding jobs and resetting the HW?
>
Herbert,
What should a driver do when:
-user tries to unbind it AND
-there are tfms referencing algorithms registered by this driver
1. If driver tries to unregister the algorithms during its .remove()
callback, then this BUG_ON is hit:
int crypto_unregister_alg(struct crypto_alg *alg)
{
[...]
BUG_ON(refcount_read(&alg->cra_refcnt) != 1);
2. If driver exits without unregistering the algorithms,
next time one of the tfms referencing those algorithms will be used
bad things will happen.
3. There is no mechanism in crypto core for notifying users
to stop using a tfm.
Thanks,
Horia
[PATCH] crypto: caam - use the same jr for rng init/exit
On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> In order to allow caam_jr_enqueue() to lock underlying JR's
> device (via device_lock(), see commit that follows) we need to make
> sure that no code calls caam_jr_enqueue() as a part of caam_jr_probe()
> to avoid a deadlock. Unfortunately, current implementation of caamrng
> code does exactly that in caam_init_buf().
>
> Another big problem with original caamrng initialization is a
> circular reference in the form of:
>
> 1. caam_rng_init() aquires JR via caam_jr_alloc(). Freed only by
> caam_rng_exit()
>
> 2. caam_rng_exit() is only called by unregister_algs() once last JR
> is shut down
>
> 3. caam_jr_remove() won't call unregister_algs() for last JR
> until tfm_count reaches zero, which can only happen via
> unregister_algs() -> caam_rng_exit() call chain.
>
> To avoid all of that, convert caamrng code to a platform device driver
> and extend caam_probe() to create corresponding platform device.
>
> Additionally, this change also allows us to remove any access to
> struct caam_drv_private in caamrng.c as well as simplify resource
> ownership/deallocation via devres.
>
I would avoid adding platform devices that don't have
corresponding DT nodes.
There's some generic advice here:
https://www.kernel.org/doc/html/latest/driver-api/driver-model/platform.html#legacy-drivers-device-probing
and there's also previous experience in caam driver:
6b175685b4a1 crypto: caam/qi - don't allocate an extra platform device
The issue in caamrng is actually that caam/jr driver (jr.c) tries to call
caam_rng_exit() on the last available jr device.
Instead, caam_rng_exit() must be called on the same jr device that
was used during caam_rng_init().
Otherwise, by the time:
void caam_rng_exit(void)
{
if (!init_done)
return;
caam_jr_free(rng_ctx->jrdev);
hwrng_unregister(&caam_rng);
[...]
is executed, rng_ctx->jrdev might have been removed.
This will cause an oops in caam_jr_free().
caam_cleanup() - .cleanup hwrng callback that is called when doing
hwrng_unregister() - also needs to be executed on that jr device.
The problem can be easily reproduced as follows.
If caamrng was initialized on jr0:
[...]
caam_jr 2101000.jr0: registering rng-caam
[...]
then by manually unbinding jr0 first, then jr1 (using i.MX6SX):
# echo -n "2101000.jr0" > /sys/bus/platform/drivers/caam_jr/
# echo -n "2102000.jr1" > /sys/bus/platform/drivers/caam_jr/
Unable to handle kernel NULL pointer dereference at virtual address 0040
pgd = 572e14e7
[0040] *pgd=be2e8831
Internal error: Oops: 17 [#1] SMP ARM
Modules linked in:
CPU: 0 PID: 629 Comm: sh Not tainted 5.3.0-rc1-00299-g8e2a2738e5d3-dirty #8
Hardware name: Freescale i.MX6 SoloX (Device Tree)
PC is at caam_jr_free+0xc/0x24
LR is at caam_rng_exit+0x20/0x3c
pc : []lr : []psr: 200f0013
sp : e9669e68 ip : 1488 fp :
r10: r9 : e9669f80 r8 : e9284010
r7 : e872d410 r6 : e872d410 r5 : e872d400 r4 : c1aa7cd8
r3 : r2 : 0040 r1 : r0 : e872d010
Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
Control: 10c5387d Table: a969004a DAC: 0051
Process sh (pid: 629, stack limit = 0xfc1b6e94)
Stack: (0xe9669e68 to 0xe966a000)
9e60: e865c940 c08af7dc e872d410 e872d410 c13a9cec c06b223c
9e80: c06b2218 e872d410 e81a9410 c06b08dc c13806f0 000b c13a9cec c06aeaf8
9ea0: 000b 000b e9284000 e91189c0 c0318c3c
9ec0: e95ddbd0 e8843500 c1308b08 c6614c9f e8843500 00438340 e9668000 0004
9ee0: c0285e00 0001 c0288a44
9f00: c1308b28 0001 c130911c 0001 c13e81d1 c0288a44
9f20: e8ed9800 c019df00 e8ed9a7c c028ac08 0001 c0288a44 c6614c9f
9f40: c1308b08 000b 00438340 e9669f80 e8843500 00438340 e9668000 c028899c
9f60: e95ddbc0 c6614c9f e8843500 e8843500 c1308b08 000b 00438340 c0288bfc
9f80: c6614c9f 000b 00438340 b6ef1da0 0004
9fa0: c01011c4 c0101000 000b 00438340 0001 00438340 000b
9fc0: 000b 00438340 b6ef1da0 0004 00438340 000b
9fe0: 006c bea7f908 b6e19e58 b6e7325c 600f0010 0001
[] (caam_jr_free) from [] (caam_rng_exit+0x20/0x3c)
[] (caam_rng_exit) from [] (caam_jr_remove+0x38/0xc0)
[] (caam_jr_remove) from [] (platform_drv_remove+0x24/0x3c)
[] (platform_drv_remove) from []
(device_release_driver_internal+0xdc/0x1a0)
[] (device_release_driver_internal) from []
(unbind_store+0x5c/0xcc)
[] (unbind_store) from [] (kernfs_fop_write+0xfc/0x1e0)
[] (kernfs_fop_write) from [] (__vfs_write+0x2c/0x1d0)
[] (__vfs_write) from [] (vfs_write+0xa0/0x180)
[] (vfs_write) from [] (ksys_write+0x5c/0xdc)
[] (ksys_write) from [] (ret_fast_syscall+0x0/0x28)
Exception stack(0xe9669fa8 to 0xe9669ff0)
9fa0: 000b 00438340 0001 00438340 000b
9fc0: 000b 004
Re: [PATCH 07/12] crypto: caam - use devres to de-initialize the RNG
On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> Use devres to de-initialize the RNG and drop explicit de-initialization
> code in caam_remove().
>
> Signed-off-by: Andrey Smirnov
> Cc: Chris Healy
> Cc: Lucas Stach
> Cc: Horia Geantă
> Cc: Herbert Xu
> Cc: Iuliana Prodan
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> ---
> drivers/crypto/caam/ctrl.c | 129 -
> 1 file changed, 70 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> index 254963498abc..25f8f76551a5 100644
> --- a/drivers/crypto/caam/ctrl.c
> +++ b/drivers/crypto/caam/ctrl.c
> @@ -175,6 +175,73 @@ static inline int run_descriptor_deco0(struct device
> *ctrldev, u32 *desc,
> return 0;
> }
>
> +/*
> + * deinstantiate_rng - builds and executes a descriptor on DECO0,
> + * which deinitializes the RNG block.
> + * @ctrldev - pointer to device
> + * @state_handle_mask - bitmask containing the instantiation status
> + * for the RNG4 state handles which exist in
> + * the RNG4 block: 1 if it's been instantiated
> + *
> + * Return: - 0 if no error occurred
> + * - -ENOMEM if there isn't enough memory to allocate the descriptor
> + * - -ENODEV if DECO0 couldn't be acquired
> + * - -EAGAIN if an error occurred when executing the descriptor
> + */
> +static int deinstantiate_rng(struct device *ctrldev, int state_handle_mask)
I assume this function is not modified, only moved further up
to avoid forward declaration.
> + if (!ret) {
> + ret = devm_add_action_or_reset(ctrldev, devm_deinstantiate_rng,
> +ctrldev);
> }
Braces not needed.
Is there any guidance wrt. when *not* to use devres?
Horia
Re: [PATCH 00/12] CAAM bugfixes, small improvements
On 9/9/2019 3:52 PM, Herbert Xu wrote: > On Mon, Sep 09, 2019 at 12:07:17PM +0000, Horia Geanta wrote: >> >> Why all? >> I've ack-ed only 1 and 4. >> >> Besides this, patches 11 and/or 12 break the functionality, >> i.e. driver gets stuck during crypto self-tests. > > Should I back out 5-12 or everything but patch 1? > > Patch 4 can't be applied without 2/3. > Let's go with patches 1-4, and revert 5-12. Thanks, Horia
Re: [PATCH 06/12] crypto: caam - use devres to remove debugfs
On 9/4/2019 5:35 AM, Andrey Smirnov wrote: > Use devres to remove debugfs and drop corresponding > debugfs_remove_recursive() call. > > Signed-off-by: Andrey Smirnov > Cc: Chris Healy > Cc: Lucas Stach > Cc: Horia Geantă > Cc: Herbert Xu > Cc: Iuliana Prodan > Cc: linux-crypto@vger.kernel.org > Cc: linux-ker...@vger.kernel.org Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH 05/12] crypto: caam - use devres to unmap memory
On 9/4/2019 5:35 AM, Andrey Smirnov wrote: > Use devres to unmap memory and drop corresponding iounmap() call. > > Signed-off-by: Andrey Smirnov > Cc: Chris Healy > Cc: Lucas Stach > Cc: Horia Geantă > Cc: Herbert Xu > Cc: Iuliana Prodan > Cc: linux-crypto@vger.kernel.org > Cc: linux-ker...@vger.kernel.org Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH 02/12] crypto: caam - use devres to unmap JR's registers
On 9/4/2019 5:35 AM, Andrey Smirnov wrote: > Use devres to unmap memory and drop explicit de-initialization > code. > > NOTE: There's no corresponding unmapping code in caam_jr_remove which > seems like a resource leak. > > Signed-off-by: Andrey Smirnov > Cc: Chris Healy > Cc: Lucas Stach > Cc: Horia Geantă > Cc: Herbert Xu > Cc: Iuliana Prodan > Cc: linux-crypto@vger.kernel.org > Cc: linux-ker...@vger.kernel.org Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH 00/12] CAAM bugfixes, small improvements
On 9/9/2019 10:53 AM, Herbert Xu wrote: > On Tue, Sep 03, 2019 at 07:35:03PM -0700, Andrey Smirnov wrote: >> Everyone: >> >> This series bugfixes and small improvement I made while doing more >> testing of CAAM code: >> >> - "crypto: caam - make sure clocks are enabled first" >> >>fixes a recent regression (and, conincidentally a leak cause by one >>of my i.MX8MQ patches) >> >> - "crypto: caam - use devres to unmap JR's registers" >>"crypto: caam - check irq_of_parse_and_map for errors" >> >>are small improvements >> >> - "crypto: caam - dispose of IRQ mapping only after IRQ is freed" >> >>fixes a bug introduced by my i.MX8MQ series >> >> - "crypto: caam - use devres to unmap memory" >>"crypto: caam - use devres to remove debugfs" >>"crypto: caam - use devres to de-initialize the RNG" >>"crypto: caam - use devres to de-initialize QI" >>"crypto: caam - user devres to populate platform devices" >>"crypto: caam - populate platform devices last" >> >>are devres conversions/small improvments >> >> - "crypto: caam - convert caamrng to platform device" >>"crypto: caam - change JR device ownership scheme" >> >>are more of an RFC than proper fixes. I don't have a very high >>confidence in those fixes, but I think they are a good conversation >>stater about the best approach to fix those issues >> >> Thanks, >> Andrey Smirnov >> >> Andrey Smirnov (12): >> crypto: caam - make sure clocks are enabled first >> crypto: caam - use devres to unmap JR's registers >> crypto: caam - check irq_of_parse_and_map for errors >> crypto: caam - dispose of IRQ mapping only after IRQ is freed >> crypto: caam - use devres to unmap memory >> crypto: caam - use devres to remove debugfs >> crypto: caam - use devres to de-initialize the RNG >> crypto: caam - use devres to de-initialize QI >> crypto: caam - user devres to populate platform devices >> crypto: caam - populate platform devices last >> crypto: caam - convert caamrng to platform device >> crypto: caam - change JR device ownership scheme >> >> drivers/crypto/caam/caamrng.c | 102 +--- >> drivers/crypto/caam/ctrl.c| 294 ++ >> drivers/crypto/caam/intern.h | 4 - >> drivers/crypto/caam/jr.c | 90 --- >> drivers/crypto/caam/qi.c | 8 +- >> drivers/crypto/caam/qi.h | 1 - >> 6 files changed, 267 insertions(+), 232 deletions(-) > > All applied. Thanks. > Why all? I've ack-ed only 1 and 4. Besides this, patches 11 and/or 12 break the functionality, i.e. driver gets stuck during crypto self-tests. Horia
Re: crypto: caam - Cast to long first before pointer conversion
On 9/9/2019 10:46 AM, Herbert Xu wrote:
> On Tue, Sep 03, 2019 at 07:35:07PM -0700, Andrey Smirnov wrote:
>> With IRQ requesting being managed by devres we need to make sure that
>> we dispose of IRQ mapping after and not before it is free'd (otherwise
>> we'll end up with a warning from the kernel). To achieve that simply
>> convert IRQ mapping to rely on devres as well.
>>
>> Fixes: f314f12db65c ("crypto: caam - convert caam_jr_init() to use devres")
>> Signed-off-by: Andrey Smirnov
>> Cc: Chris Healy
>> Cc: Lucas Stach
>> Cc: Horia Geantă
>> Cc: Herbert Xu
>> Cc: Iuliana Prodan
>> Cc: linux-crypto@vger.kernel.org
>> Cc: linux-ker...@vger.kernel.org
>> ---
>> drivers/crypto/caam/jr.c | 14 ++
>> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> I needed to apply this on top of it to shut up the compiler:
>
> ---8<---
> While storing an int in a pointer is safe the compiler is not
> happy about it. So we need some extra casting in order to make
> this warning free.
>
> Fixes: 1d3f75bce123 ("crypto: caam - dispose of IRQ mapping only...")
> Signed-off-by: Herbert Xu
>
Thanks Herbert.
Indeed, this is needed for silencing compilation on ARM64
(while compiling for ARM works fine).
Reviewed-by: Horia Geantă
for the squashed patches.
Horia
Re: [PATCH 03/12] crypto: caam - check irq_of_parse_and_map for errors
On 9/4/2019 5:35 AM, Andrey Smirnov wrote: > Irq_of_parse_and_map will return zero in case of error, so add a error > check for that. > > Signed-off-by: Andrey Smirnov > Cc: Chris Healy > Cc: Lucas Stach > Cc: Horia Geantă > Cc: Herbert Xu > Cc: Iuliana Prodan > Cc: linux-crypto@vger.kernel.org > Cc: linux-ker...@vger.kernel.org Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH 04/12] crypto: caam - dispose of IRQ mapping only after IRQ is freed
On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> With IRQ requesting being managed by devres we need to make sure that
> we dispose of IRQ mapping after and not before it is free'd (otherwise
> we'll end up with a warning from the kernel). To achieve that simply
> convert IRQ mapping to rely on devres as well.
>
> Fixes: f314f12db65c ("crypto: caam - convert caam_jr_init() to use devres")
> Signed-off-by: Andrey Smirnov
> Cc: Chris Healy
> Cc: Lucas Stach
> Cc: Horia Geantă
> Cc: Herbert Xu
> Cc: Iuliana Prodan
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
Cc:
Reviewed-by: Horia Geantă
I'd prefer pushing patches 1 and 4 first, considering they are fixes
while the rest are optimizations.
Thanks,
Horia
Re: [PATCH 01/12] crypto: caam - make sure clocks are enabled first
On 9/4/2019 5:35 AM, Andrey Smirnov wrote:
> In order to access IP block's registers we need to enable appropriate
> clocks first, otherwise we are risking hanging the CPU.
>
> The problem becomes very apparent when trying to use CAAM driver built
> as a kernel module. In that case caam_probe() gets called after
> clk_disable_unused() which means all of the necessary clocks are
> guaranteed to be disabled.
>
> Coincidentally, this change also fixes iomap leak introduced by early
> return (instead of "goto iounmap_ctrl") in commit
> 41fc54afae70 ("crypto: caam - simplfy clock initialization")
>
> Tested on ZII i.MX6Q+ RDU2
>
> Fixes: 176435ad2ac7 ("crypto: caam - defer probing until QMan is available")
> Fixes: 41fc54afae70 ("crypto: caam - simplfy clock initialization")
> Signed-off-by: Andrey Smirnov
> Cc: Chris Healy
> Cc: Lucas Stach
> Cc: Horia Geantă
> Cc: Herbert Xu
> Cc: Iuliana Prodan
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
Tested-by: Horia Geantă
Considering this is a boot hang, in case this does not make into v5.4
I would appreciate appending:
Cc:
Thanks,
Horia
Re: [PATCH] arm64: dts: imx8mq: Add CAAM node
On 8/31/2019 12:01 AM, Andrey Smirnov wrote:
> Add node for CAAM - Cryptographic Acceleration and Assurance Module.
>
> Signed-off-by: Horia Geantă
> Signed-off-by: Andrey Smirnov
> Cc: Cory Tusar
> Cc: Chris Healy
> Cc: Lucas Stach
> Cc: Herbert Xu
> Cc: Shawn Guo
> Cc: Iuliana Prodan
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> ---
>
> Shawn:
>
> Just a bit of a context: as per this thread
> https://lore.kernel.org/linux-crypto/20190830131547.ga27...@gondor.apana.org.au/
> I am hoping I can get and Ack from you for this patch, so it can go
> via cryptodev tree.
>
Could we please get an Ack in time for v5.4?
Thanks,
Horia
> Thanks,
> Andrey Smirnov
>
> arch/arm64/boot/dts/freescale/imx8mq.dtsi | 30 +++
> 1 file changed, 30 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> index d09b808eff87..752d5a61878c 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> @@ -728,6 +728,36 @@
> status = "disabled";
> };
>
> + crypto: crypto@3090 {
> + compatible = "fsl,sec-v4.0";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + reg = <0x3090 0x4>;
> + ranges = <0 0x3090 0x4>;
> + interrupts = ;
> + clocks = <&clk IMX8MQ_CLK_AHB>,
> + <&clk IMX8MQ_CLK_IPG_ROOT>;
> + clock-names = "aclk", "ipg";
> +
> + sec_jr0: jr@1000 {
> + compatible = "fsl,sec-v4.0-job-ring";
> + reg = <0x1000 0x1000>;
> + interrupts = IRQ_TYPE_LEVEL_HIGH>;
> + };
> +
> + sec_jr1: jr@2000 {
> + compatible = "fsl,sec-v4.0-job-ring";
> + reg = <0x2000 0x1000>;
> + interrupts = IRQ_TYPE_LEVEL_HIGH>;
> + };
> +
> + sec_jr2: jr@3000 {
> + compatible = "fsl,sec-v4.0-job-ring";
> + reg = <0x3000 0x1000>;
> + interrupts = IRQ_TYPE_LEVEL_HIGH>;
> + };
> + };
> +
> i2c1: i2c@30a2 {
> compatible = "fsl,imx8mq-i2c", "fsl,imx21-i2c";
> reg = <0x30a2 0x1>;
>
Re: [PATCH v7 00/15] crypto: caam - Add i.MX8MQ support
On 8/13/2019 9:51 PM, Andrey Smirnov wrote:
> On Tue, Aug 13, 2019 at 6:59 AM Horia Geanta wrote:
>>
>> On 8/12/2019 11:08 PM, Andrey Smirnov wrote:
>>> Everyone:
>>>
>>> Picking up where Chris left off (I chatted with him privately
>>> beforehead), this series adds support for i.MX8MQ to CAAM driver. Just
>>> like [v1], this series is i.MX8MQ only.
>>>
>>> Feedback is welcome!
>>> Thanks,
>>> Andrey Smirnov
>>>
>>> Changes since [v6]:
>>>
>>> - Fixed build problems in "crypto: caam - make CAAM_PTR_SZ dynamic"
>>>
>>> - Collected Reviewied-by from Horia
>>>
>>> - "crypto: caam - force DMA address to 32-bit on 64-bit i.MX SoCs"
>>> is changed to check 'caam_ptr_sz' instead of using 'caam_imx'
>>>
>>> - Incorporated feedback for "crypto: caam - request JR IRQ as the
>>> last step" and "crypto: caam - simplfy clock initialization"
>>>
>> FYI - the series does not apply cleanly on current cryptodev-2.6 tree.
>>
>
> OK, sorry about that, will fix.
>
Please also include the crypto DT node in v8 series - see patch below.
I would like to go with it through the cryptodev-2.6 tree,
to save one kernel release cycle.
This way kernel v5.4 would support CAAM on i.MX8MQ.
That's how we previously did for i.MX7ULP and Herbert and Shawn agreed.
Details (including who to Cc) here:
https://patchwork.kernel.org/patch/10978811/
-- >8 --
Subject: [PATCH] arm64: dts: imx8mq: add crypto node
Add node for CAAM - Cryptographic Acceleration and Assurance Module.
Signed-off-by: Horia Geantă
---
arch/arm64/boot/dts/freescale/imx8mq.dtsi | 30 +++
1 file changed, 30 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
index d09b808eff87..197965dac505 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
@@ -728,6 +728,36 @@
status = "disabled";
};
+ crypto: crypto@3090 {
+ compatible = "fsl,sec-v4.0";
+ #address-cells = <0x1>;
+ #size-cells = <0x1>;
+ reg = <0x3090 0x4>;
+ ranges = <0 0x3090 0x4>;
+ interrupts = ;
+ clocks = <&clk IMX8MQ_CLK_AHB>,
+<&clk IMX8MQ_CLK_IPG_ROOT>;
+ clock-names = "aclk", "ipg";
+
+ sec_jr0: jr0@1000 {
+compatible = "fsl,sec-v4.0-job-ring";
+reg = <0x1000 0x1000>;
+interrupts = ;
+ };
+
+ sec_jr1: jr1@2000 {
+compatible = "fsl,sec-v4.0-job-ring";
+reg = <0x2000 0x1000>;
+interrupts = ;
+ };
+
+ sec_jr2: jr2@3000 {
+compatible = "fsl,sec-v4.0-job-ring";
+reg = <0x3000 0x1000>;
+interrupts = ;
+ };
+ };
+
i2c1: i2c@30a2 {
compatible = "fsl,imx8mq-i2c", "fsl,imx21-i2c";
reg = <0x30a2 0x1>;
--
2.17.1
Re: [PATCH v7 00/15] crypto: caam - Add i.MX8MQ support
On 8/12/2019 11:08 PM, Andrey Smirnov wrote: > Everyone: > > Picking up where Chris left off (I chatted with him privately > beforehead), this series adds support for i.MX8MQ to CAAM driver. Just > like [v1], this series is i.MX8MQ only. > > Feedback is welcome! > Thanks, > Andrey Smirnov > > Changes since [v6]: > > - Fixed build problems in "crypto: caam - make CAAM_PTR_SZ dynamic" > > - Collected Reviewied-by from Horia > > - "crypto: caam - force DMA address to 32-bit on 64-bit i.MX SoCs" > is changed to check 'caam_ptr_sz' instead of using 'caam_imx' > > - Incorporated feedback for "crypto: caam - request JR IRQ as the > last step" and "crypto: caam - simplfy clock initialization" > FYI - the series does not apply cleanly on current cryptodev-2.6 tree. Horia
Re: [PATCH v6 12/14] crypto: caam - force DMA address to 32-bit on 64-bit i.MX SoCs
On 8/12/2019 10:27 PM, Andrey Smirnov wrote:
> On Mon, Aug 5, 2019 at 1:23 AM Horia Geanta wrote:
>>
>> On 7/17/2019 6:25 PM, Andrey Smirnov wrote:
>>> @@ -603,11 +603,13 @@ static int caam_probe(struct platform_device *pdev)
>>> ret = init_clocks(dev, ctrlpriv, imx_soc_match->data);
>>> if (ret)
>>> return ret;
>>> +
>>> + caam_ptr_sz = sizeof(u32);
>>> + } else {
>>> + caam_ptr_sz = sizeof(dma_addr_t);
>> caam_ptr_sz should be deduced by reading MCFGR[PS] bit, i.e. decoupled
>> from dma_addr_t.
>>
>
> MCFGR[PS] is not mentioned in i.MX8MQ SRM and MCFG_PS in CTPR_MS is
> documented as set to "0" (seems to match in real HW as well). Doesn't
> seem like a workable solution for i.MX8MQ. Is there something I am
> missing?
>
If CTPR_MS[PS]=0, this means CAAM does not allow choosing the "pointer size"
via MCFGR[PS]. Usually in this case the RM does not document MCFGR[PS] bit,
which is identical to MCFGR[PS]=0.
Thus the logic should be smth. like:
caam_ptr_sz = CTPR_MS[PS] && MCFGR[PS] ? 64 : 32;
>> There is another configuration that should be considered
>> (even though highly unlikely):
>> caam_ptr_sz=1 - > 32-bit addresses for CAAM
>> CONFIG_ARCH_DMA_ADDR_T_64BIT=n - 32-bit dma_addr_t
>> so the logic has to be carefully evaluated.
>>
>
> I don't understand what you mean here. 32-bit CAAM + 32-bit dma_addr_t
> should already be the case for i.MX6, etc. how is what you describe
> different?
>
Sorry for not being clear.
caam_ptr_sz=1 - > 32-bit addresses for CAAM
should have been
caam_ptr_sz=*64* - > 32-bit addresses for CAAM
i.e. CAAM address has "more than" (>) 32 bits (exact number of bits is
SoC / chassis specific) and thus will be represented on 8 bytes.
Thanks,
Horia
Re: [PATCH v6 12/14] crypto: caam - force DMA address to 32-bit on 64-bit i.MX SoCs
On 7/17/2019 6:25 PM, Andrey Smirnov wrote:
> i.MX8 SoC still use 32-bit addresses in its CAAM implmentation, so
i.MX8 SoC or i.MX8 mScale?
Looking at the documentation, some i.MX8 parts (for e.g. QM and QXP)
allow for 36-bit addresses.
> change all of the code to be able to handle that.
>
Shouldn't this case (32-bit CAAM and CONFIG_ARCH_DMA_ADDR_T_64BIT=y) work
for any ARMv8 SoC, i.e. how is this i.MX-specific?
> @@ -603,11 +603,13 @@ static int caam_probe(struct platform_device *pdev)
> ret = init_clocks(dev, ctrlpriv, imx_soc_match->data);
> if (ret)
> return ret;
> +
> + caam_ptr_sz = sizeof(u32);
> + } else {
> + caam_ptr_sz = sizeof(dma_addr_t);
caam_ptr_sz should be deduced by reading MCFGR[PS] bit, i.e. decoupled
from dma_addr_t.
There is another configuration that should be considered
(even though highly unlikely):
caam_ptr_sz=1 - > 32-bit addresses for CAAM
CONFIG_ARCH_DMA_ADDR_T_64BIT=n - 32-bit dma_addr_t
so the logic has to be carefully evaluated.
> @@ -191,7 +191,8 @@ static inline u64 caam_dma64_to_cpu(u64 value)
>
> static inline u64 cpu_to_caam_dma(u64 value)
> {
> - if (IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT))
> + if (IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT) &&
> + !caam_imx)
Related to my previous comment (i.MX-specific vs. SoC-generic),
this should probably change to smth. like: caam_ptr_sz == sizeof(u64)
> return cpu_to_caam_dma64(value);
> else
> return cpu_to_caam32(value);
> @@ -199,7 +200,8 @@ static inline u64 cpu_to_caam_dma(u64 value)
>
> static inline u64 caam_dma_to_cpu(u64 value)
> {
> - if (IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT))
> + if (IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT) &&
> + !caam_imx)
Same here.
> return caam_dma64_to_cpu(value);
> else
> return caam32_to_cpu(value);
> @@ -213,13 +215,24 @@ static inline u64 caam_dma_to_cpu(u64 value)
> static inline void jr_outentry_get(void *outring, int hw_idx, dma_addr_t
> *desc,
> u32 *jrstatus)
> {
> - struct {
> - dma_addr_t desc;/* Pointer to completed descriptor */
> - u32 jrstatus; /* Status for completed descriptor */
> - } __packed *outentry = outring;
>
> - *desc = outentry[hw_idx].desc;
> - *jrstatus = outentry[hw_idx].jrstatus;
> + if (caam_imx) {
Same here: if (caam_ptr_sz == sizeof(u32))
> + struct {
> + u32 desc;
> + u32 jrstatus;
> + } __packed *outentry = outring;
> +
> + *desc = outentry[hw_idx].desc;
> + *jrstatus = outentry[hw_idx].jrstatus;
> + } else {
> + struct {
> + dma_addr_t desc;/* Pointer to completed descriptor */
> + u32 jrstatus; /* Status for completed descriptor */
> + } __packed *outentry = outring;
> +
> + *desc = outentry[hw_idx].desc;
> + *jrstatus = outentry[hw_idx].jrstatus;
> + }
> }
>
> #define SIZEOF_JR_OUTENTRY (caam_ptr_sz + sizeof(u32))
> @@ -246,9 +259,15 @@ static inline u32 jr_outentry_jrstatus(void *outring,
> int hw_idx)
>
> static inline void jr_inpentry_set(void *inpring, int hw_idx, dma_addr_t val)
> {
> - dma_addr_t *inpentry = inpring;
> + if (caam_imx) {
And here: if (caam_ptr_sz == sizeof(u32))
> + u32 *inpentry = inpring;
>
> - inpentry[hw_idx] = val;
> + inpentry[hw_idx] = val;
> + } else {
> + dma_addr_t *inpentry = inpring;
> +
> + inpentry[hw_idx] = val;
> + }
> }
Horia
Re: [PATCH v3 0/2] crypto: validate inputs for gcm and aes
On 7/31/2019 4:06 PM, Iuliana Prodan wrote: > Added inline helper functions to check authsize and assoclen for > gcm, rfc4106 and rfc4543. > Added, also, inline helper function to check key length for AES algorithms. > These are used in the generic implementation of gcm/rfc4106/rfc4543 > and aes. > For the series: Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH v2 1/2] crypto: gcm - helper functions for assoclen/authsize check
On 7/30/2019 1:33 PM, Iuliana Prodan wrote: > --- a/include/crypto/gcm.h > +++ b/include/crypto/gcm.h > @@ -1,8 +1,63 @@ > #ifndef _CRYPTO_GCM_H > #define _CRYPTO_GCM_H > > +#include > + This is new in v2 and I missed it initially. If needed, should be used instead. Horia
Re: [PATCH v4 08/14] crypto: caam - update rfc4106 sh desc to support zero length input
On 7/30/2019 2:06 PM, Iuliana Prodan wrote: > Update share descriptor for rfc4106 to skip instructions in case > cryptlen is zero. If no instructions are jumped the DECO hangs and a > timeout error is thrown. > > Signed-off-by: Iuliana Prodan Reviewed-by: Horia Geanta Thanks, Horia
Re: [PATCH v3] crypto: gcm - restrict assoclen for rfc4543
On 7/30/2019 2:03 PM, Iuliana Prodan wrote:
> Based on seqiv, IPsec ESP and rfc4543/rfc4106 the assoclen can be 16 or
> 20 bytes.
>
> From esp4/esp6, assoclen is sizeof IP Header. This includes spi, seq_no
> and extended seq_no, that is 8 or 12 bytes.
> In seqiv, to asscolen is added the IV size (8 bytes).
> Therefore, the assoclen, for rfc4543, should be restricted to 16 or 20
> bytes, as for rfc4106.
>
> Signed-off-by: Iuliana Prodan
Reviewed-by: Horia Geantă
[...]
> diff --git a/crypto/gcm.c b/crypto/gcm.c
> index 2f3b50f..7eb5ced 100644
> --- a/crypto/gcm.c
> +++ b/crypto/gcm.c
> @@ -1034,11 +1034,23 @@ static int crypto_rfc4543_copy_src_to_dst(struct
> aead_request *req, bool enc)
>
> static int crypto_rfc4543_encrypt(struct aead_request *req)
> {
> + int err;
> +
> + err = crypto_ipsec_check_assoclen(req->assoclen);
> + if (err)
> + return err;
> +
> return crypto_rfc4543_crypt(req, true);
> }
Could as well be:
return crypto_ipsec_check_assoclen(req->assoclen) ?:
crypto_rfc4543_crypt(req, true);
The same for decryption.
Horia
Re: [PATCH v2 2/2] crypto: aes - helper function to validate key length for AES algorithms
On 7/30/2019 1:33 PM, Iuliana Prodan wrote: > Add inline helper function to check key length for AES algorithms. > The key can be 128, 192 or 256 bits size. > This function is used in the generic aes implementation. > > Signed-off-by: Iuliana Prodan Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH v2 1/2] crypto: gcm - helper functions for assoclen/authsize check
On 7/30/2019 1:33 PM, Iuliana Prodan wrote: > Added inline helper functions to check authsize and assoclen for > gcm, rfc4106 and rfc4543. > These are used in the generic implementation of gcm, rfc4106 and > rfc4543. > > Signed-off-by: Iuliana Prodan Reviewed-by: Horia Geantă Thanks, Horia
Re: Backlog support for CAAM?
On 7/28/2019 11:50 PM, Richard Weinberger wrote:
> - Ursprüngliche Mail -
>> Right now we're evaluating two options:
>> -reworking v5 above
>> -using crypto engine (crypto/crypto_engine.c)
>>
>> Ideally crypto engine should be the way to go.
>> However we need to make sure performance degradation is negligible,
>> which unfortunately is not case.
>>
>> Currently it seems that crypto engine has an issue with sending
>> multiple crypto requests from (SW) engine queue -> (HW) caam queue.
>>
>> More exactly, crypto_pump_requests() performs this check:
>>/* Make sure we are not already running a request */
>>if (engine->cur_req)
>>goto out;
>>
>> thus it's not possible to add more crypto requests to the caam queue
>> until HW finishes the work on the current crypto request and
>> calls crypto_finalize_request():
>>if (finalize_cur_req) {
>> [...]
>>engine->cur_req = NULL;
>
> Did you consider using a hybrid approach?
>
Yes, this is on our plate, though we haven't tried it yet.
> Please let me sketch my idea:
>
> - Let's have a worker thread which serves a software queue.
> - The software queue is a linked list of requests.
> - Upon job submission the driver checks whether the software queue is empty.
> - If the software queue is empty the regular submission continues.
> - Is the hardware queue full at this point, the request is put on the software
> queue and we return EBUSY.
> - If upon job submission the software queue not empty, the new job is also put
> on the software queue.
> - The worker thread is woken up every time a new job is put on the software
> queue and every time CAAM processed a job.
>
> That way we can keep the fast path fast. If hardware queue not full, software
> queue
> can be bypassed completely.
> If the software queue is used once it will become empty as soon jobs are
> getting
> submitted at a slower rate and the fast path will be used again.
>
> What do you think?
>
The optimization mentioned above - bypassing SW queue (i.e. try enqueuing
to HW queue if SW is empty) should probably be added into crypto engine
implementation itself - for e.g. in crypto_transfer_request().
Thanks,
Horia
Re: [PATCH v6 07/14] crypto: caam - drop 64-bit only wr/rd_reg64()
On 7/17/2019 6:25 PM, Andrey Smirnov wrote: > Since 32-bit of both wr_reg64 and rd_reg64 now use 64-bit IO helpers, > these functions should no longer be necessary. No functional change intended. > > Signed-off-by: Andrey Smirnov Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH v6 06/14] crypto: caam - use ioread64*_hi_lo in rd_reg64
On 7/17/2019 6:25 PM, Andrey Smirnov wrote: > Following the same transformation logic as outlined in previous commit > converting wr_reg64, convert rd_reg64 to use helpers from > first. No functional change intended. > > Signed-off-by: Andrey Smirnov Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH v6 05/14] crytpo: caam - make use of iowrite64*_hi_lo in wr_reg64
On 7/17/2019 6:25 PM, Andrey Smirnov wrote:
> In order to be able to unify 64 and 32 bit implementations of
> wr_reg64, let's convert it to use helpers from
> first. Here are the steps of the
> transformation:
>
> 1. Inline wr_reg32 helpers:
>
> if (!caam_imx && caam_little_end) {
> if (caam_little_end) {
> iowrite32(data >> 32, (u32 __iomem *)(reg) + 1);
> iowrite32(data, (u32 __iomem *)(reg));
> } else {
> iowrite32be(data >> 32, (u32 __iomem *)(reg) + 1);
> iowrite32be(data, (u32 __iomem *)(reg));
> }
> } else {
> if (caam_little_end) {
> iowrite32(data >> 32, (u32 __iomem *)(reg));
> iowrite32(data, (u32 __iomem *)(reg) + 1);
> } else {
> iowrite32be(data >> 32, (u32 __iomem *)(reg));
> iowrite32be(data, (u32 __iomem *)(reg) + 1);
> }
> }
>
> 2. Transfrom the conditionals such that the check for
> 'caam_little_end' is at the top level:
>
> if (caam_little_end) {
> if (!caam_imx) {
> iowrite32(data >> 32, (u32 __iomem *)(reg) + 1);
> iowrite32(data, (u32 __iomem *)(reg));
> } else {
> iowrite32(data >> 32, (u32 __iomem *)(reg));
> iowrite32(data, (u32 __iomem *)(reg) + 1);
> }
> } else {
> iowrite32be(data >> 32, (u32 __iomem *)(reg));
> iowrite32be(data, (u32 __iomem *)(reg) + 1);
> }
>
> 3. Invert the check for !caam_imx:
>
> if (caam_little_end) {
> if (caam_imx) {
> iowrite32(data >> 32, (u32 __iomem *)(reg));
> iowrite32(data, (u32 __iomem *)(reg) + 1);
> } else {
> iowrite32(data >> 32, (u32 __iomem *)(reg) + 1);
> iowrite32(data, (u32 __iomem *)(reg));
> }
> } else {
> iowrite32be(data >> 32, (u32 __iomem *)(reg));
> iowrite32be(data, (u32 __iomem *)(reg) + 1);
> }
>
> 4. Make use of iowrite64* helpers from
>
> if (caam_little_end) {
> if (caam_imx) {
> iowrite32(data >> 32, (u32 __iomem *)(reg));
> iowrite32(data, (u32 __iomem *)(reg) + 1);
> } else {
> iowrite64(data, reg);
> }
> } else {
> iowrite64be(data, reg);
> }
>
> No functional change intended.
>
> Signed-off-by: Andrey Smirnov
Reviewed-by: Horia Geantă
Just to clarify one thing.
For a previous patch I mentioned:
> To be consistent with CAAM engine HW spec: in case of 64-bit registers,
> irrespective of device endianness, the lower address should be read from
> / written to first, followed by the upper address.
https://lore.kernel.org/linux-crypto/vi1pr0401mb259145c2dfdb5e4084ea5dfc98...@vi1pr0401mb2591.eurprd04.prod.outlook.com/
I've checked again and actually there is no limitation wrt. the order in which
the two 32-bit parts of 64-bit registers are read from / written to,
except for performance counters (only available on DN parts, not on i.MX).
However, performance counters do not user {rd,wr}_reg64 and should be fixed
separately.
In conclusion, it's ok to use either hi_lo or lo_hi semantics (which is _data_
semantics btw).
It makes more sense for this patch to include io-64-nonatomic-hi-lo.h since
that's what regs.h currently uses.
Horia
Re: gcm.c question regarding the rfc4106 cipher suite
On 7/26/2019 6:55 PM, Pascal Van Leeuwen wrote: > Hi, > > I recently watched some patches fly by fixing issues in other drivers > regarding the checking > of supposedly illegal AAD sizes - i.e. to match the generic implementation > there. > I followed that with some interest as I'm about to implement this for the > inside-secure > driver. > > And something puzzles me. These patches, as well as the generic driver, seem > to expect > AAD lengths of 16 and 20. But to the best of my knowledge, and according to > the actual > RFC, the AAD data for GCM for ESP is only 8 or 12 bytes, namely only SPI + > sequence nr. > > The IV is NOT part of the AAD according to the RFC. It's inserted in the > encapsulated > output but it's neither encrypted nor authenticated. (It doesn't need to be > as it's > already authenticated implicitly as its used to generate the ciphertext. Note > that GMAC > (rfc4543) *does* have to authenticate the IV for this reason. But GCM doesn't > ...) > > So is this a bug or just some weird alternative way of providing the IV to > the cipher? > (beyond the usual req->iv) > Try to track the aead assoclen and cryptlen values starting from IPsec ESP (say net/ipv4/esp4.c) level. At this point IV length is part of cryptlen. When crypto API is called, for e.g. seqiv(rfc4106(gcm(aes))), IV length accounting changes from cryptlen to assoclen. In crypto/seqiv.c, seqiv_aead_encrypt(): aead_request_set_crypt(subreq, req->dst, req->dst, req->cryptlen - ivsize, info); aead_request_set_ad(subreq, req->assoclen + ivsize); thus the subrequest - rfc4106(gcm(aes)) - has to check for a 16 / 20-byte AAD. Hope this helps, Horia
Re: [PATCH v3 11/14] crypto: caam - free resources in case caam_rng registration failed
On 7/25/2019 4:58 PM, Iuliana Prodan wrote:
> Check the return value of the hardware registration for caam_rng and free
> resources in case of failure.
>
> Fixes: e24f7c9 ("crypto: caam - hwrng support")
> Signed-off-by: Iuliana Prodan
Reviewed-by: Horia Geantă
but please use at least 12 characters for the SHA1.
Horia
Re: [PATCH v3 08/14] crypto: caam - update rfc4106 sh desc to support zero length input
On 7/25/2019 4:58 PM, Iuliana Prodan wrote: > @@ -892,24 +895,26 @@ void cnstr_shdsc_rfc4106_encap(u32 * const desc, struct > alginfo *cdata, > append_math_sub_imm_u32(desc, VARSEQINLEN, REG3, IMM, ivsize); > append_math_add(desc, VARSEQOUTLEN, ZERO, REG3, CAAM_CMD_SZ); > > - /* Read assoc data */ > - append_seq_fifo_load(desc, 0, FIFOLD_CLASS_CLASS1 | FIFOLDST_VLF | > - FIFOLD_TYPE_AAD | FIFOLD_TYPE_FLUSH1); > + /* Skip AAD */ > + append_seq_fifo_store(desc, 0, FIFOST_TYPE_SKIP | FIFOLDST_VLF); > > - /* Skip IV */ > - append_seq_fifo_load(desc, ivsize, FIFOLD_CLASS_SKIP); > + /* Read cryptlen and set this value into VARSEQOUTLEN */ > + append_math_sub(desc, VARSEQOUTLEN, SEQINLEN, REG3, CAAM_CMD_SZ); > > - /* Will read cryptlen bytes */ > - append_math_sub(desc, VARSEQINLEN, SEQINLEN, REG0, CAAM_CMD_SZ); > + /* If cryptlen is ZERO jump to AAD command */ > + zero_cryptlen_jump_cmd = append_jump(desc, JUMP_TEST_ALL | > + JUMP_COND_MATH_Z); > > /* Workaround for erratum A-005473 (simultaneous SEQ FIFO skips) */ > - append_seq_fifo_load(desc, 0, FIFOLD_CLASS_CLASS1 | FIFOLD_TYPE_MSG); > + append_seq_fifo_store(desc, 0, FIFOST_TYPE_MESSAGE_DATA); > The workaround should be moved further down, just before the "Skip IV". By moving this instruction as far away as possible from previous seq fifo store, the chances of DECO stalling are reduced. > - /* Skip assoc data */ > - append_seq_fifo_store(desc, 0, FIFOST_TYPE_SKIP | FIFOLDST_VLF); > + /* Read AAD data */ > + append_seq_fifo_load(desc, 0, FIFOLD_CLASS_CLASS1 | FIFOLDST_VLF | > + FIFOLD_TYPE_AAD | FIFOLD_TYPE_FLUSH1); > > - /* cryptlen = seqoutlen - assoclen */ > - append_math_sub(desc, VARSEQOUTLEN, VARSEQINLEN, REG0, CAAM_CMD_SZ); > + /* Skip IV */ > + append_seq_fifo_load(desc, ivsize, FIFOLD_CLASS_SKIP); > + append_math_add(desc, VARSEQINLEN, VARSEQOUTLEN, REG0, CAAM_CMD_SZ); > > /* Write encrypted data */ > append_seq_fifo_store(desc, 0, FIFOST_TYPE_MESSAGE_DATA | FIFOLDST_VLF); > @@ -918,6 +923,18 @@ void cnstr_shdsc_rfc4106_encap(u32 * const desc, struct > alginfo *cdata, > append_seq_fifo_load(desc, 0, FIFOLD_CLASS_CLASS1 | FIFOLDST_VLF | >FIFOLD_TYPE_MSG | FIFOLD_TYPE_LAST1); > > + /* Jump instructions to avoid double reading of AAD */ > + skip_instructions = append_jump(desc, JUMP_TEST_ALL); > + > + /* There is no input data, cryptlen = 0 */ > + set_jump_tgt_here(desc, zero_cryptlen_jump_cmd); > + > + /* Read AAD */ > + append_seq_fifo_load(desc, 0, FIFOLD_CLASS_CLASS1 | FIFOLDST_VLF | > + FIFOLD_TYPE_AAD | FIFOLD_TYPE_LAST1); > + > + set_jump_tgt_here(desc, skip_instructions); > + > /* Write ICV */ > append_seq_store(desc, icvsize, LDST_CLASS_1_CCB | >LDST_SRCDST_BYTE_CONTEXT);
Re: [PATCH v3 06/14] crypto: caam - check assoclen
On 7/25/2019 4:58 PM, Iuliana Prodan wrote: > Check assoclen to solve the extra tests that expect -EINVAL to be > returned when the associated data size is not valid. > > Validated assoclen for RFC4106 and RFC4543 which expects an assoclen > of 16 or 20. > Based on seqiv, IPsec ESP and RFC4543/RFC4106 the assoclen is sizeof IP > Header (spi, seq_no, extended seq_no) and IV len. This can be 16 or 20 > bytes. > > Signed-off-by: Iuliana Prodan Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH v3 05/14] crypto: caam - check authsize
On 7/25/2019 4:58 PM, Iuliana Prodan wrote: > Check authsize to solve the extra tests that expect -EINVAL to be > returned when the authentication tag size is not valid. > > Validated authsize for GCM, RFC4106 and RFC4543. > > Signed-off-by: Iuliana Prodan Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH v3 04/14] crypto: caam - check key length
On 7/25/2019 4:58 PM, Iuliana Prodan wrote: > Check key length to solve the extra tests that expect -EINVAL to be > returned when the key size is not valid. > > Validated AES keylen for skcipher, ahash and aead. > > Signed-off-by: Iuliana Prodan Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH v3 10/14] crypto: caam - fix MDHA key derivation for certain user key lengths
On 7/25/2019 4:58 PM, Iuliana Prodan wrote: > From: Horia Geantă > > Fuzz testing uncovered an issue when |user key| > |derived key|. > Derived key generation has to be fixed in two cases: > > 1. Era >= 6 (DKP is available) > DKP cannot be used with immediate input key if |user key| > |derived key|, > since the resulting descriptor (after DKP execution) would be invalid - > having a few bytes from user key left in descriptor buffer > as incorrect opcodes. > > Fix DKP usage both in standalone hmac and in authenc algorithms. > For authenc the logic is simplified, by always storing both virtual > and dma key addresses. > > 2. Era < 6 > The same case (|user key| > |derived key|) fails when DKP > is not available. > Make sure gen_split_key() dma maps max(|user key|, |derived key|), > since this is an in-place (bidirectional) operation. > > Signed-off-by: Horia Geantă > > Changes since v2: > - fix MDHA key derivation for CAAM with era < 6. > --- The change log shouldn't be included in the commit message. Horia
Re: [PATCH v3 03/14] crypto: caam - update IV only when crypto operation succeeds
On 7/25/2019 4:58 PM, Iuliana Prodan wrote: > From: Horia Geantă > > skcipher encryption might fail and in some cases, like (invalid) input > length smaller then block size, updating the IV would lead to panic > due to copying from a negative offset (req->cryptlen - ivsize). > In v1 I mentioned the commit message needs to change: https://lore.kernel.org/linux-crypto/vi1pr0402mb34855e675a58e1221ace7b9498...@vi1pr0402mb3485.eurprd04.prod.outlook.com/ which happened in v2, however somehow v3 is identical to v1. Horia
Re: [PATCH 1/2] crypto: gcm - helper functions for assoclen/authsize check
On 7/25/2019 4:47 PM, Iuliana Prodan wrote: > Added inline helper functions to check authsize and assoclen for > gcm and rfc4106. Also rfc4543. > diff --git a/include/crypto/gcm.h b/include/crypto/gcm.h > index c50e057..9834b97 100644 > --- a/include/crypto/gcm.h > +++ b/include/crypto/gcm.h > @@ -5,4 +5,57 @@ > #define GCM_RFC4106_IV_SIZE 8 > #define GCM_RFC4543_IV_SIZE 8 > > +/* > + * validate authentication tag for GCM > + */ > +static inline int check_gcm_authsize(unsigned int authsize) I'd prefix the helper names with crypto_ Horia
Re: [PATCH 2/2] crypto: aes - helper function to validate key length for AES algorithms
On 7/25/2019 4:47 PM, Iuliana Prodan wrote: > Add inline helper function to check key length for AES algorithms. > The key can be 128, 192 or 256 bits size. > This function is used in the generic aes and aes_ti implementations. > Looks good. Will need to respin it, since aes has just been refactored and moved in lib/crypto/. Horia
Re: Backlog support for CAAM?
On 7/25/2019 12:22 AM, Richard Weinberger wrote:
> Hi!
>
> Recently I had the pleasure to debug a lockup on a imx6 based platform.
> It turned out that the lockup was caused by the CAAM driver because it
> just returns -EBUSY upon a full job ring.
>
> Then I found commits:
> 0618764cb25f ("dm crypt: fix deadlock when async crypto algorithm returns
> -EBUSY")
> c0403ec0bb5a ("Revert "dm crypt: fix deadlock when async crypto algorithm
> returns -EBUSY"")
>
Truly sorry for the inconvenience.
Indeed this is a caam driver issue, and not a dm-crypt one.
> Is there a reason why the driver has still no proper backlog support?
>
We've been rejected a few times or the implementation had performance issues:
v1: https://patchwork.kernel.org/patch/7144701
v2: https://patchwork.kernel.org/patch/7199241
v3: https://patchwork.kernel.org/patch/7221941
v4: https://patchwork.kernel.org/patch/7230241
v5: https://patchwork.kernel.org/patch/9033121
and we haven't been persistent enough.
> If it is just a matter of -ENOPATCH, I have some cycles left an can help.
> But before working on this topic I'd like to figure what the current state
> or plans are. :-)
>
Right now we're evaluating two options:
-reworking v5 above
-using crypto engine (crypto/crypto_engine.c)
Ideally crypto engine should be the way to go.
However we need to make sure performance degradation is negligible,
which unfortunately is not case.
Currently it seems that crypto engine has an issue with sending
multiple crypto requests from (SW) engine queue -> (HW) caam queue.
More exactly, crypto_pump_requests() performs this check:
/* Make sure we are not already running a request */
if (engine->cur_req)
goto out;
thus it's not possible to add more crypto requests to the caam queue
until HW finishes the work on the current crypto request and
calls crypto_finalize_request():
if (finalize_cur_req) {
[...]
engine->cur_req = NULL;
Horia
Re: [PATCH v6 04/14] crypto: caam - request JR IRQ as the last step
On 7/17/2019 6:25 PM, Andrey Smirnov wrote:
> In order to avoid any risk of JR IRQ request being handled while some
> of the resources used for that are not yet allocated move the code
> requesting said IRQ to the endo of caam_jr_init(). No functional
^ typo
> change intended.
>
What qualifies as a "functional change"?
I've seen this comment in several commits.
> error = caam_reset_hw_jr(dev);
> if (error)
> - goto out_kill_deq;
> + return error;
>
> error = -ENOMEM;
> jrp->inpring = dmam_alloc_coherent(dev, sizeof(*jrp->inpring) *
> JOBR_DEPTH, &inpbusaddr,
> GFP_KERNEL);
> if (!jrp->inpring)
> - goto out_kill_deq;
> + return -ENOMEM;
Above there's "error = -ENOMEM;", so why not "return err;" here and
in all the other cases below?
>
> jrp->outring = dmam_alloc_coherent(dev, sizeof(*jrp->outring) *
> JOBR_DEPTH, &outbusaddr,
> GFP_KERNEL);
> if (!jrp->outring)
> - goto out_kill_deq;
> + return -ENOMEM;
>
> jrp->entinfo = devm_kcalloc(dev, JOBR_DEPTH, sizeof(*jrp->entinfo),
> GFP_KERNEL);
> if (!jrp->entinfo)
> - goto out_kill_deq;
> + return -ENOMEM;
>
> for (i = 0; i < JOBR_DEPTH; i++)
> jrp->entinfo[i].desc_addr_dma = !0;
> @@ -483,10 +472,19 @@ static int caam_jr_init(struct device *dev)
> (JOBR_INTC_COUNT_THLD << JRCFG_ICDCT_SHIFT) |
> (JOBR_INTC_TIME_THLD << JRCFG_ICTT_SHIFT));
>
> + tasklet_init(&jrp->irqtask, caam_jr_dequeue, (unsigned long)dev);
> +
> + /* Connect job ring interrupt handler. */
> + error = devm_request_irq(dev, jrp->irq, caam_jr_interrupt, IRQF_SHARED,
> + dev_name(dev), dev);
> + if (error) {
> + dev_err(dev, "can't connect JobR %d interrupt (%d)\n",
> + jrp->ridx, jrp->irq);
> + tasklet_kill(&jrp->irqtask);
> + return error;
"return error;" should be moved out the if block.
> + }
> +
> return 0;
> -out_kill_deq:
> - tasklet_kill(&jrp->irqtask);
> - return error;
> }
Horia
Re: [PATCH v6 03/14] crypto: caam - convert caam_jr_init() to use devres
On 7/17/2019 6:25 PM, Andrey Smirnov wrote: > Use deveres to allocate all of the resources in caam_jr_init() (DMA > coherent and regular memory, IRQs) drop calls to corresponding > deallocation routines. No functional change intended. > > Signed-off-by: Andrey Smirnov Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH v6 02/14] crypto: caam - simplfy clock initialization
On 7/17/2019 6:25 PM, Andrey Smirnov wrote:
> Simplify clock initialization code by converting it to use clk-bulk,
> devres and soc_device_match() match table. No functional change
> intended.
>
Thanks.
Two nitpicks below.
> +static int init_clocks(struct device *dev,
> +struct caam_drv_private *ctrlpriv,
> +const struct caam_imx_data *data)
> +{
> + int ret;
> +
> + ctrlpriv->num_clks = data->num_clks;
> + ctrlpriv->clks = devm_kmemdup(dev, data->clks,
> + data->num_clks * sizeof(data->clks[0]),
> + GFP_KERNEL);
> + if (!ctrlpriv->clks)
> + return -ENOMEM;
> +
> + ret = devm_clk_bulk_get(dev, ctrlpriv->num_clks, ctrlpriv->clks);
> + if (ret) {
> + dev_err(dev,
> + "Failed to request all necessary clocks\n");
> + return ret;
> + }
> +
> + ret = clk_bulk_prepare_enable(ctrlpriv->num_clks, ctrlpriv->clks);
> + if (ret) {
> + dev_err(dev,
> + "Failed to prepare/enable all necessary clocks\n");
> + return ret;
> + }
> +
> + ret = devm_add_action_or_reset(dev, disable_clocks, ctrlpriv);
> + if (ret)
> + return ret;
> +
> + return 0;
Or directly:
return devm_add_action_or_reset(dev, disable_clocks, ctrlpriv);
> + imx_soc_match = soc_device_match(caam_imx_soc_table);
> + if (imx_soc_match) {
> + if (!imx_soc_match->data) {
> + dev_err(dev, "No clock data provided for i.MX SoC");
> + return -EINVAL;
[...]
> + ret = init_clocks(dev, ctrlpriv, imx_soc_match->data);
ctrlpriv can be retrieved using dev_get_drvdata(dev), thus there's no need
to pass it as parameter.
Horia
Re: [PATCH v5] crypto: caam/qi2 - Add printing dpseci fq stats using debugfs
On 7/23/2019 12:14 PM, Vakul Garg wrote: > Add support of printing the dpseci frame queue statistics using debugfs. > > Signed-off-by: Vakul Garg Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH v6 08/14] crypto: caam - make CAAM_PTR_SZ dynamic
On 7/17/2019 6:25 PM, Andrey Smirnov wrote:
> In order to be able to configure CAAM pointer size at run-time, which
> needed to support i.MX8MQ, which is 64-bit SoC with 32-bit pointer
> size, convert CAAM_PTR_SZ to refer to a global variable of the same
> name ("caam_ptr_sz") and adjust the rest of the code accordingly. No
> functional change intended.
>
I am seeing compilation errors like:
In file included from drivers/crypto/caam/ctrl.c:25:0:
drivers/crypto/caam/qi.h:87:6: error: variably modified 'sh_desc' at file scope
u32 sh_desc[MAX_SDLEN];
^
Adding comments for this commit, since it looks like the fixes
should be included here (related to DESC_JOB_IO_LEN vs. DESC_JOB_IO_LEN_MAX).
Please make sure caam/qi and and caam/qi2 drivers are at least compile-tested.
By caam/qi I am referring to:
CONFIG_CRYPTO_DEV_FSL_CAAM_CRYPTO_API_QI=y/m
and caam/qi2:
CONFIG_CRYPTO_DEV_FSL_DPAA2_CAAM=y/m
Horia
Re: [PATCH v4] crypto: caam/qi2 - Add printing dpseci fq stats using debugfs
On 7/23/2019 11:42 AM, Vakul Garg wrote: > diff --git a/drivers/crypto/caam/dpseci-debugfs.c > b/drivers/crypto/caam/dpseci-debugfs.c > new file mode 100644 > index ..2e43ba9b7491 > --- /dev/null > +++ b/drivers/crypto/caam/dpseci-debugfs.c > @@ -0,0 +1,80 @@ > +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */ > +/* Copyright 2019 NXP > + */ .c and .h files have different syntax, see: https://www.kernel.org/doc/html/latest/process/license-rules.html#license-identifier-syntax > diff --git a/drivers/crypto/caam/dpseci-debugfs.h > b/drivers/crypto/caam/dpseci-debugfs.h > new file mode 100644 > index ..1dbdb2587758 > --- /dev/null > +++ b/drivers/crypto/caam/dpseci-debugfs.h > @@ -0,0 +1,19 @@ > +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */ > +/* Copyright 2019 NXP > + */ Yet another nitpick: incorrect commenting style (for the 2nd comment) https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting Why not make it a single-line comment? Sorry for not catching these earlier. Horia
Re: [PATCH v2] crypto: caam/qi2 - Add printing dpseci fq stats using debugfs
On 7/23/2019 4:20 AM, Vakul Garg wrote:
>>> @@ -64,6 +65,7 @@ struct dpaa2_caam_priv {
>>> struct iommu_domain *domain;
>>>
>>> struct dpaa2_caam_priv_per_cpu __percpu *ppriv;
>>> + struct dentry *dfs_root;
>> dfs_root is used only in dpseci-debugfs.c, let's have it there as global.
>>
>
> I submitted this change in v3. There is still a minor issue with this patch
> version.
> Before submitting the next v4, I have a question.
>
> Could there be a situation that there are multiple dpseci objects assigned
> to kernel?
In theory, yes.
fsl-mc, the bus dpseci devices sit on, allows for multiple instances.
However, caam/qi2 (driver for dpseci devices) doesn't have support
for this.
For e.g., all dpseci instances would try to register the algorithms using
the same name & driver name - something that will trigger an error
in crypto API.
This could be easily fixed, however the real issue is that
there is no load balancing support - neither at crypto API level
nor at driver level.
> In that case, we need to maintain dfs_root for each separately.
>
Ok, let's keep dfs_root per device.
For now this has no practical value, but at least makes the work easier
in case load balancing support is added at some point.
Horia
Re: [PATCH v2] crypto: caam/qi2 - Add printing dpseci fq stats using debugfs
On 7/22/2019 5:29 PM, Vakul Garg wrote: >> -Original Message- >> From: Horia Geanta >> Sent: Monday, July 22, 2019 7:55 PM >> To: Vakul Garg ; linux-crypto@vger.kernel.org >> Cc: Aymen Sghaier ; >> herb...@gondor.apana.org.au >> Subject: Re: [PATCH v2] crypto: caam/qi2 - Add printing dpseci fq stats using >> debugfs >> >> On 7/19/2019 2:23 PM, Vakul Garg wrote: >> [...] >>> +if CRYPTO_DEV_FSL_DPAA2_CAAM >>> + >>> +config CRYPTO_DEV_FSL_DPAA2_CAAM_DEBUGFS >>> + depends on DEBUG_FS >>> + bool "Enable debugfs support" >>> + help >>> + Selecting this will enable printing of various debug information >>> + in the DPAA2 CAAM driver. >>> + >>> +endif >> Let's enable this based on CONFIG_DEBUG_FS. > > It is not clear to me. > Do you mean not have additional CRYPTO_DEV_FSL_DPAA2_CAAM_DEBUGFS? > Yes. >> >>> diff --git a/drivers/crypto/caam/Makefile >>> b/drivers/crypto/caam/Makefile index 9ab4e81ea21e..e4e9fa481a44 >> 100644 >>> --- a/drivers/crypto/caam/Makefile >>> +++ b/drivers/crypto/caam/Makefile >>> @@ -30,3 +30,4 @@ endif >>> obj-$(CONFIG_CRYPTO_DEV_FSL_DPAA2_CAAM) += dpaa2_caam.o >>> >>> dpaa2_caam-y:= caamalg_qi2.o dpseci.o >>> +dpaa2_caam-$(CONFIG_CRYPTO_DEV_FSL_DPAA2_CAAM_DEBUGFS) += >>> +dpseci-debugfs.o >> dpaa2_caam-$(CONFIG_DEBUG_FS) >> >> [...] >>> diff --git a/drivers/crypto/caam/caamalg_qi2.h >>> b/drivers/crypto/caam/caamalg_qi2.h >>> index 973f6296bc6f..b450e2a25c1f 100644 >>> --- a/drivers/crypto/caam/caamalg_qi2.h >>> +++ b/drivers/crypto/caam/caamalg_qi2.h >>> @@ -10,6 +10,7 @@ >>> #include >>> #include >>> #include >>> +#include >> How is this change related to current patch? >> > > It should have been here in first place because we have some napi related > things in this file. > It is required as I got compilation errors now. > Indeed, this is caused by including caamalg_qi2.h -> dpseci-debugfs.h -> dpseci-debugfs.c. Compiling this patch without the inclusion: CC drivers/crypto/caam/dpseci-debugfs.o In file included from drivers/crypto/caam/dpseci-debugfs.h:9:0, from drivers/crypto/caam/dpseci-debugfs.c:8: drivers/crypto/caam/caamalg_qi2.h:84:21: error: field 'napi' has incomplete type struct napi_struct napi; ^ drivers/crypto/caam/caamalg_qi2.h:85:20: error: field 'net_dev' has incomplete type struct net_device net_dev; ^ scripts/Makefile.build:278: recipe for target 'drivers/crypto/caam/dpseci-debugfs.o' failed Other driver files include linux/netdevice.h indirectly, via compat.h -> linux/rtnetlink.h. Most *.c files in drivers/crypto/caam solve dependencies by including compat.h, but this approach is messy. Let's keep the explicit inclusion then. Horia
Re: [PATCH v2] crypto: caam/qi2 - Add printing dpseci fq stats using debugfs
On 7/19/2019 2:23 PM, Vakul Garg wrote:
[...]
> +if CRYPTO_DEV_FSL_DPAA2_CAAM
> +
> +config CRYPTO_DEV_FSL_DPAA2_CAAM_DEBUGFS
> + depends on DEBUG_FS
> + bool "Enable debugfs support"
> + help
> + Selecting this will enable printing of various debug information
> + in the DPAA2 CAAM driver.
> +
> +endif
Let's enable this based on CONFIG_DEBUG_FS.
> diff --git a/drivers/crypto/caam/Makefile b/drivers/crypto/caam/Makefile
> index 9ab4e81ea21e..e4e9fa481a44 100644
> --- a/drivers/crypto/caam/Makefile
> +++ b/drivers/crypto/caam/Makefile
> @@ -30,3 +30,4 @@ endif
> obj-$(CONFIG_CRYPTO_DEV_FSL_DPAA2_CAAM) += dpaa2_caam.o
>
> dpaa2_caam-y:= caamalg_qi2.o dpseci.o
> +dpaa2_caam-$(CONFIG_CRYPTO_DEV_FSL_DPAA2_CAAM_DEBUGFS) += dpseci-debugfs.o
dpaa2_caam-$(CONFIG_DEBUG_FS)
[...]
> diff --git a/drivers/crypto/caam/caamalg_qi2.h
> b/drivers/crypto/caam/caamalg_qi2.h
> index 973f6296bc6f..b450e2a25c1f 100644
> --- a/drivers/crypto/caam/caamalg_qi2.h
> +++ b/drivers/crypto/caam/caamalg_qi2.h
> @@ -10,6 +10,7 @@
> #include
> #include
> #include
> +#include
How is this change related to current patch?
> #include "dpseci.h"
> #include "desc_constr.h"
>
> @@ -64,6 +65,7 @@ struct dpaa2_caam_priv {
> struct iommu_domain *domain;
>
> struct dpaa2_caam_priv_per_cpu __percpu *ppriv;
> + struct dentry *dfs_root;
dfs_root is used only in dpseci-debugfs.c, let's have it there as global.
Horia
Re: [PATCH] crypto: caam - move shared symbols in a common location
On 7/18/2019 5:49 PM, Iuliana Prodan wrote: > Moved to a common location the symbols shared by all CAAM drivers (jr, > qi, qi2). > > Signed-off-by: Iuliana Prodan Reviewed-by: Horia Geantă > --- > This patch depends on series: > https://patchwork.kernel.org/project/linux-crypto/list/?series=147479 > Note however that the patch will have to be respinned, since common_if needs to be reworked according to: https://lore.kernel.org/linux-crypto/20190719151526.uqvg4q4pbpzto...@gondor.apana.org.au/ i.e. generic helpers should be moved into crypto API Horia
Re: [PATCH] crypto: caam/qi2 - Increase napi budget to process more caam responses
On 7/18/2019 2:29 PM, Vakul Garg wrote: > While running ipsec processing for traffic through multiple network > interfaces, it is observed that caam driver gets less time to poll > responses from caam block compared to ethernet driver. This is because > ethernet driver has as many napi instances per cpu as the number of > ethernet interfaces in system. Therefore, caam driver's napi executes > lesser than the ethernet driver's napi instances. This results in > situation that we end up submitting more requests to caam (which it is > able to finish off quite fast), but don't dequeue the responses at same > rate. This makes caam response FQs bloat with large number of frames. In > some situations, it makes kernel crash due to out-of-memory. To prevent > it We increase the napi budget of dpseci driver to a big value so that > caam driver is able to drain its response queues at enough rate. > > Signed-off-by: Vakul Garg Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH v2 14/14] crypto: caam - change return value in case CAAM has no MDHA
On 7/19/2019 2:58 AM, Iuliana Prodan wrote:
> To be consistent with other CAAM modules, caamhash should return 0
> instead of -ENODEV in case CAAM has no MDHA.
>
> Based on commit 1b46c90c8e00 ("crypto: caam - convert top level drivers to
> libraries")
> the value returned by entry point is never checked and
> the exit point is always executed.
>
> Signed-off-by: Iuliana Prodan
Reviewed-by: Horia Geantă
Horia
Re: [PATCH v2 13/14] crypto: caam - unregister algorithm only if the registration succeeded
On 7/19/2019 2:58 AM, Iuliana Prodan wrote:
> To know if a registration succeeded added a new struct,
> caam_akcipher_alg, that keeps, also, the registration status.
> This status is updated in caam_pkc_init and verified in
> caam_pkc_exit to unregister an algorithm.
>
Fixes: 1b46c90c8e00 ("crypto: caam - convert top level drivers to libraries")
> Signed-off-by: Iuliana Prodan
Reviewed-by: Horia Geantă
Horia
Re: [PATCH v2 12/14] crypto: caam - execute module exit point only if necessary
On 7/19/2019 2:58 AM, Iuliana Prodan wrote:
> Commit 1b46c90c8e00 ("crypto: caam - convert top level drivers to libraries")
> changed entry and exit points behavior for caamalg,
> caamalg_qi, caamalg_qi2, caamhash, caampkc, caamrng.
>
> For example, previously caam_pkc_init() and caam_pkc_exit() were
> module entry/exit points. This means that if an error would happen
> in caam_pkc_init(), then caam_pkc_exit() wouldn't have been called.
> After the mentioned commit, caam_pkc_init() and caam_pkc_exit()
> are manually called - from jr.c. caam_pkc_exit() is called
> unconditionally, even if caam_pkc_init() failed.
>
> Added a global variable to keep the status of the algorithm
> registration and free of resources.
> The exit point of caampkc/caamrng module is executed only if the
> registration was successful. Therefore we avoid double free of
> resources in case the algorithm registration failed.
>
> Fixes: 1b46c90c8e00 ("crypto: caam - convert top level drivers to libraries")
> Signed-off-by: Iuliana Prodan
Reviewed-by: Horia Geantă
Horia
Re: [PATCH v2 11/14] crypto: caam - free resources in case caam_rng registration failed
On 7/19/2019 2:58 AM, Iuliana Prodan wrote:
> Check the return value of the hardware registration for caam_rng and free
> resources in case of failure.
>
> Fixes: 6e4e603a9 ("crypto: caam - Dynamic memory allocation for caam_rng_ctx
> object")
This should be:
Fixes: e24f7c9e87d4 ("crypto: caam - hwrng support")
since there are resources leaked (like DMA mapped buffers) due to not checking
the return code of hwrng_register() even in the initial caamrng commit.
This doesn't have much practical value, since we haven't seen this failure
in practice and we don't intend fixing previous kernel releases.
Horia
Re: [PATCH v2 08/14] crypto: caam - update rfc4106 sh desc to support zero length input
On 7/19/2019 2:58 AM, Iuliana Prodan wrote:
> Update share descriptor for rfc4106 to skip instructions in case
> cryptlen is zero. If no instructions are jumped the DECO hangs and a
> timeout error is thrown.
>
> Signed-off-by: Iuliana Prodan
> ---
> drivers/crypto/caam/caamalg_desc.c | 46
> +-
> drivers/crypto/caam/caamalg_desc.h | 2 +-
> 2 files changed, 31 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/crypto/caam/caamalg_desc.c
> b/drivers/crypto/caam/caamalg_desc.c
> index 7253183..99f419a 100644
> --- a/drivers/crypto/caam/caamalg_desc.c
> +++ b/drivers/crypto/caam/caamalg_desc.c
> @@ -843,13 +843,16 @@ EXPORT_SYMBOL(cnstr_shdsc_gcm_decap);
> * @ivsize: initialization vector size
> * @icvsize: integrity check value (ICV) size (truncated or full)
> * @is_qi: true when called from caam/qi
> + *
> + * Input sequence: AAD | PTXT
> + * Output sequence: AAD | CTXT | ICV
> + * AAD length (assoclen), which includes the IV length, is available in
> Math3.
> */
> void cnstr_shdsc_rfc4106_encap(u32 * const desc, struct alginfo *cdata,
> unsigned int ivsize, unsigned int icvsize,
> const bool is_qi)
> {
> - u32 *key_jump_cmd;
> -
> + u32 *key_jump_cmd, *zero_cryptlen_jump_cmd, *skip_instructions;
> init_sh_desc(desc, HDR_SHARE_SERIAL);
>
> /* Skip key loading if it is loaded due to sharing */
> @@ -890,26 +893,25 @@ void cnstr_shdsc_rfc4106_encap(u32 * const desc, struct
> alginfo *cdata,
> }
>
> append_math_sub_imm_u32(desc, VARSEQINLEN, REG3, IMM, ivsize);
> - append_math_add(desc, VARSEQOUTLEN, ZERO, REG3, CAAM_CMD_SZ);
> + append_math_add(desc, VARSEQOUTLEN, REG0, REG3, CAAM_CMD_SZ);
>
Why is this needed?
-all math registers are zero when descriptors start execution
-all caam hw revisions have support for math instruction
with ZERO as first operand
-descriptor size stays the same
> - /* Read assoc data */
> + /* Skip AAD */
> + append_seq_fifo_store(desc, 0, FIFOST_TYPE_SKIP | FIFOLDST_VLF);
> +
> + /* Read cryptlen and set this value into VARSEQOUTLEN */
> + append_math_sub(desc, VARSEQOUTLEN, SEQINLEN, REG3, CAAM_CMD_SZ);
> +
> + /* If cryptlen is ZERO jump to AAD command */
> + zero_cryptlen_jump_cmd = append_jump(desc, JUMP_TEST_ALL |
> + JUMP_COND_MATH_Z);
> +
> + /* Read AAD data */
> append_seq_fifo_load(desc, 0, FIFOLD_CLASS_CLASS1 | FIFOLDST_VLF |
>FIFOLD_TYPE_AAD | FIFOLD_TYPE_FLUSH1);
>
> /* Skip IV */
> append_seq_fifo_load(desc, ivsize, FIFOLD_CLASS_SKIP);
> -
> - /* Will read cryptlen bytes */
> - append_math_sub(desc, VARSEQINLEN, SEQINLEN, REG0, CAAM_CMD_SZ);
> -
> - /* Workaround for erratum A-005473 (simultaneous SEQ FIFO skips) */
> - append_seq_fifo_load(desc, 0, FIFOLD_CLASS_CLASS1 | FIFOLD_TYPE_MSG);
> -
Erratum workaround is dropped without any explanation.
The trigger condition (SKIPs both in input and output sequence) still exists.
The descriptor should be validated on a caam with erratum not fixed (era < 6).
Horia
Re: [PATCH v2 07/14] crypto: caam - check zero-length input
On 7/19/2019 2:58 AM, Iuliana Prodan wrote: > Check zero-length input, for skcipher algorithm, to solve the extra > tests. This is a valid operation, therefore the API will return no error. > > Signed-off-by: Iuliana Prodan Reviewed-by: Horia Geantă Horia
Re: [PATCH v2 06/14] crypto: caam - check assoclen
On 7/19/2019 2:58 AM, Iuliana Prodan wrote:
[...]
> --- a/drivers/crypto/caam/common_if.c
> +++ b/drivers/crypto/caam/common_if.c
> @@ -66,6 +66,23 @@ int check_rfc4106_authsize(unsigned int authsize)
> }
> EXPORT_SYMBOL(check_rfc4106_authsize);
>
> +/*
> + * validate assoclen for RFC4106/RFC4543
> + */
> +int check_ipsec_assoclen(unsigned int assoclen)
> +{
> + switch (assoclen) {
> + case 16:
> + case 20:
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(check_ipsec_assoclen);
> +
It's probably worth making this function (and even the other ones
in common_if) inline.
Herbert, is it worth having these checks moved to crypto API,
so they could be shared b/w all implementations?
This would also provide clear guidance wrt. rules related to
input parameters length.
Thanks,
Horia
Re: [PATCH v2 05/14] crypto: caam - check authsize
On 7/19/2019 2:58 AM, Iuliana Prodan wrote: [...] > diff --git a/drivers/crypto/caam/caamalg_qi.c > b/drivers/crypto/caam/caamalg_qi.c > index 46097e3..5f9b14a 100644 > --- a/drivers/crypto/caam/caamalg_qi.c > +++ b/drivers/crypto/caam/caamalg_qi.c > @@ -18,6 +18,7 @@ > #include "qi.h" > #include "jr.h" > #include "caamalg_desc.h" > +#include "common_if.h" > This include should have been added previously, in 04/14. [...] > diff --git a/drivers/crypto/caam/caamalg_qi2.c > b/drivers/crypto/caam/caamalg_qi2.c > index da4abf1..0b4de21 100644 > --- a/drivers/crypto/caam/caamalg_qi2.c > +++ b/drivers/crypto/caam/caamalg_qi2.c > @@ -10,6 +10,7 @@ > #include "dpseci_cmd.h" > #include "desc_constr.h" > #include "error.h" > +#include "common_if.h" Same here, the include should have been added previously, in 04/14. Horia
Re: [PATCH v2 04/14] crypto: caam - check key length
On 7/19/2019 2:58 AM, Iuliana Prodan wrote:
> Check key length to solve the extra tests that expect -EINVAL to be
> returned when the key size is not valid.
>
> Validated AES keylen for skcipher and ahash.
>
Also aead was updated.
> The check_aes_keylen function is added in a common file, to be used
> also for caam/qi and caam/qi2.
>
[...]
> diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
> index 28d55a0..6ac59b1 100644
> --- a/drivers/crypto/caam/caamalg.c
> +++ b/drivers/crypto/caam/caamalg.c
[...]
> @@ -683,10 +691,17 @@ static int rfc4106_setkey(struct crypto_aead *aead,
> {
> struct caam_ctx *ctx = crypto_aead_ctx(aead);
> struct device *jrdev = ctx->jrdev;
> + int err;
>
> if (keylen < 4)
> return -EINVAL;
>
This is no longer needed, check_aes_keylen() catches this case too.
> + err = check_aes_keylen(keylen - 4);
> + if (err) {
> + crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
> + return err;
> + }
> +
> print_hex_dump_debug("key in @"__stringify(__LINE__)": ",
>DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
>
> @@ -707,10 +722,17 @@ static int rfc4543_setkey(struct crypto_aead *aead,
> {
> struct caam_ctx *ctx = crypto_aead_ctx(aead);
> struct device *jrdev = ctx->jrdev;
> + int err;
>
> if (keylen < 4)
> return -EINVAL;
>
Same here, check_aes_keylen() handles this case.
> + err = check_aes_keylen(keylen - 4);
> + if (err) {
> + crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
> + return err;
> + }
> +
> print_hex_dump_debug("key in @"__stringify(__LINE__)": ",
>DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
>
[...]
> diff --git a/drivers/crypto/caam/caamalg_qi.c
> b/drivers/crypto/caam/caamalg_qi.c
> index 66531d6..46097e3 100644
> --- a/drivers/crypto/caam/caamalg_qi.c
> +++ b/drivers/crypto/caam/caamalg_qi.c
> @@ -385,6 +385,12 @@ static int gcm_setkey(struct crypto_aead *aead,
> struct device *jrdev = ctx->jrdev;
> int ret;
>
> + ret = check_aes_keylen(keylen);
> + if (ret) {
> + crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
> + return ret;
> + }
> +
> print_hex_dump_debug("key in @" __stringify(__LINE__)": ",
>DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
>
> @@ -483,6 +489,12 @@ static int rfc4106_setkey(struct crypto_aead *aead,
> if (keylen < 4)
> return -EINVAL;
>
Same here, check_aes_keylen() handles this case.
> + ret = check_aes_keylen(keylen - 4);
> + if (ret) {
> + crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
> + return ret;
> + }
> +
> print_hex_dump_debug("key in @" __stringify(__LINE__)": ",
>DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
>
> @@ -585,6 +597,12 @@ static int rfc4543_setkey(struct crypto_aead *aead,
> if (keylen < 4)
> return -EINVAL;
>
Same here, check_aes_keylen() handles this case.
> + ret = check_aes_keylen(keylen - 4);
> + if (ret) {
> + crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
> + return ret;
> + }
> +
> print_hex_dump_debug("key in @" __stringify(__LINE__)": ",
>DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
>
[...]
> +static int des_skcipher_setkey(struct crypto_skcipher *skcipher,
> +const u8 *key, unsigned int keylen)
> +{
> + u32 tmp[DES_EXPKEY_WORDS];
> +
> + if (!des_ekey(tmp, key) && (crypto_skcipher_get_flags(skcipher) &
CRYPTO_DEV_FSL_CAAM_CRYPTO_API_QI needs to select CRYPTO_DES,
such that des_ekey is available.
> + CRYPTO_TFM_REQ_FORBID_WEAK_KEYS)) {
> + crypto_skcipher_set_flags(skcipher,
> + CRYPTO_TFM_RES_WEAK_KEY);
> + return -EINVAL;
> + }
> +
> + return skcipher_setkey(skcipher, key, keylen, 0);
> }
>
> static int xts_skcipher_setkey(struct crypto_skcipher *skcipher, const u8
> *key,
[...]
> diff --git a/drivers/crypto/caam/caamalg_qi2.c
> b/drivers/crypto/caam/caamalg_qi2.c
> index bc370af..da4abf1 100644
> --- a/drivers/crypto/caam/caamalg_qi2.c
> +++ b/drivers/crypto/caam/caamalg_qi2.c
[...]
> @@ -817,10 +823,17 @@ static int rfc4106_setkey(struct crypto_aead *aead,
> {
> struct caam_ctx *ctx = crypto_aead_ctx(aead);
> struct device *dev = ctx->dev;
> + int ret;
>
> if (keylen < 4)
> return -EINVAL;
>
Same here, check_aes_keylen() handles this case.
> + ret = check_aes_keylen(keylen - 4);
> + if (ret) {
> + crypto_aead_set_flags(aead, CRYPTO_TFM_RES_BAD_KEY_LEN);
> + return ret;
> + }
> +
> print_hex_dump_debug("key in @" __stringify(__LINE__)": ",
>
Re: [PATCH v2 09/14] crypto: caam - keep both virtual and dma key addresses
On 7/19/2019 2:58 AM, Iuliana Prodan wrote:
> From: Horia Geantă
>
> Update alginfo struct to keep both virtual and dma key addresses,
> so that descriptors have them at hand.
> One example where this is needed is in the xcbc(aes) shared descriptors,
> which are updated in current patch.
> Another example is the upcoming fix for DKP.
>
> Signed-off-by: Horia Geantă
> ---
> drivers/crypto/caam/caamhash.c | 26 --
> drivers/crypto/caam/caamhash_desc.c | 5 ++---
> drivers/crypto/caam/caamhash_desc.h | 2 +-
> drivers/crypto/caam/desc_constr.h | 10 --
> 4 files changed, 19 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
> index 2ec4bad..14fdfa1 100644
> --- a/drivers/crypto/caam/caamhash.c
> +++ b/drivers/crypto/caam/caamhash.c
[...]
> @@ -508,6 +502,10 @@ static int axcbc_setkey(struct crypto_ahash *ahash,
> const u8 *key,
> memcpy(ctx->key, key, keylen);
> dma_sync_single_for_device(jrdev, ctx->key_dma, keylen, DMA_TO_DEVICE);
> ctx->adata.keylen = keylen;
> + /* key is loaded from memory for UPDATE and FINALIZE states */
> + ctx->adata.key_dma = ctx->key_dma;
> + /* key is immediate data for INIT and INITFINAL states */
> + ctx->adata.key_virt = ctx->key;
>
Now that it's possible to store both virtual and dma key addresses,
it would be more efficient to move these assignments .cra_init callback.
I'll submit the changes in v3.
Horia
[...]
> diff --git a/drivers/crypto/caam/desc_constr.h
> b/drivers/crypto/caam/desc_constr.h
> index 5988a26..8154174 100644
> --- a/drivers/crypto/caam/desc_constr.h
> +++ b/drivers/crypto/caam/desc_constr.h
> @@ -457,8 +457,8 @@ do { \
> * functions where it is used.
> * @keylen: length of the provided algorithm key, in bytes
> * @keylen_pad: padded length of the provided algorithm key, in bytes
> - * @key: address where algorithm key resides; virtual address if key_inline
> - * is true, dma (bus) address if key_inline is false.
> + * @key_dma: dma (bus) address where algorithm key resides
> + * @key_virt: virtual address where algorithm key resides
> * @key_inline: true - key can be inlined in the descriptor; false - key is
> * referenced by the descriptor
> */
> @@ -466,10 +466,8 @@ struct alginfo {
> u32 algtype;
> unsigned int keylen;
> unsigned int keylen_pad;
> - union {
> - dma_addr_t key_dma;
> - const void *key_virt;
> - };
> + dma_addr_t key_dma;
> + const void *key_virt;
> bool key_inline;
> };
>
>
Re: [PATCH 03/14] crypto: caam - update IV only when crypto operation succeeds
On 7/18/2019 5:45 PM, Iuliana Prodan wrote:
> From: Horia Geantă
>
> skcipher encryption might fail and in some cases, like (invalid) input
> length smaller then block size, updating the IV would lead to panic
> due to copying from a negative offset (req->cryptlen - ivsize).
>
The commit message is no longer in sync with the code base.
More exactly, after commit
334d37c9e263 ("crypto: caam - update IV using HW support")
there shouldn't be any panic, only a useless IV copy in case HW issued
an error.
> Signed-off-by: Horia Geantă
> Signed-off-by: Iuliana Prodan
> ---
> drivers/crypto/caam/caamalg.c | 5 ++---
> drivers/crypto/caam/caamalg_qi.c | 4 +++-
> drivers/crypto/caam/caamalg_qi2.c | 8 ++--
> 3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
> index 06b4f2d..28d55a0 100644
> --- a/drivers/crypto/caam/caamalg.c
> +++ b/drivers/crypto/caam/caamalg.c
> @@ -990,10 +990,9 @@ static void skcipher_encrypt_done(struct device *jrdev,
> u32 *desc, u32 err,
>* ciphertext block (CBC mode) or last counter (CTR mode).
>* This is used e.g. by the CTS mode.
>*/
> - if (ivsize) {
> + if (ivsize && !ecode) {
> memcpy(req->iv, (u8 *)edesc->sec4_sg + edesc->sec4_sg_bytes,
> ivsize);
> -
> print_hex_dump_debug("dstiv @"__stringify(__LINE__)": ",
>DUMP_PREFIX_ADDRESS, 16, 4, req->iv,
>edesc->src_nents > 1 ? 100 : ivsize, 1);
> @@ -1030,7 +1029,7 @@ static void skcipher_decrypt_done(struct device *jrdev,
> u32 *desc, u32 err,
>* ciphertext block (CBC mode) or last counter (CTR mode).
>* This is used e.g. by the CTS mode.
>*/
> - if (ivsize) {
> + if (ivsize && !ecode) {
> memcpy(req->iv, (u8 *)edesc->sec4_sg + edesc->sec4_sg_bytes,
> ivsize);
>
> diff --git a/drivers/crypto/caam/caamalg_qi.c
> b/drivers/crypto/caam/caamalg_qi.c
> index ab263b1..66531d6 100644
> --- a/drivers/crypto/caam/caamalg_qi.c
> +++ b/drivers/crypto/caam/caamalg_qi.c
> @@ -1201,7 +1201,9 @@ static void skcipher_done(struct caam_drv_req *drv_req,
> u32 status)
>* ciphertext block (CBC mode) or last counter (CTR mode).
>* This is used e.g. by the CTS mode.
>*/
> - memcpy(req->iv, (u8 *)&edesc->sgt[0] + edesc->qm_sg_bytes, ivsize);
> + if (!ecode)
> + memcpy(req->iv, (u8 *)&edesc->sgt[0] + edesc->qm_sg_bytes,
> +ivsize);
>
> qi_cache_free(edesc);
> skcipher_request_complete(req, ecode);
> diff --git a/drivers/crypto/caam/caamalg_qi2.c
> b/drivers/crypto/caam/caamalg_qi2.c
> index 2681581..bc370af 100644
> --- a/drivers/crypto/caam/caamalg_qi2.c
> +++ b/drivers/crypto/caam/caamalg_qi2.c
> @@ -1358,7 +1358,9 @@ static void skcipher_encrypt_done(void *cbk_ctx, u32
> status)
>* ciphertext block (CBC mode) or last counter (CTR mode).
>* This is used e.g. by the CTS mode.
>*/
> - memcpy(req->iv, (u8 *)&edesc->sgt[0] + edesc->qm_sg_bytes, ivsize);
> + if (!ecode)
> + memcpy(req->iv, (u8 *)&edesc->sgt[0] + edesc->qm_sg_bytes,
> +ivsize);
>
> qi_cache_free(edesc);
> skcipher_request_complete(req, ecode);
> @@ -1394,7 +1396,9 @@ static void skcipher_decrypt_done(void *cbk_ctx, u32
> status)
>* ciphertext block (CBC mode) or last counter (CTR mode).
>* This is used e.g. by the CTS mode.
>*/
> - memcpy(req->iv, (u8 *)&edesc->sgt[0] + edesc->qm_sg_bytes, ivsize);
> + if (!ecode)
> + memcpy(req->iv, (u8 *)&edesc->sgt[0] + edesc->qm_sg_bytes,
> +ivsize);
>
> qi_cache_free(edesc);
> skcipher_request_complete(req, ecode);
>
xts fuzz testing and lack of ciphertext stealing support
Hi, With fuzz testing enabled, I am seeing xts(aes) failures on caam drivers. Below are several failures, extracted from different runs: [3.921654] alg: skcipher: xts-aes-caam encryption unexpectedly succeeded on test vector "random: len=40 klen=64"; expected_error=-22, cfg="random: inplace use_finup nosimd src_divs=[57.93%@+11, 37.18%@+164, 0.68%@+4, 0.50%@+305, 3.71%@alignmask+3975]" [3.726698] alg: skcipher: xts-aes-caam encryption unexpectedly succeeded on test vector "random: len=369 klen=64"; expected_error=-22, cfg="random: inplace may_sleep use_digest src_divs=[100.0%@alignmask+584]" [3.741082] alg: skcipher: xts-aes-caam encryption unexpectedly succeeded on test vector "random: len=2801 klen=64"; expected_error=-22, cfg="random: inplace may_sleep use_digest src_divs=[100.0%@+6] iv_offset=18" It looks like the problem is not in CAAM driver. More exactly, fuzz testing is generating random test vectors and running them through both SW generic (crypto/xts.c) and CAAM implementation: -SW generic implementation of xts(aes) does not support ciphertext stealing and throws -EINVAL when input is not a multiple of AES block size (16B) -caam has support for ciphertext stealing, and allows for any input size which results in "unexpectedly succeeded" error messages. Any suggestion how this should be fixed? Thanks, Horia
Re: [PATCH] crypto: caam/qi2 - Add printing dpseci fq stats using debugfs
On 7/10/2019 1:13 PM, Vakul Garg wrote:
> Add support of printing the dpseci frame queue statistics using debugfs.
>
Please move this into a separate file, that gets compiled only if
CONFIG_DEBUG_FS=y.
Function(s) that are needed outside this file should be called unconditionally
and should have an empty version, in case CONFIG_DEBUG_FS=n.
> Signed-off-by: Vakul Garg
> ---
> drivers/crypto/caam/caamalg_qi2.c | 95
> +++
> drivers/crypto/caam/caamalg_qi2.h | 3 ++
> 2 files changed, 98 insertions(+)
>
> diff --git a/drivers/crypto/caam/caamalg_qi2.c
> b/drivers/crypto/caam/caamalg_qi2.c
> index 06bf32c32cbd..9414d2149b9a 100644
> --- a/drivers/crypto/caam/caamalg_qi2.c
> +++ b/drivers/crypto/caam/caamalg_qi2.c
> @@ -5009,6 +5009,87 @@ static int __cold dpaa2_dpseci_disable(struct
> dpaa2_caam_priv *priv)
> return 0;
> }
>
> +#ifdef CONFIG_DEBUG_FS
> +static int dpseci_dbg_fqs_show(struct seq_file *file, void *offset)
> +{
> + struct dpaa2_caam_priv *priv = (struct dpaa2_caam_priv *)file->private;
> + u32 fqid, fcnt, bcnt;
> + int i, err;
> +
> + seq_printf(file, "FQ stats for %s:\n", dev_name(priv->dev));
> + seq_printf(file, "%s%16s%16s\n",
> +"Rx-VFQID",
> +"Pending frames",
> +"Pending bytes");
> +
> + for (i = 0; i < priv->num_pairs; i++) {
> + fqid = priv->rx_queue_attr[i].fqid;
> + err = dpaa2_io_query_fq_count(NULL, fqid, &fcnt, &bcnt);
> + if (err)
> + continue;
> +
> + seq_printf(file, "%5d%16u%16u\n", fqid, fcnt, bcnt);
> + }
> +
> + seq_printf(file, "%s%16s%16s\n",
> +"Tx-VFQID",
> +"Pending frames",
> +"Pending bytes");
> +
> + for (i = 0; i < priv->num_pairs; i++) {
> + fqid = priv->tx_queue_attr[i].fqid;
> + err = dpaa2_io_query_fq_count(NULL, fqid, &fcnt, &bcnt);
> + if (err)
> + continue;
> +
> + seq_printf(file, "%5d%16u%16u\n", fqid, fcnt, bcnt);
> + }
> +
> + return 0;
> +}
> +
> +static int dpseci_dbg_fqs_open(struct inode *inode, struct file *file)
> +{
> + int err;
> + struct dpaa2_caam_priv *priv;
> +
> + priv = (struct dpaa2_caam_priv *)inode->i_private;
> +
> + err = single_open(file, dpseci_dbg_fqs_show, priv);
> + if (err < 0)
> + dev_err(priv->dev, "single_open() failed\n");
> +
> + return err;
> +}
> +
> +static const struct file_operations dpseci_dbg_fq_ops = {
> + .open = dpseci_dbg_fqs_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release = single_release,
> +};
> +
> +static int dpaa2_dpseci_debugfs_init(struct dpaa2_caam_priv *priv)
> +{
> + struct dentry *res;
> +
> + res = debugfs_create_dir(dev_name(priv->dev), NULL);
> + if (IS_ERR_OR_NULL(res))
> + return PTR_ERR(res);
> +
Error checking not needed.
See previous work done by GregKH to clean up debugfs-related error path.
> + priv->dfs_root = res;
> +
> + res = debugfs_create_file("fq_stats", 0444, res, priv,
> + &dpseci_dbg_fq_ops);
> + if (IS_ERR_OR_NULL(res)) {
> + debugfs_remove_recursive(priv->dfs_root);
> + return PTR_ERR(res);
> + }
> +
> + return 0;
> +}
> +#endif
> +
> static struct list_head hash_list;
>
> static int dpaa2_caam_probe(struct fsl_mc_device *dpseci_dev)
> @@ -5098,6 +5179,14 @@ static int dpaa2_caam_probe(struct fsl_mc_device
> *dpseci_dev)
> goto err_bind;
> }
>
> +#ifdef CONFIG_DEBUG_FS
> + err = dpaa2_dpseci_debugfs_init(priv);
> + if (err) {
> + dev_err(dev, "dpaa2_dpseci_debugfs_init() failed\n");
> + goto err_debugfs;
> + }
> +#endif
> +
> /* register crypto algorithms the device supports */
> for (i = 0; i < ARRAY_SIZE(driver_algs); i++) {
> struct caam_skcipher_alg *t_alg = driver_algs + i;
> @@ -5242,6 +5331,8 @@ static int dpaa2_caam_probe(struct fsl_mc_device
> *dpseci_dev)
>
> return err;
>
> +err_debugfs:
> + dpaa2_dpseci_disable(priv);
> err_bind:
> dpaa2_dpseci_dpio_free(priv);
> err_dpio_setup:
> @@ -5265,6 +5356,10 @@ static int __cold dpaa2_caam_remove(struct
> fsl_mc_device *ls_dev)
> dev = &ls_dev->dev;
> priv = dev_get_drvdata(dev);
>
> +#ifdef CONFIG_DEBUG_FS
> + debugfs_remove_recursive(priv->dfs_root);
> +#endif
> +
Please get rid of the ifdeffery, debugfs_remove_recursive() should be called
unconditionally.
> for (i = 0; i < ARRAY_SIZE(driver_aeads); i++) {
> struct caam_aead_alg *t_alg = driver_aeads + i;
>
> diff --git a/drivers/crypto/caam/caamalg_qi2.h
> b/drivers/crypto/caam/caamalg_qi2.h
> index 0f207b275578..74644d92f9d7 100644
> --- a/drivers/crypto/caam/caamalg_qi2.h
> +++ b/drivers/crypto/caam/caamalg_qi2.h
> @@ -64,6 +64,9
Re: [PATCH v4 03/16] crypto: caam - move tasklet_init() call down
On 7/4/2019 2:45 AM, Leonard Crestez wrote:
> On 7/3/2019 8:14 PM, Andrey Smirnov wrote:
>> On Wed, Jul 3, 2019 at 6:51 AM Leonard Crestez
>> wrote:
>>> On 7/3/2019 11:14 AM, Andrey Smirnov wrote:
Move tasklet_init() call further down in order to simplify error path
cleanup. No functional change intended.
diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
index 4b25b2fa3d02..a7ca2bbe243f 100644
--- a/drivers/crypto/caam/jr.c
+++ b/drivers/crypto/caam/jr.c
@@ -441,15 +441,13 @@ static int caam_jr_init(struct device *dev)
jrp = dev_get_drvdata(dev);
- tasklet_init(&jrp->irqtask, caam_jr_dequeue, (unsigned long)dev);
-
/* Connect job ring interrupt handler. */
error = request_irq(jrp->irq, caam_jr_interrupt, IRQF_SHARED,
dev_name(dev), dev);
if (error) {
dev_err(dev, "can't connect JobR %d interrupt (%d)\n",
jrp->ridx, jrp->irq);
- goto out_kill_deq;
+ return error;
}
>>>
>>> The caam_jr_interrupt handler can schedule the tasklet so it makes sense
>>> to have it be initialized ahead of request_irq. In theory it's possible
>>> for an interrupt to be triggered immediately when request_irq is called.
>>>
>>> I'm not very familiar with the CAAM ip, can you ensure no interrupts are
>>> pending in HW at probe time? The "no functional change" part is not obvious.
>>>
Actually there is a previous report (in i.MX BSP) of CAAM job ring generating
an interrupt at probe time, between request_irq() and reset:
https://source.codeaurora.org/external/imx/linux-imx/commit/drivers/crypto/caam?h=imx_4.14.98_2.0.0_ga&id=aa7b3f51971ec1f60f41fe8ea71870b215376b8a
So yes, there might be cases when interrupts are pending.
>>
>> Said tasklet will use both jrp->outring and jrp->entinfo array
>> initialized after IRQ request call in both versions of the code
>> (before/after this patch). AFAICT, the only case where this patch
>> would change initialization safety of the original code is if JR was
>> scheduled somehow while ORSFx is 0 (no jobs done), which I don't think
>> is possible.
>
> I took a second look at caam_jr_init and there is apparently a whole
> bunch of other reset/init stuff done after request_irq. For example
> caam_reset_hw_jr is done after request_irq and masks interrupts?
>
> What I'd expect is that request_irq is done last after all other
> initialization is performed. But I'm not familiar with how CAAM JRs work
> so feel free to ignore this.
>
There's some history here... (which is in contradiction with above-mentioned
report).
Commit 9620fd959fb1 ("crypto: caam - handle interrupt lines shared across
rings")
moved request_irq() before JR reset:
"
- resetting a job ring triggers an interrupt, so move request_irq
prior to jr_reset to avoid 'got IRQ but nobody cared' messages.
"
but IIUC that ("resetting a job ring triggers an interrupt") was actually
due to disabling the IRQ line using disable_irq() instead of masking
the interrupt in HW using JRCFGR_LS[IMSK].
The way JR reset sequence is implemented now - in caam_reset_hw_jr() - should
not
trigger any interrupt.
Thus, it should be safe to move request_irq() after everything is set up,
at the end of probing.
My suggestion is to move both tasklet_init() and request_irq() at the bottom
of the probe callback.
However, I'd say this is a fix that should be marked accordingly and
should be posted either separately, or at the top of the patch set.
Thanks,
Horia
Re: [PATCH v4 05/16] crypto: caam - use devres to allocate 'outring'
On 7/3/2019 11:14 AM, Andrey Smirnov wrote:
> Use devres to allocate 'outring' and drop corresponding call to
> dma_free_coherent() as well as extra references to 'struct
> jr_outentry' (needed in following commits). No functional change
> inteded.
>
> Signed-off-by: Andrey Smirnov
> Cc: Chris Spencer
> Cc: Cory Tusar
> Cc: Chris Healy
> Cc: Lucas Stach
> Cc: Horia Geantă
> Cc: Aymen Sghaier
> Cc: Leonard Crestez
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> ---
> drivers/crypto/caam/jr.c | 15 +--
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c
> index fc7deb445aa8..1eaa91dcc146 100644
> --- a/drivers/crypto/caam/jr.c
> +++ b/drivers/crypto/caam/jr.c
> @@ -108,7 +108,7 @@ static int caam_reset_hw_jr(struct device *dev)
> static int caam_jr_shutdown(struct device *dev)
> {
> struct caam_drv_private_jr *jrp = dev_get_drvdata(dev);
> - dma_addr_t inpbusaddr, outbusaddr;
> + dma_addr_t inpbusaddr;
> int ret;
>
> ret = caam_reset_hw_jr(dev);
> @@ -120,11 +120,8 @@ static int caam_jr_shutdown(struct device *dev)
>
> /* Free rings */
> inpbusaddr = rd_reg64(&jrp->rregs->inpring_base);
> - outbusaddr = rd_reg64(&jrp->rregs->outring_base);
> dma_free_coherent(dev, sizeof(dma_addr_t) * JOBR_DEPTH,
> jrp->inpring, inpbusaddr);
> - dma_free_coherent(dev, sizeof(struct jr_outentry) * JOBR_DEPTH,
> - jrp->outring, outbusaddr);
>
> return ret;
> }
> @@ -459,15 +456,16 @@ static int caam_jr_init(struct device *dev)
> if (!jrp->inpring)
> goto out_free_irq;
>
> - jrp->outring = dma_alloc_coherent(dev, sizeof(*jrp->outring) *
> - JOBR_DEPTH, &outbusaddr, GFP_KERNEL);
> + jrp->outring = dmam_alloc_coherent(dev, sizeof(*jrp->outring) *
> +JOBR_DEPTH, &outbusaddr,
> +GFP_KERNEL);
> if (!jrp->outring)
> goto out_free_inpring;
>
> jrp->entinfo = devm_kcalloc(dev, JOBR_DEPTH, sizeof(*jrp->entinfo),
> GFP_KERNEL);
> if (!jrp->entinfo)
> - goto out_free_outring;
> + return -ENOMEM;
>
This is going to leak resources, so should instead be:
goto out_free_inpring;
I suggest merging patches 4, 5, 6.
They have the same goal, are rather small and make changes in the same places,
and would avoid issues like this one.
> tasklet_init(&jrp->irqtask, caam_jr_dequeue, (unsigned long)dev);
>
> @@ -495,9 +493,6 @@ static int caam_jr_init(struct device *dev)
>
> return 0;
>
> -out_free_outring:
> - dma_free_coherent(dev, sizeof(struct jr_outentry) * JOBR_DEPTH,
> - jrp->outring, outbusaddr);
> out_free_inpring:
> dma_free_coherent(dev, sizeof(dma_addr_t) * JOBR_DEPTH,
> jrp->inpring, inpbusaddr);
>
Re: [PATCH v2 06/35] crypto: Use kmemdup rather than duplicating its implementation
On 7/3/2019 7:27 PM, Fuqian Huang wrote: > kmemdup is introduced to duplicate a region of memory in a neat way. > Rather than kmalloc/kzalloc + memcpy, which the programmer needs to > write the size twice (sometimes lead to mistakes), kmemdup improves > readability, leads to smaller code and also reduce the chances of mistakes. > Suggestion to use kmemdup rather than using kmalloc/kzalloc + memcpy. > > Signed-off-by: Fuqian Huang Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH v3 06/30] crypto: caam/des - switch to new verification routines
On 6/28/2019 12:35 PM, Ard Biesheuvel wrote: > Signed-off-by: Ard Biesheuvel Tested-by: Horia Geantă Thanks, Horia
Re: [PATCH v2 00/30] crypto: DES/3DES cleanup
On 6/27/2019 5:54 PM, Ard Biesheuvel wrote:
> On Thu, 27 Jun 2019 at 16:50, Ard Biesheuvel
> wrote:
>>
>> On Thu, 27 Jun 2019 at 16:44, Horia Geanta wrote:
>>>
>>> On 6/27/2019 3:03 PM, Ard Biesheuvel wrote:
>>>> n my effort to remove crypto_alloc_cipher() invocations from non-crypto
>>>> code, i ran into a DES call in the CIFS driver. This is addressed in
>>>> patch #30.
>>>>
>>>> The other patches are cleanups for the quirky DES interface, and lots
>>>> of duplication of the weak key checks etc.
>>>>
>>>> Changes since
>>>> vpub/scm/linux/kernel/git/arm64/linux.git/log/?h=for-next/fixes1/RFC:
>>>> - fix build errors in various drivers that i failed to catch in my
>>>> initial testing
>>>> - put all caam changes into the correct patch
>>>> - fix weak key handling error flagged by the self tests, as reported
>>>> by Eric.
>>> I am seeing a similar (?) issue:
>>> alg: skcipher: ecb-des-caam setkey failed on test vector 4;
>>> expected_error=-22, actual_error=-126, flags=0x100101
>>>
>>> crypto_des_verify_key() in include/crypto/internal/des.h returns -ENOKEY,
>>> while testmgr expects -EINVAL (setkey_error = -EINVAL in the test vector).
>>>
>>> I assume crypto_des_verify_key() should return -EINVAL, not -ENOKEY.
>>>
>>
>> Yes, but I tried to keep handling of the crypto_tfm flags out of the
>> library interface.
>>
>> I will try to find a way to solve this cleanly.
>
> Actually, it should be as simple as
>
> diff --git a/include/crypto/internal/des.h b/include/crypto/internal/des.h
> index dfe5e8f92270..522c09c08002 100644
> --- a/include/crypto/internal/des.h
> +++ b/include/crypto/internal/des.h
> @@ -27,10 +27,12 @@ static inline int crypto_des_verify_key(struct
> crypto_tfm *tfm, const u8 *key)
> int err;
>
> err = des_expand_key(&tmp, key, DES_KEY_SIZE);
> - if (err == -ENOKEY &&
> - !(crypto_tfm_get_flags(tfm) & CRYPTO_TFM_REQ_FORBID_WEAK_KEYS))
> - err = 0;
> -
> + if (err == -ENOKEY) {
> + if (crypto_tfm_get_flags(tfm) &
> CRYPTO_TFM_REQ_FORBID_WEAK_KEYS)
> + err = -EINVAL;
> + else
> + err = 0;
> + }
> if (err)
> crypto_tfm_set_flags(tfm, CRYPTO_TFM_RES_WEAK_KEY);
>
Confirming this works for me (on CAAM accelerator).
Thanks,
Horia
Re: [PATCH v2 00/30] crypto: DES/3DES cleanup
On 6/27/2019 3:03 PM, Ard Biesheuvel wrote: > n my effort to remove crypto_alloc_cipher() invocations from non-crypto > code, i ran into a DES call in the CIFS driver. This is addressed in > patch #30. > > The other patches are cleanups for the quirky DES interface, and lots > of duplication of the weak key checks etc. > > Changes since v1/RFC: > - fix build errors in various drivers that i failed to catch in my > initial testing > - put all caam changes into the correct patch > - fix weak key handling error flagged by the self tests, as reported > by Eric. I am seeing a similar (?) issue: alg: skcipher: ecb-des-caam setkey failed on test vector 4; expected_error=-22, actual_error=-126, flags=0x100101 crypto_des_verify_key() in include/crypto/internal/des.h returns -ENOKEY, while testmgr expects -EINVAL (setkey_error = -EINVAL in the test vector). I assume crypto_des_verify_key() should return -EINVAL, not -ENOKEY. Horia
Re: [PATCH v2 06/30] crypto: caam/des - switch to new verification routines
On 6/27/2019 3:03 PM, Ard Biesheuvel wrote:
> @@ -785,20 +781,23 @@ static int skcipher_setkey(struct crypto_skcipher
> *skcipher, const u8 *key,
> static int des_skcipher_setkey(struct crypto_skcipher *skcipher,
> const u8 *key, unsigned int keylen)
> {
> - u32 tmp[DES3_EDE_EXPKEY_WORDS];
> - struct crypto_tfm *tfm = crypto_skcipher_tfm(skcipher);
> + int err;
>
> - if (keylen == DES3_EDE_KEY_SIZE &&
> - __des3_ede_setkey(tmp, &tfm->crt_flags, key, DES3_EDE_KEY_SIZE)) {
> - return -EINVAL;
> - }
> + err = crypto_des_verify_key(crypto_skcipher_tfm(skcipher), key);
> + if (unlikely(err))
> + return err;
>
> - if (!des_ekey(tmp, key) && (crypto_skcipher_get_flags(skcipher) &
> - CRYPTO_TFM_REQ_FORBID_WEAK_KEYS)) {
> - crypto_skcipher_set_flags(skcipher,
> - CRYPTO_TFM_RES_WEAK_KEY);
> - return -EINVAL;
> - }
> + return skcipher_setkey(skcipher, key, keylen);
This would be a bit more compact:
return unlikely(crypto_des_verify_key(crypto_skcipher_tfm(skcipher),
key)) ?:
skcipher_setkey(skcipher, key, keylen);
and could be used in most places.
Actually here:
> @@ -697,8 +693,13 @@ static int skcipher_setkey(struct crypto_skcipher
> *skcipher, const u8 *key,
> static int des3_skcipher_setkey(struct crypto_skcipher *skcipher,
> const u8 *key, unsigned int keylen)
> {
> - return unlikely(des3_verify_key(skcipher, key)) ?:
> -skcipher_setkey(skcipher, key, keylen);
> + int err;
> +
> + err = crypto_des3_ede_verify_key(crypto_skcipher_tfm(skcipher), key);
> + if (unlikely(err))
> + return err;
> +
> + return skcipher_setkey(skcipher, key, keylen);
> }
this pattern is already used, only the verification function
has to be replaced.
Horia
Re: [PATCH v3 1/5] crypto: caam - move DMA mask selection into a function
On 6/17/2019 7:04 PM, Andrey Smirnov wrote: > Exactly the same code to figure out DMA mask is repeated twice in the > driver code. To avoid repetition, move that logic into a standalone > subroutine in intern.h. While at it re-shuffle the code to make it > more readable with early returns. > > Signed-off-by: Andrey Smirnov > Cc: Chris Spencer > Cc: Cory Tusar > Cc: Chris Healy > Cc: Lucas Stach > Cc: Horia Geantă > Cc: Aymen Sghaier > Cc: Leonard Crestez > Cc: linux-crypto@vger.kernel.org > Cc: linux-ker...@vger.kernel.org Reviewed-by: Horia Geantă Being the 1st patch in the series and not i.MX8-specific, I'd say it should be merged separately. Thanks, Horia
Re: [RFC PATCH 06/30] crypto: caam/des - switch to new verification routines
On 6/22/2019 3:32 AM, Ard Biesheuvel wrote:
> Signed-off-by: Ard Biesheuvel
> ---
> drivers/crypto/caam/caamalg.c | 13 +++
> drivers/crypto/caam/caamalg_qi.c | 23 ++--
> drivers/crypto/caam/caamalg_qi2.c | 23 ++--
> drivers/crypto/caam/compat.h | 2 +-
> 4 files changed, 26 insertions(+), 35 deletions(-)
>
Compiling the patch set, I get the following errors:
drivers/crypto/caam/caamalg.c: In function 'des3_aead_setkey':
drivers/crypto/caam/caamalg.c:642:51: error: 'tfm' undeclared (first use in
this function)
err = crypto_des3_ede_verify_key(crypto_aead_tfm(tfm), keys.enckey,
^
drivers/crypto/caam/caamalg.c:642:51: note: each undeclared identifier is
reported only once for each function it appears in
drivers/crypto/caam/caamalg.c: In function 'des_skcipher_setkey':
drivers/crypto/caam/caamalg.c:783:2: error: implicit declaration of function
'des_verify_key' [-Werror=implicit-function-declaration]
err = des_verify_key(crypto_skcipher_tfm(skcipher), key, keylen);
^
drivers/crypto/caam/caamalg.c: In function 'des3_skcipher_setkey':
drivers/crypto/caam/caamalg.c:795:28: warning: passing argument 1 of
'des3_ede_verify_key' from incompatible pointer type
err = des3_ede_verify_key(crypto_skcipher_tfm(skcipher), key, keylen);
^
In file included from drivers/crypto/caam/compat.h:35:0,
from drivers/crypto/caam/caamalg.c:49:
./include/crypto/internal/des.h:49:19: note: expected 'const u8 *' but argument
is of type 'struct crypto_tfm *'
static inline int des3_ede_verify_key(const u8 *key, unsigned int key_len,
^
drivers/crypto/caam/caamalg.c:795:59: warning: passing argument 2 of
'des3_ede_verify_key' makes integer from pointer without a cast
err = des3_ede_verify_key(crypto_skcipher_tfm(skcipher), key, keylen);
^
In file included from drivers/crypto/caam/compat.h:35:0,
from drivers/crypto/caam/caamalg.c:49:
./include/crypto/internal/des.h:49:19: note: expected 'unsigned int' but
argument is of type 'const u8 *'
static inline int des3_ede_verify_key(const u8 *key, unsigned int key_len,
^
> diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
> index 5d4fa65a015f..b4ab64146b21 100644
> --- a/drivers/crypto/caam/caamalg.c
> +++ b/drivers/crypto/caam/caamalg.c
> @@ -633,23 +633,16 @@ static int des3_aead_setkey(struct crypto_aead *aead,
> const u8 *key,
> unsigned int keylen)
> {
> struct crypto_authenc_keys keys;
> - u32 flags;
> int err;
>
> err = crypto_authenc_extractkeys(&keys, key, keylen);
> if (unlikely(err))
> goto badkey;
>
> - err = -EINVAL;
> - if (keys.enckeylen != DES3_EDE_KEY_SIZE)
> - goto badkey;
> -
> - flags = crypto_aead_get_flags(aead);
> - err = __des3_verify_key(&flags, keys.enckey);
> - if (unlikely(err)) {
> - crypto_aead_set_flags(aead, flags);
> + err = crypto_des3_ede_verify_key(crypto_aead_tfm(tfm), keys.enckey,
> + keys.enckeylen);
> + if (unlikely(err))
> goto out;
> - }
>
> err = aead_setkey(aead, key, keylen);
>
> diff --git a/drivers/crypto/caam/caamalg_qi.c
> b/drivers/crypto/caam/caamalg_qi.c
> index 32f0f8a72067..01d92ef0142a 100644
> --- a/drivers/crypto/caam/caamalg_qi.c
> +++ b/drivers/crypto/caam/caamalg_qi.c
> @@ -296,23 +296,16 @@ static int des3_aead_setkey(struct crypto_aead *aead,
> const u8 *key,
> unsigned int keylen)
> {
> struct crypto_authenc_keys keys;
> - u32 flags;
> int err;
>
> err = crypto_authenc_extractkeys(&keys, key, keylen);
> if (unlikely(err))
> goto badkey;
>
> - err = -EINVAL;
> - if (keys.enckeylen != DES3_EDE_KEY_SIZE)
> - goto badkey;
> -
> - flags = crypto_aead_get_flags(aead);
> - err = __des3_verify_key(&flags, keys.enckey);
> - if (unlikely(err)) {
> - crypto_aead_set_flags(aead, flags);
> + err = crypto_des3_ede_verify_key(crypto_aead_tfm(aead), keys.enckey,
> + keys.enckeylen);
> + if (unlikely(err))
> goto out;
> - }
>
> err = aead_setkey(aead, key, keylen);
>
> @@ -697,8 +690,14 @@ static int skcipher_setkey(struct crypto_skcipher
> *skcipher, const u8 *key,
> static int des3_skcipher_setkey(struct crypto_skcipher *skcipher,
> const u8 *key, unsigned int keylen)
> {
> - return unlikely(des3_verify_key(skcipher, key)) ?:
> -skcipher_setkey(skcipher, key, keylen);
> + int err;
> +
> + err = crypto_des3_ede_verify_key(crypto_skcipher_tfm(skcipher), key,
> + keylen);
> + if
Re: [PATCH] crypto: caam - fix dependency on CRYPTO_DES
On 6/27/2019 12:12 PM, Ard Biesheuvel wrote:
> On Thu, 27 Jun 2019 at 11:10, Horia Geanta wrote:
>>
>> (changed subject to make patchwork happy
>> was: [RFC PATCH 27/30] crypto: des - split off DES library from generic DES
>> cipher driver)
>>
>> On 6/22/2019 3:32 AM, Ard Biesheuvel wrote:
>>> diff --git a/drivers/crypto/caam/Kconfig b/drivers/crypto/caam/Kconfig
>>> index 3720ddabb507..4a358391b6cb 100644
>>> --- a/drivers/crypto/caam/Kconfig
>>> +++ b/drivers/crypto/caam/Kconfig
>>> @@ -98,7 +98,7 @@ config CRYPTO_DEV_FSL_CAAM_CRYPTO_API
>>> select CRYPTO_AEAD
>>> select CRYPTO_AUTHENC
>>> select CRYPTO_BLKCIPHER
>>> - select CRYPTO_DES
>>> + select CRYPTO_LIB_DES
>>> help
>>> Selecting this will offload crypto for users of the
>>> scatterlist crypto API (such as the linux native IPSec
>>
>> There are two other config symbols that should select CRYPTO_LIB_DES:
>> CRYPTO_DEV_FSL_CAAM_CRYPTO_API_QI
>> CRYPTO_DEV_FSL_DPAA2_CAAM
>>
>> True, this is not stricty related to refactoring in this patch set,
>> but actually a fix of
>> commit 1b52c40919e6 ("crypto: caam - Forbid 2-key 3DES in FIPS mode")
>>
>
> The 3des key checks are static inline functions defined in des.h, so
> there is no need to depend on the library or on the generic driver
> AFAICT
>
True, des3_verify_key and __des3_verify_key are in des.h.
Please ignore this.
Thanks,
Horia
Re: [RFC PATCH 05/30] crypto: bcm/des - switch to new verification routines
On 6/22/2019 3:31 AM, Ard Biesheuvel wrote: > Signed-off-by: Ard Biesheuvel > --- > drivers/crypto/bcm/cipher.c | 82 +--- > drivers/crypto/caam/caamalg.c | 31 > 2 files changed, 37 insertions(+), 76 deletions(-) > Typo: caam changes should be part of next patch (06/30). Horia
[PATCH] crypto: caam - fix dependency on CRYPTO_DES
(changed subject to make patchwork happy
was: [RFC PATCH 27/30] crypto: des - split off DES library from generic DES
cipher driver)
On 6/22/2019 3:32 AM, Ard Biesheuvel wrote:
> diff --git a/drivers/crypto/caam/Kconfig b/drivers/crypto/caam/Kconfig
> index 3720ddabb507..4a358391b6cb 100644
> --- a/drivers/crypto/caam/Kconfig
> +++ b/drivers/crypto/caam/Kconfig
> @@ -98,7 +98,7 @@ config CRYPTO_DEV_FSL_CAAM_CRYPTO_API
> select CRYPTO_AEAD
> select CRYPTO_AUTHENC
> select CRYPTO_BLKCIPHER
> - select CRYPTO_DES
> + select CRYPTO_LIB_DES
> help
> Selecting this will offload crypto for users of the
> scatterlist crypto API (such as the linux native IPSec
There are two other config symbols that should select CRYPTO_LIB_DES:
CRYPTO_DEV_FSL_CAAM_CRYPTO_API_QI
CRYPTO_DEV_FSL_DPAA2_CAAM
True, this is not stricty related to refactoring in this patch set,
but actually a fix of
commit 1b52c40919e6 ("crypto: caam - Forbid 2-key 3DES in FIPS mode")
I am adding a fix inline.
Herbert, I think it would be better to apply it separately.
-- >8 --
Fix caam/qi and caam/qi2 dependency on CRYPTO_DES, introduced by
commit strengthening 3DES key checks.
Fixes: 1b52c40919e6 ("crypto: caam - Forbid 2-key 3DES in FIPS mode")
Signed-off-by: Horia Geantă
---
drivers/crypto/caam/Kconfig | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/crypto/caam/Kconfig b/drivers/crypto/caam/Kconfig
index 3720ddabb507..524b961360d2 100644
--- a/drivers/crypto/caam/Kconfig
+++ b/drivers/crypto/caam/Kconfig
@@ -111,6 +111,7 @@ config CRYPTO_DEV_FSL_CAAM_CRYPTO_API_QI
select CRYPTO_DEV_FSL_CAAM_CRYPTO_API_DESC
select CRYPTO_AUTHENC
select CRYPTO_BLKCIPHER
+ select CRYPTO_DES
help
Selecting this will use CAAM Queue Interface (QI) for sending
& receiving crypto jobs to/from CAAM. This gives better performance
@@ -158,6 +159,7 @@ config CRYPTO_DEV_FSL_DPAA2_CAAM
select CRYPTO_DEV_FSL_CAAM_CRYPTO_API_DESC
select CRYPTO_DEV_FSL_CAAM_AHASH_API_DESC
select CRYPTO_BLKCIPHER
+ select CRYPTO_DES
select CRYPTO_AUTHENC
select CRYPTO_AEAD
select CRYPTO_HASH
--
2.17.1
Re: [PATCH v3 2/4] crypto: talitos - fix hash on SEC1.
On 6/13/2019 3:48 PM, Christophe Leroy wrote:
> @@ -336,15 +336,18 @@ static void flush_channel(struct device *dev, int ch,
> int error, int reset_ch)
> tail = priv->chan[ch].tail;
> while (priv->chan[ch].fifo[tail].desc) {
> __be32 hdr;
> + struct talitos_edesc *edesc;
>
> request = &priv->chan[ch].fifo[tail];
> + edesc = container_of(request->desc, struct talitos_edesc, desc);
Not needed for all cases, should be moved to the block that uses it.
>
> /* descriptors with their done bits set don't get the error */
> rmb();
> if (!is_sec1)
> hdr = request->desc->hdr;
> else if (request->desc->next_desc)
> - hdr = (request->desc + 1)->hdr1;
> + hdr = ((struct talitos_desc *)
> +(edesc->buf + edesc->dma_len))->hdr1;
> else
> hdr = request->desc->hdr1;
>
[snip]
> @@ -2058,7 +2065,18 @@ static int ahash_process_req(struct ahash_request
> *areq, unsigned int nbytes)
> sg_copy_to_buffer(areq->src, nents,
> ctx_buf + req_ctx->nbuf, offset);
> req_ctx->nbuf += offset;
> - req_ctx->psrc = areq->src;
> + for (sg = areq->src; sg && offset >= sg->length;
> + offset -= sg->length, sg = sg_next(sg))
> + ;
> + if (offset) {
> + sg_init_table(req_ctx->bufsl, 2);
> + sg_set_buf(req_ctx->bufsl, sg_virt(sg) + offset,
> +sg->length - offset);
> + sg_chain(req_ctx->bufsl, 2, sg_next(sg));
> + req_ctx->psrc = req_ctx->bufsl;
Isn't this what scatterwalk_ffwd() does?
Horia
Re: [PATCH v3 2/4] crypto: talitos - fix hash on SEC1.
On 6/13/2019 3:48 PM, Christophe Leroy wrote:
> On SEC1, hash provides wrong result when performing hashing in several
> steps with input data SG list has more than one element. This was
> detected with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>
> [ 44.185947] alg: hash: md5-talitos test failed (wrong result) on test
> vector 6, cfg="random: may_sleep use_finup src_divs=[25.88%@+8063,
> 24.19%@+9588, 28.63%@+16333, 4.60%@+6756, 16.70%@+16281]
> dst_divs=[71.61%@alignmask+16361, 14.36%@+7756, 14.3%@+"
> [ 44.325122] alg: hash: sha1-talitos test failed (wrong result) on test
> vector 3, cfg="random: inplace use_final
> src_divs=[16.56%@+16378, 52.0%@+16329,
> 21.42%@alignmask+16380, 10.2%@alignmask+16380] iv_offset=39"
> [ 44.493500] alg: hash: sha224-talitos test failed (wrong result) on test
> vector 4, cfg="random: use_final nosimd src_divs=[52.27%@+7401,
> 17.34%@+16285, 17.71%@+26, 12.68%@+10644] iv_offset=43"
> [ 44.673262] alg: hash: sha256-talitos test failed (wrong result) on test
> vector 4, cfg="random: may_sleep use_finup src_divs=[60.6%@+12790,
> 17.86%@+1329, 12.64%@alignmask+16300, 8.29%@+15, 0.40%@+13506,
> 0.51%@+16322, 0.24%@+16339] dst_divs"
>
> This is due to two issues:
> - We have an overlap between the buffer used for copying the input
> data (SEC1 doesn't do scatter/gather) and the chained descriptor.
> - Data copy is wrong when the previous hash left less than one
> blocksize of data to hash, implying a complement of the previous
> block with a few bytes from the new request.
>
I fail to spot these issues.
IIUC in case of SEC1, the variable part of talitos_edesc structure contains
a 2nd "chained" descriptor (talitos_desc struct) followed by an area
dedicated to linearizing the input (in case input is scattered).
Where is the overlap?
> Fix it by:
> - Moving the second descriptor after the buffer, as moving the buffer
> after the descriptor would make it more complex for other cipher
> operations (AEAD, ABLKCIPHER)
> - Rebuiding a new data SG list without the bytes taken from the new
> request to complete the previous one.
>
> Preceding patch ("crypto: talitos - move struct talitos_edesc into
> talitos.h") as required for this change to build properly.
>
> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on
> SEC1")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Christophe Leroy
> ---
> drivers/crypto/talitos.c | 63
> ++--
> 1 file changed, 40 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
> index 5b401aec6c84..4f03baef952b 100644
> --- a/drivers/crypto/talitos.c
> +++ b/drivers/crypto/talitos.c
> @@ -336,15 +336,18 @@ static void flush_channel(struct device *dev, int ch,
> int error, int reset_ch)
> tail = priv->chan[ch].tail;
> while (priv->chan[ch].fifo[tail].desc) {
> __be32 hdr;
> + struct talitos_edesc *edesc;
>
> request = &priv->chan[ch].fifo[tail];
> + edesc = container_of(request->desc, struct talitos_edesc, desc);
>
> /* descriptors with their done bits set don't get the error */
> rmb();
> if (!is_sec1)
> hdr = request->desc->hdr;
> else if (request->desc->next_desc)
> - hdr = (request->desc + 1)->hdr1;
> + hdr = ((struct talitos_desc *)
> +(edesc->buf + edesc->dma_len))->hdr1;
> else
> hdr = request->desc->hdr1;
>
> @@ -476,8 +479,14 @@ static u32 current_desc_hdr(struct device *dev, int ch)
> }
> }
>
> - if (priv->chan[ch].fifo[iter].desc->next_desc == cur_desc)
> - return (priv->chan[ch].fifo[iter].desc + 1)->hdr;
> + if (priv->chan[ch].fifo[iter].desc->next_desc == cur_desc) {
> + struct talitos_edesc *edesc;
> +
> + edesc = container_of(priv->chan[ch].fifo[iter].desc,
> + struct talitos_edesc, desc);
> + return ((struct talitos_desc *)
> + (edesc->buf + edesc->dma_len))->hdr;
> + }
>
> return priv->chan[ch].fifo[iter].desc->hdr;
> }
> @@ -1402,15 +1411,11 @@ static struct talitos_edesc
> *talitos_edesc_alloc(struct device *dev,
> edesc->dst_nents = dst_nents;
> edesc->iv_dma = iv_dma;
> edesc->dma_len = dma_len;
> - if (dma_len) {
> - void *addr = &edesc->link_tbl[0];
> -
> - if (is_sec1 && !dst)
> - addr += sizeof(struct talitos_desc);
> - edesc->dma_link_tbl = dma_map_single(dev, addr,
> + if (dma_len)
> + edesc->dma_link_tbl = dma_map_single(dev, &edesc->link_tbl[0],
>edesc->dma_len,
>DMA_BIDIRECTIONAL);
> - }
> +
> return ede
Re: [PATCH v2 1/4] crypto: talitos - move struct talitos_edesc into talitos.h
On 6/11/2019 5:39 PM, Christophe Leroy wrote:
> Next patch will require struct talitos_edesc to be defined
> earlier in talitos.c
>
> This patch moves it into talitos.h so that it can be used
> from any place in talitos.c
>
> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on
> SEC1")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Christophe Leroy
Again, this patch does not qualify as a fix.
Horia
Re: [PATCH v2 1/4] crypto: talitos - move struct talitos_edesc into talitos.h
On 6/13/2019 3:16 PM, Christophe Leroy wrote:
>
>
> Le 13/06/2019 à 14:13, Horia Geanta a écrit :
>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>> Next patch will require struct talitos_edesc to be defined
>>> earlier in talitos.c
>>>
>>> This patch moves it into talitos.h so that it can be used
>>> from any place in talitos.c
>>>
>>> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on
>>> SEC1")
>>> Cc: sta...@vger.kernel.org
>>> Signed-off-by: Christophe Leroy
>> Again, this patch does not qualify as a fix.
>>
>
> But as I said, the following one is a fix and require that one, you told
> me to add stable in Cc: to make it explicit it was to go into stable.
Yes, but you should remove the Fixes tag.
And probably replace "Next patch" with the commit headline.
> If someone tries to merge following one into stable with taking that one
> first, build will fail.
This shouldn't happen, order from main tree should be preserved.
Horia
Re: [PATCH v2 4/4] crypto: talitos - drop icv_ool
On 6/11/2019 5:39 PM, Christophe Leroy wrote:
> icv_ool is not used anymore, drop it.
>
> Fixes: 9cc87bc3613b ("crypto: talitos - fix AEAD processing")
I can't find this SHA1.
Are you referring to commit e345177ded17 ("crypto: talitos - fix AEAD
processing.")?
Horia
Re: [PATCH v2 3/4] crypto: talitos - eliminate unneeded 'done' functions at build time
On 6/11/2019 5:39 PM, Christophe Leroy wrote: > When building for SEC1 only, talitos2_done functions are unneeded > and should go away. > > For this, use has_ftr_sec1() which will always return true when only > SEC1 support is being built, allowing GCC to drop TALITOS2 functions. > > Signed-off-by: Christophe Leroy Reviewed-by: Horia Geantă Thanks, Horia
Re: [PATCH v2 1/4] crypto: talitos - move struct talitos_edesc into talitos.h
On 6/13/2019 3:32 PM, Christophe Leroy wrote:
>
>
> Le 13/06/2019 à 14:24, Horia Geanta a écrit :
>> On 6/13/2019 3:16 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 13/06/2019 à 14:13, Horia Geanta a écrit :
>>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>>> Next patch will require struct talitos_edesc to be defined
>>>>> earlier in talitos.c
>>>>>
>>>>> This patch moves it into talitos.h so that it can be used
>>>>> from any place in talitos.c
>>>>>
>>>>> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash
>>>>> on SEC1")
>>>>> Cc: sta...@vger.kernel.org
>>>>> Signed-off-by: Christophe Leroy
>>>> Again, this patch does not qualify as a fix.
>>>>
>>>
>>> But as I said, the following one is a fix and require that one, you told
>>> me to add stable in Cc: to make it explicit it was to go into stable.
>> Yes, but you should remove the Fixes tag.
>> And probably replace "Next patch" with the commit headline.
>>
>>> If someone tries to merge following one into stable with taking that one
>>> first, build will fail.
>> This shouldn't happen, order from main tree should be preserved.
>>
>
> When they pick up fixes, AFAIK they don't take all the preceeding commits.
>
This is not about Fixes tag, but Cc tag:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#option-1
Horia
Re: [PATCH v2 0/4] Additional fixes on Talitos driver
On 6/12/2019 8:52 AM, Christophe Leroy wrote:
>
>
> Le 11/06/2019 à 18:30, Horia Geanta a écrit :
>> On 6/11/2019 6:40 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 11/06/2019 à 17:37, Horia Geanta a écrit :
>>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>>> This series is the last set of fixes for the Talitos driver.
>>>>>
>>>>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>>>>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>>>>
>>>> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
>>>> hmac(sha512):
>>>
>>> Is that new with this series or did you already have it before ?
>>>
>> Looks like this happens with or without this series.
>
> Found the issue, that's in
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=b8fbdc2bc4e71b62646031d5df5f08aafe15d5ad
>
> CONFIG_CRYPTO_DEV_TALITOS_SEC2 should be CONFIG_CRYPTO_DEV_TALITOS2 instead.
>
> Just sent a patch to fix it.
>
Thanks, I've tested it and the hmac failures go away.
However, testing gets stuck.
Seems there is another issue lurking in the driver.
Used cryptodev-2.6/master with the following on top:
crypto: testmgr - add some more preemption points
https://patchwork.kernel.org/patch/10972337/
crypto: talitos - fix max key size for sha384 and sha512
https://patchwork.kernel.org/patch/10988473/
[...]
alg: skcipher: skipping comparison tests for ecb-3des-talitos because
ecb(des3_ede-generic) is unavailable
INFO: task cryptomgr_test:314 blocked for more than 120 seconds.
Not tainted 5.2.0-rc1-g905bfd415e8a #1
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
cryptomgr_test D0 314 2 0x0800
Call Trace:
[e78337e0] [0004] 0x4 (unreliable)
[e78338a8] [c08a6e5c] __schedule+0x20c/0x4d4
[e78338f8] [c08a7158] schedule+0x34/0xc8
[e7833908] [c08aa5ec] schedule_timeout+0x1d4/0x350
[e7833958] [c08a7be4] wait_for_common+0xa0/0x164
[e7833998] [c03a7b14] do_ahash_op+0xa4/0xc4
[e78339b8] [c03aba00] test_ahash_vec_cfg+0x188/0x5e4
[e7833aa8] [c03ac1c8] test_hash_vs_generic_impl+0x1b0/0x2b4
[e7833de8] [c03ac498] __alg_test_hash+0x1cc/0x2d0
[e7833e28] [c03a9fb4] alg_test.part.37+0x8c/0x3ac
[e7833ef8] [c03a54d0] cryptomgr_test+0x4c/0x54
[e7833f08] [c006c410] kthread+0xf8/0x124
[e7833f38] [c001227c] ret_from_kernel_thread+0x14/0x1c
addr2line on c03aba00 points to crypto/testmgr.c:1335
1327) if (cfg->finalization_type == FINALIZATION_TYPE_DIGEST ||
1328) vec->digest_error) {
1329) /* Just using digest() */
1330) ahash_request_set_callback(req, req_flags, crypto_req_done,
1331) &wait);
1332) ahash_request_set_crypt(req, tsgl->sgl, result, vec->psize);
1333) err = do_ahash_op(crypto_ahash_digest, req, &wait,
cfg->nosimd);
1334) if (err) {
-> 1335) if (err == vec->digest_error)
1336) return 0;
1337) pr_err("alg: ahash: %s digest() failed on test vector
%s; expected_error=%d, actual_error=%d, cfg=\"%s\"\n",
1338) driver, vec_name, vec->digest_error, err,
1339) cfg->name);
1340) return err;
1341) }
1342) if (vec->digest_error) {
1343) pr_err("alg: ahash: %s digest() unexpectedly
succeeded on test vector %s; expected_error=%d, cfg=\"%s\"\n",
1344) driver, vec_name, vec->digest_error,
cfg->name);
1345) return -EINVAL;
1346) }
1347) goto result_ready;
1348) }
Seems that for some reason driver does not receive the interrupt from HW,
thus completion callback does not run.
Tried with or without current patch series, no change in behaviour.
If you cannot reproduce and don't have any idea, I'll try the hard way
(git bisect).
Thanks,
Horia
Re: [PATCH] crypto: talitos - fix max key size for sha384 and sha512
On 6/12/2019 8:49 AM, Christophe Leroy wrote:
> Below commit came with a typo in the CONFIG_ symbol, leading
> to a permanently reduced max key size regarless of the driver
> capabilities.
>
> Reported-by: Horia Geantă
> Fixes: b8fbdc2bc4e7 ("crypto: talitos - reduce max key size for SEC1")
> Signed-off-by: Christophe Leroy
Reviewed-by: Horia Geantă
Thanks,
Horia
Re: [PATCH] ARM: dts: imx7ulp: add crypto support
On 6/12/2019 4:06 PM, Shawn Guo wrote:
> On Wed, Jun 12, 2019 at 11:45:18AM +0000, Horia Geanta wrote:
>> On 6/12/2019 1:40 PM, Shawn Guo wrote:
>>> On Thu, Jun 06, 2019 at 11:02:55AM +0300, Horia Geantă wrote:
>>>> From: Iuliana Prodan
>>>>
>>>> Add crypto node in device tree for CAAM support.
>>>>
>>>> Noteworthy is that on 7ulp the interrupt line is shared
>>>> between the two job rings.
>>>>
>>>> Signed-off-by: Iuliana Prodan
>>>> Signed-off-by: Franck LENORMAND
>>>> Signed-off-by: Horia Geantă
>>>> ---
>>>>
>>>> I've just realized that this patch should be merged through the crypto
>>>> tree,
>>>> else bisectability could be affected due to cryptodev-2.6
>>>> commit 385cfc84a5a8 ("crypto: caam - disable some clock checks for
>>>> iMX7ULP")
>>>> ( https://patchwork.kernel.org/patch/10970017/ )
>>>> which should come first.
>>>
>>> I'm not sure I follow it. This is a new device added to imx7ulp DT.
>>> It's never worked before on imx7ulp. How would it affect git bisect?
>>>
>> Driver corresponding to this device (drivers/crypto/caam) has to be updated
>> before adding the node in DT.
>> Is there any guarantee wrt. merge order of the crypto and DT trees?
>
> Do not merge DT changes until driver part hits mainline.
>
That would mean driver changes would be merged in v5.3 and DT node in v5.4.
Would going through the crypto tree with this patch be such a big issue?
I don't think it's the first time (relatively small) DT patches
are merged via other trees.
Thanks,
Horia
Re: [PATCH] ARM: dts: imx7ulp: add crypto support
On 6/12/2019 1:40 PM, Shawn Guo wrote:
> On Thu, Jun 06, 2019 at 11:02:55AM +0300, Horia Geantă wrote:
>> From: Iuliana Prodan
>>
>> Add crypto node in device tree for CAAM support.
>>
>> Noteworthy is that on 7ulp the interrupt line is shared
>> between the two job rings.
>>
>> Signed-off-by: Iuliana Prodan
>> Signed-off-by: Franck LENORMAND
>> Signed-off-by: Horia Geantă
>> ---
>>
>> I've just realized that this patch should be merged through the crypto tree,
>> else bisectability could be affected due to cryptodev-2.6
>> commit 385cfc84a5a8 ("crypto: caam - disable some clock checks for iMX7ULP")
>> ( https://patchwork.kernel.org/patch/10970017/ )
>> which should come first.
>
> I'm not sure I follow it. This is a new device added to imx7ulp DT.
> It's never worked before on imx7ulp. How would it affect git bisect?
>
Driver corresponding to this device (drivers/crypto/caam) has to be updated
before adding the node in DT.
Is there any guarantee wrt. merge order of the crypto and DT trees?
Thanks,
Horia
Re: ctr(aes) broken in CAAM driver
On 6/12/2019 12:40 PM, Sascha Hauer wrote: > Hi Horia, > > On Wed, May 15, 2019 at 01:35:16PM +0000, Horia Geanta wrote: >> For talitos, the problem is the lack of IV update. >> >> For caam, the problem is incorrect IV update (output IV is equal to last >> ciphertext block, which is correect for cbc, but not for ctr mode). >> >> I am working at a fix, but it takes longer since I would like to program the >> accelerator to the save the IV (and not do counter increment in SW, which >> created problems for many other implementations). > > Any news here? With the fix Ard provided gcm(aes) now works again, but > only as long as the crypto self tests are disabled. > I've recently submitted support for IV update done in HW (caam engine), which fixes this issue: https://patchwork.kernel.org/cover/10984927/ Unfortunately it's probably too big to be sent to -stable. We'll have to rely on Ard's workaround on previous kernels. Horia
Re: [PATCH v2 0/4] Additional fixes on Talitos driver
On 6/11/2019 6:40 PM, Christophe Leroy wrote:
>
>
> Le 11/06/2019 à 17:37, Horia Geanta a écrit :
>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>> This series is the last set of fixes for the Talitos driver.
>>>
>>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>>
>> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
>> hmac(sha512):
>
> Is that new with this series or did you already have it before ?
>
Looks like this happens with or without this series.
I haven't checked the state of this driver for quite some time.
Since I've noticed increased activity, I thought it would be worth
actually testing the changes.
Are changes in patch 2/4 ("crypto: talitos - fix hash on SEC1.")
strictly for sec 1.x or they affect all revisions?
> What do you mean by "fuzz testing" enabled ? Is that
> CONFIG_CRYPTO_MANAGER_EXTRA_TESTS or something else ?
>
Yes, it's this config symbol.
Horia
Re: [PATCH v2 0/4] Additional fixes on Talitos driver
On 6/11/2019 5:39 PM, Christophe Leroy wrote: > This series is the last set of fixes for the Talitos driver. > > We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and > SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS: > I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and hmac(sha512): alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=2497 ksize=124", cfg="random: inplace use_finup nosimd src_divs=[76.49%@+4002, 23.51%@alignmask+26] iv_offset=4" alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=27 ksize=121", cfg="random: inplace may_sleep use_digest src_divs=[100.0%@+10] iv_offset=9" Reproducibility rate is 100% so far, here are a few more runs - they might help finding a pattern: 1. alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=184 ksize=121", cfg="random: use_finup src_divs=[100.0%@+3988] dst_divs=[100.0%@+547] iv_offset=44" alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=7 ksize=122", cfg="random: may_sleep use_digest src_divs=[100.0%@+3968] dst_divs=[100.0%@+20]" 2. alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=6481 ksize=120", cfg="random: use_final src_divs=[100.0%@+6] dst_divs=[43.84%@alignmask+6, 56.16%@+22]" alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=635 ksize=128", cfg="random: may_sleep use_finup src_divs=[100.0%@+4062] dst_divs=[20.47%@+2509, 72.36%@alignmask+2, 7.17%@alignmask+3990]" 3. alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=2428 ksize=127", cfg="random: may_sleep use_finup src_divs=[35.19%@+18, 64.81%@+1755] dst_divs=[100.0%@+111] iv_offset=5" alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=4345 ksize=128", cfg="random: may_sleep use_digest src_divs=[100.0%@+2820] iv_offset=59" If you run several times with fuzz testing enabled on your sec2.2, are you able to see similar failures? Thanks, Horia
Re: [PATCH v1 2/5] crypto: talitos - move struct talitos_edesc into talitos.h
On 6/11/2019 3:38 PM, Christophe Leroy wrote:
>
>
> Le 11/06/2019 à 13:57, Horia Geanta a écrit :
>> On 6/6/2019 2:31 PM, Christophe Leroy wrote:
>>> Next patch will require struct talitos_edesc to be defined
>>> earlier in talitos.c
>>>
>>> This patch moves it into talitos.h so that it can be used
>>> from any place in talitos.c
>>>
>>> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on
>>> SEC1")
>> This isn't really a fix, so please drop the tag.
>
> As the next patch requires this one and Fixes 37b5e8897eb5, by setting
> Fixes: 37b5e8897eb5 here was for me a way to tell stable that this one
> is required for the following one.
>
> Otherwise, how can I ensure that this one will be taken when next one is
> taken ?
>
If you want these patches to be automatically sent to -stable (once they
are merged in main tree), then add a Cc: tag.
Horia
Re: [PATCH v1 0/5] Additional fixes on Talitos driver
On 6/6/2019 2:31 PM, Christophe Leroy wrote: > This series is the last set of fixes for the Talitos driver. > > We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and > SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS: > I get failures, probably due to patch 1/5: alg: skcipher: cbc-aes-talitos encryption test failed (wrong result) on test vector 0, cfg="in-place" alg: skcipher: cbc-des-talitos encryption test failed (wrong result) on test vector 0, cfg="in-place" alg: skcipher: cbc-3des-talitos encryption test failed (wrong result) on test vector 0, cfg="in-place" Horia
Re: [PATCH v1 2/5] crypto: talitos - move struct talitos_edesc into talitos.h
On 6/6/2019 2:31 PM, Christophe Leroy wrote:
> Next patch will require struct talitos_edesc to be defined
> earlier in talitos.c
>
> This patch moves it into talitos.h so that it can be used
> from any place in talitos.c
>
> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on
> SEC1")
This isn't really a fix, so please drop the tag.
Thanks,
Horia
