LinuxLists.cc - [RFC PATCH 0/2] crypto: caam - fix cts(cbc(aes)) with CAAM driver

[permalink] [raw]

Subject: [RFC PATCH 1/2] crypto: caam - properly set IV after {en,de}crypt

Certain cipher modes like CTS expect the IV (req->info) of
ablkcipher_request (or equivalently req->iv of skcipher_request) to
contain the last ciphertext block when the {en,de}crypt operation is done.
This is currently not the case for the CAAM driver which in turn breaks
e.g. cts(cbc(aes)) when the CAAM driver is enabled.

This patch fixes the CAAM driver to properly set the IV after the
{en,de}crypt operation of ablkcipher finishes.

Signed-off-by: David Gstir <[email protected]>
---
drivers/crypto/caam/caamalg.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 398807d1b77e..d13c1aee4427 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -882,10 +882,11 @@ static void ablkcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err,
{
struct ablkcipher_request *req = context;
struct ablkcipher_edesc *edesc;
-#ifdef DEBUG
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
+ int nents;

+#ifdef DEBUG
dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
#endif

@@ -904,6 +905,19 @@ static void ablkcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err,
#endif

ablkcipher_unmap(jrdev, edesc, req);
+
+ if (req->src == req->dst)
+ nents = edesc->src_nents;
+ else
+ nents = edesc->dst_nents;
+
+ /*
+ * The crypto API expects us to set the IV (req->info) to the last
+ * ciphertext block. This is used e.g. by the CTS mode.
+ */
+ sg_pcopy_to_buffer(req->dst, nents, req->info, ivsize,
+ req->nbytes - ivsize);
+
kfree(edesc);

ablkcipher_request_complete(req, err);
@@ -914,10 +928,10 @@ static void ablkcipher_decrypt_done(struct device *jrdev, u32 *desc, u32 err,
{
struct ablkcipher_request *req = context;
struct ablkcipher_edesc *edesc;
-#ifdef DEBUG
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
int ivsize = crypto_ablkcipher_ivsize(ablkcipher);

+#ifdef DEBUG
dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
#endif

@@ -935,6 +949,14 @@ static void ablkcipher_decrypt_done(struct device *jrdev, u32 *desc, u32 err,
#endif

ablkcipher_unmap(jrdev, edesc, req);
+
+ /*
+ * The crypto API expects us to set the IV (req->info) to the last
+ * ciphertext block.
+ */
+ sg_pcopy_to_buffer(req->src, edesc->src_nents, req->info, ivsize,
+ req->nbytes - ivsize);
+
kfree(edesc);

ablkcipher_request_complete(req, err);
--
2.12.0

2017-06-02 12:25:08

[permalink] [raw]

Subject: [RFC PATCH 2/2] crypto: caam - fix k*alloc if called from own cipher callback

There are cases (e.g. the cts mode) where a cipher can be called again
from its own callback. In our case this callback is executed from within
a tasklet in the jobring, we must not sleep when allocating memory.

This patch detects such cases by using in_interrupt() to properly set the
k*alloc flags. In most cases we will still use GFP_KERNEL if the flags
CRYPTO_TFM_REQ_MAY_SLEEP or CRYPTO_TFM_REQ_MAY_BACKLOG are set for the
cipher request.

Signed-off-by: David Gstir <[email protected]>
---
drivers/crypto/caam/caamalg.c | 29 +++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index d13c1aee4427..4c4a5d1ad875 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -1209,13 +1209,18 @@ static struct aead_edesc *aead_edesc_alloc(struct aead_request *req,
struct crypto_aead *aead = crypto_aead_reqtfm(req);
struct caam_ctx *ctx = crypto_aead_ctx(aead);
struct device *jrdev = ctx->jrdev;
- gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ? GFP_KERNEL : GFP_ATOMIC;
+ gfp_t flags;
int src_nents, mapped_src_nents, dst_nents = 0, mapped_dst_nents = 0;
struct aead_edesc *edesc;
int sec4_sg_index, sec4_sg_len, sec4_sg_bytes;
unsigned int authsize = ctx->authsize;

+ if (!in_interrupt() && req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
+ CRYPTO_TFM_REQ_MAY_SLEEP))
+ flags = GFP_KERNEL;
+ else
+ flags = GFP_ATOMIC;
+
if (unlikely(req->dst != req->src)) {
src_nents = sg_nents_for_len(req->src, req->assoclen +
req->cryptlen);
@@ -1497,9 +1502,7 @@ static struct ablkcipher_edesc *ablkcipher_edesc_alloc(struct ablkcipher_request
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
struct caam_ctx *ctx = crypto_ablkcipher_ctx(ablkcipher);
struct device *jrdev = ctx->jrdev;
- gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ?
- GFP_KERNEL : GFP_ATOMIC;
+ gfp_t flags;
int src_nents, mapped_src_nents, dst_nents = 0, mapped_dst_nents = 0;
struct ablkcipher_edesc *edesc;
dma_addr_t iv_dma = 0;
@@ -1507,6 +1510,12 @@ static struct ablkcipher_edesc *ablkcipher_edesc_alloc(struct ablkcipher_request
int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
int dst_sg_idx, sec4_sg_ents, sec4_sg_bytes;

+ if (!in_interrupt() && req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
+ CRYPTO_TFM_REQ_MAY_SLEEP))
+ flags = GFP_KERNEL;
+ else
+ flags = GFP_ATOMIC;
+
src_nents = sg_nents_for_len(req->src, req->nbytes);
if (unlikely(src_nents < 0)) {
dev_err(jrdev, "Insufficient bytes (%d) in src S/G\n",
@@ -1703,9 +1712,7 @@ static struct ablkcipher_edesc *ablkcipher_giv_edesc_alloc(
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
struct caam_ctx *ctx = crypto_ablkcipher_ctx(ablkcipher);
struct device *jrdev = ctx->jrdev;
- gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ?
- GFP_KERNEL : GFP_ATOMIC;
+ gfp_t flags;
int src_nents, mapped_src_nents, dst_nents, mapped_dst_nents;
struct ablkcipher_edesc *edesc;
dma_addr_t iv_dma = 0;
@@ -1713,6 +1720,12 @@ static struct ablkcipher_edesc *ablkcipher_giv_edesc_alloc(
int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
int dst_sg_idx, sec4_sg_ents, sec4_sg_bytes;

+ if (!in_interrupt() && req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
+ CRYPTO_TFM_REQ_MAY_SLEEP))
+ flags = GFP_KERNEL;
+ else
+ flags = GFP_ATOMIC;
+
src_nents = sg_nents_for_len(req->src, req->nbytes);
if (unlikely(src_nents < 0)) {
dev_err(jrdev, "Insufficient bytes (%d) in src S/G\n",
--
2.12.0

2017-06-13 11:54:02

[permalink] [raw]

Subject: Re: [RFC PATCH 0/2] crypto: caam - fix cts(cbc(aes)) with CAAM driver

Friendly ping. Any feedback on that?

Thanks,
David

> On 2 Jun 2017, at 14:24, David Gstir <[email protected]> wrote:
>
> Hi!
>
> While testing fscrypt's filename encryption, I noticed that the implementation
> of cts(cbc(aes)) is broken when the CAAM hardware crypto driver is enabled.
> Some digging showed that the refactoring of crypto/cts.c in v4.8
> (commit 0605c41cc53ca) exposed some problems with CAAM's aes-cbc
> implementation. This can be tested quite easily by loading the tcrypt
> module with mode=38. Looks like the cts mode is not used very often
> and this went unnoticed since 4.8...
>
> This patch series is an attempt to fix that and get cts(cbc(aes)) working
> properly again when the CAAM driver is enabled.
>
> Specifically, the issues are:
>
> 1) The cts implementation assumes ->info of ablkcipher_request (or ->iv of
> skcipher_request respectively) to be set to the last ciphertext block when
> the aes-cbc encryption is finished. Meaning that ->info is changed by the
> aes-cbc code. The CAAM driver does not do that and leaves ->info as-is.
>
> It is not fully clear to me yet if this is a requirement of the crypto API,
> or if this is just a side effect that is exploited by the cts implementation?
>
> For now, I assumed it is a requirement of the crypto API since I've seen
> other crypto drivers (e.g. AMD's CCP) do that. So the patch fixes the CAAM
> crypto driver accordingly.
>
> Also, the aead code in the CAAM driver, more specifically the gcm mode code
> seems to have the same flaw, so it'll need a similar fix in case.
>
>
> 2) The cts implementation uses aes-cbc twice to perform its job. The second
> time, it is called from within the callback of the first call to aes-cbc.
> This usage is not properly handled in the CAAM driver which triggers the
> BUG below.
>
> My current proposal is to use in_interrupt() to detect such cases and set
> the k*alloc flags accordingly. However, since using in_interrupt() is not
> really nice, I'm wondering if there is a better way to handle this?
>
>
> Thanks,
> David
>
> <snip>
> [ 126.252543] BUG: sleeping function called from invalid context at mm/slab.h:432
> [ 126.260099] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
> [ 126.266837] no locks held by swapper/0/0.
> [ 126.270969] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.12.0-rc3-00052-g0ddec680d395 #287
> [ 126.279226] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> [ 126.285821] Backtrace:
> [ 126.288406] [<c010c3e4>] (dump_backtrace) from [<c010c690>] (show_stack+0x18/0x1c)
> [ 126.296057] r7:00000000 r6:60000113 r5:00000000 r4:c102ab48
> [ 126.301798] [<c010c678>] (show_stack) from [<c0427060>] (dump_stack+0xb4/0xe8)
> [ 126.309106] [<c0426fac>] (dump_stack) from [<c014f020>] (___might_sleep+0x17c/0x2a0)
> [ 126.316929] r9:00000000 r8:c0a016dc r7:c101ee3e r6:000001b0 r5:c0c12fac r4:ffffe000
> [ 126.324751] [<c014eea4>] (___might_sleep) from [<c014f1a8>] (__might_sleep+0x64/0xa4)
> [ 126.332655] r7:014080c1 r6:00000000 r5:000001b0 r4:c0c12fac
> [ 126.338397] [<c014f144>] (__might_sleep) from [<c0224a20>] (__kmalloc+0x130/0x1b8)
> [ 126.346039] r6:c071a8ec r5:014080c1 r4:eec01e00
> [ 126.350742] [<c02248f0>] (__kmalloc) from [<c071a8ec>] (ablkcipher_edesc_alloc.constprop.1+0x200/0x900)
> [ 126.360213] r10:00000000 r9:00000000 r8:c0a016dc r7:c0a016dc r6:ee05ac10 r5:edfdaec0
> [ 126.368109] r4:00000001 r3:00000020
> [ 126.371765] [<c071a6ec>] (ablkcipher_edesc_alloc.constprop.1) from [<c071b010>] (ablkcipher_encrypt+0x24/0x9c)
> [ 126.381843] r10:00000000 r9:00000020 r8:00000001 r7:ee05ac10 r6:ed597000 r5:edfdaec0
> [ 126.389738] r4:ed475d08
> [ 126.392354] [<c071afec>] (ablkcipher_encrypt) from [<c03d6f20>] (skcipher_encrypt_ablkcipher+0x68/0x6c)
> [ 126.401822] r7:ed475d08 r6:ed597000 r5:00000400 r4:ed475d08
> [ 126.407566] [<c03d6eb8>] (skcipher_encrypt_ablkcipher) from [<c03e8a70>] (cts_cbc_encrypt+0x118/0x124)
> [ 126.416947] r7:ed475d08 r6:c1001cc0 r5:00000010 r4:edfdae00
> [ 126.422686] [<c03e8958>] (cts_cbc_encrypt) from [<c03e8b88>] (crypto_cts_encrypt_done+0x20/0x54)
> [ 126.431548] r10:00000000 r9:ee05ac10 r8:00000000 r7:00000010 r6:edc8e6c0 r5:edc8e6d8
> [ 126.439443] r4:edfdae00
> [ 126.442056] [<c03e8b68>] (crypto_cts_encrypt_done) from [<c07195a0>] (ablkcipher_encrypt_done+0x88/0x9c)
> [ 126.445180] fec 2188000.ethernet eth0: MDIO read timeout
> [ 126.456948] r5:edc8e6d8 r4:edfdaec0
> [ 126.460604] [<c0719518>] (ablkcipher_encrypt_done) from [<c0715ca0>] (caam_jr_dequeue+0x214/0x2d4)
> [ 126.469639] r9:00000001 r8:ee088010 r7:000001ff r6:00000001 r5:00000000 r4:edfdaec0
> [ 126.477467] [<c0715a8c>] (caam_jr_dequeue) from [<c012b3f0>] (tasklet_action+0x98/0x154)
> [ 126.485160] fec 2188000.ethernet eth0: MDIO read timeout
> [ 126.490975] r10:c1080b80 r9:c1008b84 r8:00000000 r7:c1000000 r6:00000000 r5:ee088028
> [ 126.498870] r4:ee088024
> [ 126.501484] [<c012b358>] (tasklet_action) from [<c012b60c>] (__do_softirq+0xf0/0x2a4)
> [ 126.509390] r10:40000006 r9:c1002080 r8:00000101 r7:c1002098 r6:c1000000 r5:00000006
> [ 126.517285] r4:00000000
> [ 126.519896] [<c012b51c>] (__do_softirq) from [<c012bb90>] (irq_exit+0xec/0x168)
> [ 126.525127] fec 2188000.ethernet eth0: MDIO write timeout
> [ 126.532709] r10:c1008cf0 r9:eec08400 r8:00000001 r7:00000000 r6:c1008b84 r5:00000000
> [ 126.540603] r4:c0fd940c
> [ 126.543222] [<c012baa4>] (irq_exit) from [<c017ff24>] (__handle_domain_irq+0x74/0xe8)
> [ 126.551135] [<c017feb0>] (__handle_domain_irq) from [<c01015fc>] (gic_handle_irq+0x58/0xbc)
> [ 126.559564] r9:f4000100 r8:c102af80 r7:c1001ea8 r6:000003ff r5:000003eb r4:f400010c
> [ 126.567384] [<c01015a4>] (gic_handle_irq) from [<c010d2f0>] (__irq_svc+0x70/0x98)
> [ 126.574937] Exception stack(0xc1001ea8 to 0xc1001ef0)
> [ 126.580065] 1ea0: 00000001 2e39a000 00000000 c100c080 00000000 ef374698
> [ 126.588320] 1ec0: 653b20b1 0000001d 653afed6 0000001d 00000000 c1001f24 c1001ec8 c1001ef8
> [ 126.596568] 1ee0: c016fcf4 c06f1fbc 20000013 ffffffff
> [ 126.601696] r10:00000000 r9:c1000000 r8:653afed6 r7:c1001edc r6:ffffffff r5:20000013
> [ 126.609591] r4:c06f1fbc
> [ 126.612210] [<c06f1e34>] (cpuidle_enter_state) from [<c06f2120>] (cpuidle_enter+0x1c/0x20)
> [ 126.620551] r10:c100f8c0 r9:ef374698 r8:c1008b84 r7:c0fda690 r6:c10089ec r5:c1008a38
> [ 126.628448] r4:c1000000 r3:ef374698
> [ 126.632114] [<c06f2104>] (cpuidle_enter) from [<c0167330>] (call_cpuidle+0x28/0x44)
> [ 126.639859] [<c0167308>] (call_cpuidle) from [<c0167608>] (do_idle+0x1ac/0x220)
> [ 126.647249] [<c016745c>] (do_idle) from [<c0167a20>] (cpu_startup_entry+0x20/0x24)
> [ 126.654895] r10:c0e60a50 r9:efffc940 r8:c1080000 r7:c10089c0 r6:c1080000 r5:00000002
> [ 126.662793] r4:000000bd r3:c0e733c4
> [ 126.666457] [<c0167a00>] (cpu_startup_entry) from [<c09cfbd4>] (rest_init+0x154/0x198)
> [ 126.674463] [<c09cfa80>] (rest_init) from [<c0e00cf0>] (start_kernel+0x344/0x3b8)
> [ 126.682015] r5:ffffffff r4:c108004c
> [ 126.685668] [<c0e009ac>] (start_kernel) from [<1000807c>] (0x1000807c)
> </snip>
>
> crypto: caam - properly set IV after {en,de}crypt
> crypto: caam - fix k*alloc if called from own cipher callback
>
> drivers/crypto/caam/caamalg.c | 55 +++++++++++++++++++++++++++++++++++--------
> 1 file changed, 45 insertions(+), 10 deletions(-)
>
> --
> 2.12.0

2017-06-15 14:57:25

[permalink] [raw]

Subject: Re: [RFC PATCH 0/2] crypto: caam - fix cts(cbc(aes)) with CAAM driver

On 6/2/2017 3:25 PM, David Gstir wrote:
> Hi!
>
> While testing fscrypt's filename encryption, I noticed that the implementation
> of cts(cbc(aes)) is broken when the CAAM hardware crypto driver is enabled.
> Some digging showed that the refactoring of crypto/cts.c in v4.8
> (commit 0605c41cc53ca) exposed some problems with CAAM's aes-cbc
> implementation. This can be tested quite easily by loading the tcrypt
> module with mode=38. Looks like the cts mode is not used very often
> and this went unnoticed since 4.8...
>
> This patch series is an attempt to fix that and get cts(cbc(aes)) working
> properly again when the CAAM driver is enabled.
>
David, thanks for taking time to fix these.

> Specifically, the issues are:
>
> 1) The cts implementation assumes ->info of ablkcipher_request (or ->iv of
> skcipher_request respectively) to be set to the last ciphertext block when
> the aes-cbc encryption is finished. Meaning that ->info is changed by the
> aes-cbc code. The CAAM driver does not do that and leaves ->info as-is.
>
> It is not fully clear to me yet if this is a requirement of the crypto API,
> or if this is just a side effect that is exploited by the cts implementation?
>
> For now, I assumed it is a requirement of the crypto API since I've seen
> other crypto drivers (e.g. AMD's CCP) do that. So the patch fixes the CAAM
> crypto driver accordingly.
>
IIUC, yes, the Crypto API requires ->info to be updated.

Dan, Radu,

IIRC, you've hit smth. similar while testing CAAM on i.MX.
Could you please review David's fix, compare it with yours, so in the
end we would choose the one that fits best?

> Also, the aead code in the CAAM driver, more specifically the gcm mode code
> seems to have the same flaw, so it'll need a similar fix in case.
>
>
> 2) The cts implementation uses aes-cbc twice to perform its job. The second
> time, it is called from within the callback of the first call to aes-cbc.
> This usage is not properly handled in the CAAM driver which triggers the
> BUG below.
>
Dan, Radu - have you seen this on i.MX?

> My current proposal is to use in_interrupt() to detect such cases and set
> the k*alloc flags accordingly. However, since using in_interrupt() is not
> really nice, I'm wondering if there is a better way to handle this?
>
Nothing else crosses my mind right now, but I'd like to sleep on it.

>
> Thanks,
> David
>
> <snip>
> [ 126.252543] BUG: sleeping function called from invalid context at mm/slab.h:432
> [ 126.260099] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
> [ 126.266837] no locks held by swapper/0/0.
> [ 126.270969] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.12.0-rc3-00052-g0ddec680d395 #287
> [ 126.279226] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> [ 126.285821] Backtrace:
> [ 126.288406] [<c010c3e4>] (dump_backtrace) from [<c010c690>] (show_stack+0x18/0x1c)
> [ 126.296057] r7:00000000 r6:60000113 r5:00000000 r4:c102ab48
> [ 126.301798] [<c010c678>] (show_stack) from [<c0427060>] (dump_stack+0xb4/0xe8)
> [ 126.309106] [<c0426fac>] (dump_stack) from [<c014f020>] (___might_sleep+0x17c/0x2a0)
> [ 126.316929] r9:00000000 r8:c0a016dc r7:c101ee3e r6:000001b0 r5:c0c12fac r4:ffffe000
> [ 126.324751] [<c014eea4>] (___might_sleep) from [<c014f1a8>] (__might_sleep+0x64/0xa4)
> [ 126.332655] r7:014080c1 r6:00000000 r5:000001b0 r4:c0c12fac
> [ 126.338397] [<c014f144>] (__might_sleep) from [<c0224a20>] (__kmalloc+0x130/0x1b8)
> [ 126.346039] r6:c071a8ec r5:014080c1 r4:eec01e00
> [ 126.350742] [<c02248f0>] (__kmalloc) from [<c071a8ec>] (ablkcipher_edesc_alloc.constprop.1+0x200/0x900)
> [ 126.360213] r10:00000000 r9:00000000 r8:c0a016dc r7:c0a016dc r6:ee05ac10 r5:edfdaec0
> [ 126.368109] r4:00000001 r3:00000020
> [ 126.371765] [<c071a6ec>] (ablkcipher_edesc_alloc.constprop.1) from [<c071b010>] (ablkcipher_encrypt+0x24/0x9c)
> [ 126.381843] r10:00000000 r9:00000020 r8:00000001 r7:ee05ac10 r6:ed597000 r5:edfdaec0
> [ 126.389738] r4:ed475d08
> [ 126.392354] [<c071afec>] (ablkcipher_encrypt) from [<c03d6f20>] (skcipher_encrypt_ablkcipher+0x68/0x6c)
> [ 126.401822] r7:ed475d08 r6:ed597000 r5:00000400 r4:ed475d08
> [ 126.407566] [<c03d6eb8>] (skcipher_encrypt_ablkcipher) from [<c03e8a70>] (cts_cbc_encrypt+0x118/0x124)
> [ 126.416947] r7:ed475d08 r6:c1001cc0 r5:00000010 r4:edfdae00
> [ 126.422686] [<c03e8958>] (cts_cbc_encrypt) from [<c03e8b88>] (crypto_cts_encrypt_done+0x20/0x54)
> [ 126.431548] r10:00000000 r9:ee05ac10 r8:00000000 r7:00000010 r6:edc8e6c0 r5:edc8e6d8
> [ 126.439443] r4:edfdae00
> [ 126.442056] [<c03e8b68>] (crypto_cts_encrypt_done) from [<c07195a0>] (ablkcipher_encrypt_done+0x88/0x9c)
> [ 126.445180] fec 2188000.ethernet eth0: MDIO read timeout
> [ 126.456948] r5:edc8e6d8 r4:edfdaec0
> [ 126.460604] [<c0719518>] (ablkcipher_encrypt_done) from [<c0715ca0>] (caam_jr_dequeue+0x214/0x2d4)
> [ 126.469639] r9:00000001 r8:ee088010 r7:000001ff r6:00000001 r5:00000000 r4:edfdaec0
> [ 126.477467] [<c0715a8c>] (caam_jr_dequeue) from [<c012b3f0>] (tasklet_action+0x98/0x154)
> [ 126.485160] fec 2188000.ethernet eth0: MDIO read timeout
> [ 126.490975] r10:c1080b80 r9:c1008b84 r8:00000000 r7:c1000000 r6:00000000 r5:ee088028
> [ 126.498870] r4:ee088024
> [ 126.501484] [<c012b358>] (tasklet_action) from [<c012b60c>] (__do_softirq+0xf0/0x2a4)
> [ 126.509390] r10:40000006 r9:c1002080 r8:00000101 r7:c1002098 r6:c1000000 r5:00000006
> [ 126.517285] r4:00000000
> [ 126.519896] [<c012b51c>] (__do_softirq) from [<c012bb90>] (irq_exit+0xec/0x168)
> [ 126.525127] fec 2188000.ethernet eth0: MDIO write timeout
> [ 126.532709] r10:c1008cf0 r9:eec08400 r8:00000001 r7:00000000 r6:c1008b84 r5:00000000
> [ 126.540603] r4:c0fd940c
> [ 126.543222] [<c012baa4>] (irq_exit) from [<c017ff24>] (__handle_domain_irq+0x74/0xe8)
> [ 126.551135] [<c017feb0>] (__handle_domain_irq) from [<c01015fc>] (gic_handle_irq+0x58/0xbc)
> [ 126.559564] r9:f4000100 r8:c102af80 r7:c1001ea8 r6:000003ff r5:000003eb r4:f400010c
> [ 126.567384] [<c01015a4>] (gic_handle_irq) from [<c010d2f0>] (__irq_svc+0x70/0x98)
> [ 126.574937] Exception stack(0xc1001ea8 to 0xc1001ef0)
> [ 126.580065] 1ea0: 00000001 2e39a000 00000000 c100c080 00000000 ef374698
> [ 126.588320] 1ec0: 653b20b1 0000001d 653afed6 0000001d 00000000 c1001f24 c1001ec8 c1001ef8
> [ 126.596568] 1ee0: c016fcf4 c06f1fbc 20000013 ffffffff
> [ 126.601696] r10:00000000 r9:c1000000 r8:653afed6 r7:c1001edc r6:ffffffff r5:20000013
> [ 126.609591] r4:c06f1fbc
> [ 126.612210] [<c06f1e34>] (cpuidle_enter_state) from [<c06f2120>] (cpuidle_enter+0x1c/0x20)
> [ 126.620551] r10:c100f8c0 r9:ef374698 r8:c1008b84 r7:c0fda690 r6:c10089ec r5:c1008a38
> [ 126.628448] r4:c1000000 r3:ef374698
> [ 126.632114] [<c06f2104>] (cpuidle_enter) from [<c0167330>] (call_cpuidle+0x28/0x44)
> [ 126.639859] [<c0167308>] (call_cpuidle) from [<c0167608>] (do_idle+0x1ac/0x220)
> [ 126.647249] [<c016745c>] (do_idle) from [<c0167a20>] (cpu_startup_entry+0x20/0x24)
> [ 126.654895] r10:c0e60a50 r9:efffc940 r8:c1080000 r7:c10089c0 r6:c1080000 r5:00000002
> [ 126.662793] r4:000000bd r3:c0e733c4
> [ 126.666457] [<c0167a00>] (cpu_startup_entry) from [<c09cfbd4>] (rest_init+0x154/0x198)
> [ 126.674463] [<c09cfa80>] (rest_init) from [<c0e00cf0>] (start_kernel+0x344/0x3b8)
> [ 126.682015] r5:ffffffff r4:c108004c
> [ 126.685668] [<c0e009ac>] (start_kernel) from [<1000807c>] (0x1000807c)
> </snip>
>
> crypto: caam - properly set IV after {en,de}crypt
> crypto: caam - fix k*alloc if called from own cipher callback
>
> drivers/crypto/caam/caamalg.c | 55 +++++++++++++++++++++++++++++++++++--------
> 1 file changed, 45 insertions(+), 10 deletions(-)
>

2017-06-16 07:57:06

[permalink] [raw]

Subject: Re: [RFC PATCH 0/2] crypto: caam - fix cts(cbc(aes)) with CAAM driver

On 6/15/2017 5:57 PM, Horia Geant? wrote:
> On 6/2/2017 3:25 PM, David Gstir wrote:
>> Hi!
>>
>> While testing fscrypt's filename encryption, I noticed that the implementation
>> of cts(cbc(aes)) is broken when the CAAM hardware crypto driver is enabled.
>> Some digging showed that the refactoring of crypto/cts.c in v4.8
>> (commit 0605c41cc53ca) exposed some problems with CAAM's aes-cbc
>> implementation. This can be tested quite easily by loading the tcrypt
>> module with mode=38. Looks like the cts mode is not used very often
>> and this went unnoticed since 4.8...
>>
>> This patch series is an attempt to fix that and get cts(cbc(aes)) working
>> properly again when the CAAM driver is enabled.
>>
> David, thanks for taking time to fix these.
>
>> Specifically, the issues are:
[snip]
>> 2) The cts implementation uses aes-cbc twice to perform its job. The second
>> time, it is called from within the callback of the first call to aes-cbc.
>> This usage is not properly handled in the CAAM driver which triggers the
>> BUG below.
>>
> Dan, Radu - have you seen this on i.MX?
>
>> My current proposal is to use in_interrupt() to detect such cases and set
>> the k*alloc flags accordingly. However, since using in_interrupt() is not
>> really nice, I'm wondering if there is a better way to handle this?
>>
> Nothing else crosses my mind right now, but I'd like to sleep on it.
>
Herbert,

Commit 0605c41cc53ca ("crypto: cts - Convert to skcipher") appends
CRYPTO_TFM_REQ_MAY_BACKLOG to the original crypto request flags for the
last block - when calling cts_cbc_encrypt().
Is it really needed?

For cts(cbc(aes)) with cbc(aes) offloaded in HW, i.e. running in async
mode, we get the below stack for CAAM driver.
Driver is told that it can sleep (CRYPTO_TFM_REQ_MAY_BACKLOG flag), so
it uses GFP_KERNEL to allocate memory. However, this is incorrect, since
driver runs in atomic context (softirq).

I wouldn't say that:
-David's suggestion - adding in_interrupt()
- or in general: trying to detect in CAAM driver whether we are in
atomic context using anything else than what crypto API provides
(MAY_BACKLOG, MAY_SLEEP flags)

is something we want to do.
IIUC, in general caller (cts / crypto API) is responsible for telling
the callee if it will run in atomic context or not:
https://lwn.net/Articles/274695/

Thanks,
Horia

>> <snip>
>> [ 126.252543] BUG: sleeping function called from invalid context at mm/slab.h:432
>> [ 126.260099] in_atomic(): 1, irqs_disabled(): 0, pid: 0, name: swapper/0
>> [ 126.266837] no locks held by swapper/0/0.
>> [ 126.270969] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.12.0-rc3-00052-g0ddec680d395 #287
>> [ 126.279226] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
>> [ 126.285821] Backtrace:
>> [ 126.288406] [<c010c3e4>] (dump_backtrace) from [<c010c690>] (show_stack+0x18/0x1c)
>> [ 126.296057] r7:00000000 r6:60000113 r5:00000000 r4:c102ab48
>> [ 126.301798] [<c010c678>] (show_stack) from [<c0427060>] (dump_stack+0xb4/0xe8)
>> [ 126.309106] [<c0426fac>] (dump_stack) from [<c014f020>] (___might_sleep+0x17c/0x2a0)
>> [ 126.316929] r9:00000000 r8:c0a016dc r7:c101ee3e r6:000001b0 r5:c0c12fac r4:ffffe000
>> [ 126.324751] [<c014eea4>] (___might_sleep) from [<c014f1a8>] (__might_sleep+0x64/0xa4)
>> [ 126.332655] r7:014080c1 r6:00000000 r5:000001b0 r4:c0c12fac
>> [ 126.338397] [<c014f144>] (__might_sleep) from [<c0224a20>] (__kmalloc+0x130/0x1b8)
>> [ 126.346039] r6:c071a8ec r5:014080c1 r4:eec01e00
>> [ 126.350742] [<c02248f0>] (__kmalloc) from [<c071a8ec>] (ablkcipher_edesc_alloc.constprop.1+0x200/0x900)
>> [ 126.360213] r10:00000000 r9:00000000 r8:c0a016dc r7:c0a016dc r6:ee05ac10 r5:edfdaec0
>> [ 126.368109] r4:00000001 r3:00000020
>> [ 126.371765] [<c071a6ec>] (ablkcipher_edesc_alloc.constprop.1) from [<c071b010>] (ablkcipher_encrypt+0x24/0x9c)
>> [ 126.381843] r10:00000000 r9:00000020 r8:00000001 r7:ee05ac10 r6:ed597000 r5:edfdaec0
>> [ 126.389738] r4:ed475d08
>> [ 126.392354] [<c071afec>] (ablkcipher_encrypt) from [<c03d6f20>] (skcipher_encrypt_ablkcipher+0x68/0x6c)
>> [ 126.401822] r7:ed475d08 r6:ed597000 r5:00000400 r4:ed475d08
>> [ 126.407566] [<c03d6eb8>] (skcipher_encrypt_ablkcipher) from [<c03e8a70>] (cts_cbc_encrypt+0x118/0x124)
>> [ 126.416947] r7:ed475d08 r6:c1001cc0 r5:00000010 r4:edfdae00
>> [ 126.422686] [<c03e8958>] (cts_cbc_encrypt) from [<c03e8b88>] (crypto_cts_encrypt_done+0x20/0x54)
>> [ 126.431548] r10:00000000 r9:ee05ac10 r8:00000000 r7:00000010 r6:edc8e6c0 r5:edc8e6d8
>> [ 126.439443] r4:edfdae00
>> [ 126.442056] [<c03e8b68>] (crypto_cts_encrypt_done) from [<c07195a0>] (ablkcipher_encrypt_done+0x88/0x9c)
>> [ 126.445180] fec 2188000.ethernet eth0: MDIO read timeout
>> [ 126.456948] r5:edc8e6d8 r4:edfdaec0
>> [ 126.460604] [<c0719518>] (ablkcipher_encrypt_done) from [<c0715ca0>] (caam_jr_dequeue+0x214/0x2d4)
>> [ 126.469639] r9:00000001 r8:ee088010 r7:000001ff r6:00000001 r5:00000000 r4:edfdaec0
>> [ 126.477467] [<c0715a8c>] (caam_jr_dequeue) from [<c012b3f0>] (tasklet_action+0x98/0x154)
>> [ 126.485160] fec 2188000.ethernet eth0: MDIO read timeout
>> [ 126.490975] r10:c1080b80 r9:c1008b84 r8:00000000 r7:c1000000 r6:00000000 r5:ee088028
>> [ 126.498870] r4:ee088024
>> [ 126.501484] [<c012b358>] (tasklet_action) from [<c012b60c>] (__do_softirq+0xf0/0x2a4)
>> [ 126.509390] r10:40000006 r9:c1002080 r8:00000101 r7:c1002098 r6:c1000000 r5:00000006
>> [ 126.517285] r4:00000000
>> [ 126.519896] [<c012b51c>] (__do_softirq) from [<c012bb90>] (irq_exit+0xec/0x168)
>> [ 126.525127] fec 2188000.ethernet eth0: MDIO write timeout
>> [ 126.532709] r10:c1008cf0 r9:eec08400 r8:00000001 r7:00000000 r6:c1008b84 r5:00000000
>> [ 126.540603] r4:c0fd940c
>> [ 126.543222] [<c012baa4>] (irq_exit) from [<c017ff24>] (__handle_domain_irq+0x74/0xe8)
>> [ 126.551135] [<c017feb0>] (__handle_domain_irq) from [<c01015fc>] (gic_handle_irq+0x58/0xbc)
>> [ 126.559564] r9:f4000100 r8:c102af80 r7:c1001ea8 r6:000003ff r5:000003eb r4:f400010c
>> [ 126.567384] [<c01015a4>] (gic_handle_irq) from [<c010d2f0>] (__irq_svc+0x70/0x98)
>> [ 126.574937] Exception stack(0xc1001ea8 to 0xc1001ef0)
>> [ 126.580065] 1ea0: 00000001 2e39a000 00000000 c100c080 00000000 ef374698
>> [ 126.588320] 1ec0: 653b20b1 0000001d 653afed6 0000001d 00000000 c1001f24 c1001ec8 c1001ef8
>> [ 126.596568] 1ee0: c016fcf4 c06f1fbc 20000013 ffffffff
>> [ 126.601696] r10:00000000 r9:c1000000 r8:653afed6 r7:c1001edc r6:ffffffff r5:20000013
>> [ 126.609591] r4:c06f1fbc
>> [ 126.612210] [<c06f1e34>] (cpuidle_enter_state) from [<c06f2120>] (cpuidle_enter+0x1c/0x20)
>> [ 126.620551] r10:c100f8c0 r9:ef374698 r8:c1008b84 r7:c0fda690 r6:c10089ec r5:c1008a38
>> [ 126.628448] r4:c1000000 r3:ef374698
>> [ 126.632114] [<c06f2104>] (cpuidle_enter) from [<c0167330>] (call_cpuidle+0x28/0x44)
>> [ 126.639859] [<c0167308>] (call_cpuidle) from [<c0167608>] (do_idle+0x1ac/0x220)
>> [ 126.647249] [<c016745c>] (do_idle) from [<c0167a20>] (cpu_startup_entry+0x20/0x24)
>> [ 126.654895] r10:c0e60a50 r9:efffc940 r8:c1080000 r7:c10089c0 r6:c1080000 r5:00000002
>> [ 126.662793] r4:000000bd r3:c0e733c4
>> [ 126.666457] [<c0167a00>] (cpu_startup_entry) from [<c09cfbd4>] (rest_init+0x154/0x198)
>> [ 126.674463] [<c09cfa80>] (rest_init) from [<c0e00cf0>] (start_kernel+0x344/0x3b8)
>> [ 126.682015] r5:ffffffff r4:c108004c
>> [ 126.685668] [<c0e009ac>] (start_kernel) from [<1000807c>] (0x1000807c)
>> </snip>
>>
>> crypto: caam - properly set IV after {en,de}crypt
>> crypto: caam - fix k*alloc if called from own cipher callback
>>
>> drivers/crypto/caam/caamalg.c | 55 +++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 45 insertions(+), 10 deletions(-)
>>

2017-06-16 08:00:12

[permalink] [raw]

Subject: Re: [RFC PATCH 0/2] crypto: caam - fix cts(cbc(aes)) with CAAM driver

On Fri, Jun 16, 2017 at 07:57:00AM +0000, Horia Geantă wrote:
>
> Commit 0605c41cc53ca ("crypto: cts - Convert to skcipher") appends
> CRYPTO_TFM_REQ_MAY_BACKLOG to the original crypto request flags for the
> last block - when calling cts_cbc_encrypt().
> Is it really needed?

Yes, because at this point we cannot tell the sender to back off.

> For cts(cbc(aes)) with cbc(aes) offloaded in HW, i.e. running in async
> mode, we get the below stack for CAAM driver.
> Driver is told that it can sleep (CRYPTO_TFM_REQ_MAY_BACKLOG flag), so
> it uses GFP_KERNEL to allocate memory. However, this is incorrect, since
> driver runs in atomic context (softirq).

This is wrong. Whether you can sleep or not is determined by
MAY_SLEEP, not MAY_BACKLOG. MAY_BACKLOG only indicates that this
request must be queued, even if the queue is full.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2017-06-16 21:01:51

[permalink] [raw]

Subject: Re: [RFC PATCH 0/2] crypto: caam - fix cts(cbc(aes)) with CAAM driver

On 6/16/2017 11:00 AM, Herbert Xu wrote:
> On Fri, Jun 16, 2017 at 07:57:00AM +0000, Horia Geant? wrote:
>>
>> Commit 0605c41cc53ca ("crypto: cts - Convert to skcipher") appends
>> CRYPTO_TFM_REQ_MAY_BACKLOG to the original crypto request flags for the
>> last block - when calling cts_cbc_encrypt().
>> Is it really needed?
>
> Yes, because at this point we cannot tell the sender to back off.
>
>> For cts(cbc(aes)) with cbc(aes) offloaded in HW, i.e. running in async
>> mode, we get the below stack for CAAM driver.
>> Driver is told that it can sleep (CRYPTO_TFM_REQ_MAY_BACKLOG flag), so
>> it uses GFP_KERNEL to allocate memory. However, this is incorrect, since
>> driver runs in atomic context (softirq).
>
> This is wrong. Whether you can sleep or not is determined by
> MAY_SLEEP, not MAY_BACKLOG. MAY_BACKLOG only indicates that this
> request must be queued, even if the queue is full.
>
Indeed, CAAM driver incorrectly decides to use GFP_KERNEL for allocation
when MAY_BACKLOG flag is set. This seems to be a long-standing issue, I
will send a fix (separately).

Still I think we have a problem.
David reported that the user is fscrypt. Looking into fscrypt code, I
see that besides MAY_BACKLOG, MAY_SLEEP flag is also set. So we end up
in the situation I described earlier: the last block is encrypted in
atomic context and with MAY_SLEEP set.

Thanks,
Horia

2017-06-17 09:06:00

[permalink] [raw]

Subject: Re: [RFC PATCH 0/2] crypto: caam - fix cts(cbc(aes)) with CAAM driver

Horia,

> On 16 Jun 2017, at 23:01, Horia Geantă <[email protected]> wrote:
>
> On 6/16/2017 11:00 AM, Herbert Xu wrote:
>> On Fri, Jun 16, 2017 at 07:57:00AM +0000, Horia Geantă wrote:
>>>
>>> Commit 0605c41cc53ca ("crypto: cts - Convert to skcipher") appends
>>> CRYPTO_TFM_REQ_MAY_BACKLOG to the original crypto request flags for the
>>> last block - when calling cts_cbc_encrypt().
>>> Is it really needed?
>>
>> Yes, because at this point we cannot tell the sender to back off.
>>
>>> For cts(cbc(aes)) with cbc(aes) offloaded in HW, i.e. running in async
>>> mode, we get the below stack for CAAM driver.
>>> Driver is told that it can sleep (CRYPTO_TFM_REQ_MAY_BACKLOG flag), so
>>> it uses GFP_KERNEL to allocate memory. However, this is incorrect, since
>>> driver runs in atomic context (softirq).
>>
>> This is wrong. Whether you can sleep or not is determined by
>> MAY_SLEEP, not MAY_BACKLOG. MAY_BACKLOG only indicates that this
>> request must be queued, even if the queue is full.
>>
> Indeed, CAAM driver incorrectly decides to use GFP_KERNEL for allocation
> when MAY_BACKLOG flag is set. This seems to be a long-standing issue, I
> will send a fix (separately).
>
> Still I think we have a problem.
> David reported that the user is fscrypt. Looking into fscrypt code, I
> see that besides MAY_BACKLOG, MAY_SLEEP flag is also set. So we end up
> in the situation I described earlier: the last block is encrypted in
> atomic context and with MAY_SLEEP set.

Fixing the MAY_BACKLOG issue in the CAAM driver should get rid of the problem
altogether since the CTS code clears the MAY_SLEEP flag when encrypting the
last block [1].

David

[1] http://elixir.free-electrons.com/linux/v4.12-rc5/source/crypto/cts.c#L124

2017-06-19 08:50:19

[permalink] [raw]

Subject: [PATCH 1/2] crypto: caam - fix gfp allocation flags (part I)

Changes in the SW cts (ciphertext stealing) code in
commit 0605c41cc53ca ("crypto: cts - Convert to skcipher")
revealed a problem in the CAAM driver:
when cts(cbc(aes)) is executed and cts runs in SW,
cbc(aes) is offloaded in CAAM; cts encrypts the last block
in atomic context and CAAM incorrectly decides to use GFP_KERNEL
for memory allocation.

Fix this by allowing GFP_KERNEL (sleeping) only when MAY_SLEEP flag is
set, i.e. remove MAY_BACKLOG flag.

We split the fix in two parts - first is sent to -stable, while the
second is not (since there is no known failure case).

Link: http://lkml.kernel.org/g/[email protected]
Cc: <[email protected]> # 4.8+
Reported-by: David Gstir <[email protected]>
Signed-off-by: Horia Geantă <[email protected]>
---
drivers/crypto/caam/caamalg.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 398807d1b77e..4ecf92e3b404 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -1475,8 +1475,7 @@ static struct ablkcipher_edesc *ablkcipher_edesc_alloc(struct ablkcipher_request
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
struct caam_ctx *ctx = crypto_ablkcipher_ctx(ablkcipher);
struct device *jrdev = ctx->jrdev;
- gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ?
+ gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
GFP_KERNEL : GFP_ATOMIC;
int src_nents, mapped_src_nents, dst_nents = 0, mapped_dst_nents = 0;
struct ablkcipher_edesc *edesc;
--
2.12.0.264.gd6db3f216544

2017-06-19 08:50:46

[permalink] [raw]

Subject: [PATCH 2/2] crypto: caam - fix gfp allocation flags (part II)

This is the 2nd part of fixing the usage of GFP_KERNEL for memory
allocations, taking care off all the places that haven't caused a real
problem / failure.
Again, the issue being fixed is that GFP_KERNEL should be used only when
MAY_SLEEP flag is set, i.e. MAY_BACKLOG flag usage is orthogonal.

Signed-off-by: Horia Geantă <[email protected]>
---
drivers/crypto/caam/caamalg.c | 7 +++----
drivers/crypto/caam/caamalg_qi.c | 10 ++++------
drivers/crypto/caam/caamhash.c | 32 ++++++++++++++++----------------
drivers/crypto/caam/caampkc.c | 4 ++--
4 files changed, 25 insertions(+), 28 deletions(-)

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 4ecf92e3b404..fde399c88779 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -1187,8 +1187,8 @@ static struct aead_edesc *aead_edesc_alloc(struct aead_request *req,
struct crypto_aead *aead = crypto_aead_reqtfm(req);
struct caam_ctx *ctx = crypto_aead_ctx(aead);
struct device *jrdev = ctx->jrdev;
- gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ? GFP_KERNEL : GFP_ATOMIC;
+ gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
+ GFP_KERNEL : GFP_ATOMIC;
int src_nents, mapped_src_nents, dst_nents = 0, mapped_dst_nents = 0;
struct aead_edesc *edesc;
int sec4_sg_index, sec4_sg_len, sec4_sg_bytes;
@@ -1680,8 +1680,7 @@ static struct ablkcipher_edesc *ablkcipher_giv_edesc_alloc(
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
struct caam_ctx *ctx = crypto_ablkcipher_ctx(ablkcipher);
struct device *jrdev = ctx->jrdev;
- gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ?
+ gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
GFP_KERNEL : GFP_ATOMIC;
int src_nents, mapped_src_nents, dst_nents, mapped_dst_nents;
struct ablkcipher_edesc *edesc;
diff --git a/drivers/crypto/caam/caamalg_qi.c b/drivers/crypto/caam/caamalg_qi.c
index ea0e5b8b9171..78c4c0485c58 100644
--- a/drivers/crypto/caam/caamalg_qi.c
+++ b/drivers/crypto/caam/caamalg_qi.c
@@ -555,8 +555,8 @@ static struct aead_edesc *aead_edesc_alloc(struct aead_request *req,
struct caam_aead_alg *alg = container_of(crypto_aead_alg(aead),
typeof(*alg), aead);
struct device *qidev = ctx->qidev;
- gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ? GFP_KERNEL : GFP_ATOMIC;
+ gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
+ GFP_KERNEL : GFP_ATOMIC;
int src_nents, mapped_src_nents, dst_nents = 0, mapped_dst_nents = 0;
struct aead_edesc *edesc;
dma_addr_t qm_sg_dma, iv_dma = 0;
@@ -808,8 +808,7 @@ static struct ablkcipher_edesc *ablkcipher_edesc_alloc(struct ablkcipher_request
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
struct caam_ctx *ctx = crypto_ablkcipher_ctx(ablkcipher);
struct device *qidev = ctx->qidev;
- gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ?
+ gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
GFP_KERNEL : GFP_ATOMIC;
int src_nents, mapped_src_nents, dst_nents = 0, mapped_dst_nents = 0;
struct ablkcipher_edesc *edesc;
@@ -953,8 +952,7 @@ static struct ablkcipher_edesc *ablkcipher_giv_edesc_alloc(
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
struct caam_ctx *ctx = crypto_ablkcipher_ctx(ablkcipher);
struct device *qidev = ctx->qidev;
- gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ?
+ gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
GFP_KERNEL : GFP_ATOMIC;
int src_nents, mapped_src_nents, dst_nents, mapped_dst_nents;
struct ablkcipher_edesc *edesc;
diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index da4f94eab3da..7c44c90ad593 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -719,8 +719,8 @@ static int ahash_update_ctx(struct ahash_request *req)
struct caam_hash_ctx *ctx = crypto_ahash_ctx(ahash);
struct caam_hash_state *state = ahash_request_ctx(req);
struct device *jrdev = ctx->jrdev;
- gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ? GFP_KERNEL : GFP_ATOMIC;
+ gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
+ GFP_KERNEL : GFP_ATOMIC;
u8 *buf = current_buf(state);
int *buflen = current_buflen(state);
u8 *next_buf = alt_buf(state);
@@ -849,8 +849,8 @@ static int ahash_final_ctx(struct ahash_request *req)
struct caam_hash_ctx *ctx = crypto_ahash_ctx(ahash);
struct caam_hash_state *state = ahash_request_ctx(req);
struct device *jrdev = ctx->jrdev;
- gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ? GFP_KERNEL : GFP_ATOMIC;
+ gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
+ GFP_KERNEL : GFP_ATOMIC;
int buflen = *current_buflen(state);
u32 *desc;
int sec4_sg_bytes, sec4_sg_src_index;
@@ -926,8 +926,8 @@ static int ahash_finup_ctx(struct ahash_request *req)
struct caam_hash_ctx *ctx = crypto_ahash_ctx(ahash);
struct caam_hash_state *state = ahash_request_ctx(req);
struct device *jrdev = ctx->jrdev;
- gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ? GFP_KERNEL : GFP_ATOMIC;
+ gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
+ GFP_KERNEL : GFP_ATOMIC;
int buflen = *current_buflen(state);
u32 *desc;
int sec4_sg_src_index;
@@ -1013,8 +1013,8 @@ static int ahash_digest(struct ahash_request *req)
struct caam_hash_ctx *ctx = crypto_ahash_ctx(ahash);
struct caam_hash_state *state = ahash_request_ctx(req);
struct device *jrdev = ctx->jrdev;
- gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ? GFP_KERNEL : GFP_ATOMIC;
+ gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
+ GFP_KERNEL : GFP_ATOMIC;
u32 *desc;
int digestsize = crypto_ahash_digestsize(ahash);
int src_nents, mapped_nents;
@@ -1093,8 +1093,8 @@ static int ahash_final_no_ctx(struct ahash_request *req)
struct caam_hash_ctx *ctx = crypto_ahash_ctx(ahash);
struct caam_hash_state *state = ahash_request_ctx(req);
struct device *jrdev = ctx->jrdev;
- gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ? GFP_KERNEL : GFP_ATOMIC;
+ gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
+ GFP_KERNEL : GFP_ATOMIC;
u8 *buf = current_buf(state);
int buflen = *current_buflen(state);
u32 *desc;
@@ -1154,8 +1154,8 @@ static int ahash_update_no_ctx(struct ahash_request *req)
struct caam_hash_ctx *ctx = crypto_ahash_ctx(ahash);
struct caam_hash_state *state = ahash_request_ctx(req);
struct device *jrdev = ctx->jrdev;
- gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ? GFP_KERNEL : GFP_ATOMIC;
+ gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
+ GFP_KERNEL : GFP_ATOMIC;
u8 *buf = current_buf(state);
int *buflen = current_buflen(state);
u8 *next_buf = alt_buf(state);
@@ -1280,8 +1280,8 @@ static int ahash_finup_no_ctx(struct ahash_request *req)
struct caam_hash_ctx *ctx = crypto_ahash_ctx(ahash);
struct caam_hash_state *state = ahash_request_ctx(req);
struct device *jrdev = ctx->jrdev;
- gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ? GFP_KERNEL : GFP_ATOMIC;
+ gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
+ GFP_KERNEL : GFP_ATOMIC;
int buflen = *current_buflen(state);
u32 *desc;
int sec4_sg_bytes, sec4_sg_src_index, src_nents, mapped_nents;
@@ -1370,8 +1370,8 @@ static int ahash_update_first(struct ahash_request *req)
struct caam_hash_ctx *ctx = crypto_ahash_ctx(ahash);
struct caam_hash_state *state = ahash_request_ctx(req);
struct device *jrdev = ctx->jrdev;
- gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ? GFP_KERNEL : GFP_ATOMIC;
+ gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
+ GFP_KERNEL : GFP_ATOMIC;
u8 *next_buf = alt_buf(state);
int *next_buflen = alt_buflen(state);
int to_hash;
diff --git a/drivers/crypto/caam/caampkc.c b/drivers/crypto/caam/caampkc.c
index 9c508ba6b0f1..7a897209f181 100644
--- a/drivers/crypto/caam/caampkc.c
+++ b/drivers/crypto/caam/caampkc.c
@@ -173,8 +173,8 @@ static struct rsa_edesc *rsa_edesc_alloc(struct akcipher_request *req,
struct caam_rsa_ctx *ctx = akcipher_tfm_ctx(tfm);
struct device *dev = ctx->dev;
struct rsa_edesc *edesc;
- gfp_t flags = (req->base.flags & (CRYPTO_TFM_REQ_MAY_BACKLOG |
- CRYPTO_TFM_REQ_MAY_SLEEP)) ? GFP_KERNEL : GFP_ATOMIC;
+ gfp_t flags = (req->base.flags & CRYPTO_TFM_REQ_MAY_SLEEP) ?
+ GFP_KERNEL : GFP_ATOMIC;
int sgc;
int sec4_sg_index, sec4_sg_len = 0, sec4_sg_bytes;
int src_nents, dst_nents;
--
2.12.0.264.gd6db3f216544

2017-06-19 10:31:34

[permalink] [raw]

Subject: Re: [RFC PATCH 1/2] crypto: caam - properly set IV after {en,de}crypt

On 6/2/2017 3:25 PM, David Gstir wrote:
> Certain cipher modes like CTS expect the IV (req->info) of
> ablkcipher_request (or equivalently req->iv of skcipher_request) to
> contain the last ciphertext block when the {en,de}crypt operation is done.
> This is currently not the case for the CAAM driver which in turn breaks
> e.g. cts(cbc(aes)) when the CAAM driver is enabled.
>
> This patch fixes the CAAM driver to properly set the IV after the
> {en,de}crypt operation of ablkcipher finishes.
>
> Signed-off-by: David Gstir <[email protected]>
> ---
> drivers/crypto/caam/caamalg.c | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
> index 398807d1b77e..d13c1aee4427 100644
> --- a/drivers/crypto/caam/caamalg.c
> +++ b/drivers/crypto/caam/caamalg.c
> @@ -882,10 +882,11 @@ static void ablkcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err,
> {
> struct ablkcipher_request *req = context;
> struct ablkcipher_edesc *edesc;
> -#ifdef DEBUG
> struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
> int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
> + int nents;
>
> +#ifdef DEBUG
> dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
> #endif
>
> @@ -904,6 +905,19 @@ static void ablkcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err,
> #endif
>
> ablkcipher_unmap(jrdev, edesc, req);
> +
> + if (req->src == req->dst)
> + nents = edesc->src_nents;
> + else
> + nents = edesc->dst_nents;
> +
> + /*
> + * The crypto API expects us to set the IV (req->info) to the last
> + * ciphertext block. This is used e.g. by the CTS mode.
> + */

IIUC, IV update is required only in case of CBC.
Since this callback is used also for CTR, we should avoid the copy:
if ((ctx->cdata.algtype & OP_ALG_AAI_MASK) == OP_ALG_AAI_CBC) ...

> + sg_pcopy_to_buffer(req->dst, nents, req->info, ivsize,
> + req->nbytes - ivsize);

scatterwalk_map_and_copy() should be used instead.

> +
> kfree(edesc);
>
> ablkcipher_request_complete(req, err);
> @@ -914,10 +928,10 @@ static void ablkcipher_decrypt_done(struct device *jrdev, u32 *desc, u32 err,
> {
> struct ablkcipher_request *req = context;
> struct ablkcipher_edesc *edesc;
> -#ifdef DEBUG
> struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
> int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
>
> +#ifdef DEBUG
> dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
> #endif
>
> @@ -935,6 +949,14 @@ static void ablkcipher_decrypt_done(struct device *jrdev, u32 *desc, u32 err,
> #endif
>
> ablkcipher_unmap(jrdev, edesc, req);
> +
> + /*
> + * The crypto API expects us to set the IV (req->info) to the last
> + * ciphertext block.
> + */
> + sg_pcopy_to_buffer(req->src, edesc->src_nents, req->info, ivsize,
> + req->nbytes - ivsize);
> +
> kfree(edesc);
>
> ablkcipher_request_complete(req, err);
>

2017-06-20 02:22:16

[permalink] [raw]

Subject: Re: [RFC PATCH 1/2] crypto: caam - properly set IV after {en,de}crypt

On Mon, Jun 19, 2017 at 10:31:27AM +0000, Horia Geantă wrote:
>
> IIUC, IV update is required only in case of CBC.
> Since this callback is used also for CTR, we should avoid the copy:
> if ((ctx->cdata.algtype & OP_ALG_AAI_MASK) == OP_ALG_AAI_CBC) ...

No it is needed for CTR too.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2017-06-22 09:00:30

[permalink] [raw]

Subject: Re: [PATCH 1/2] crypto: caam - fix gfp allocation flags (part I)

On Mon, Jun 19, 2017 at 11:44:45AM +0300, Horia Geantă wrote:
> Changes in the SW cts (ciphertext stealing) code in
> commit 0605c41cc53ca ("crypto: cts - Convert to skcipher")
> revealed a problem in the CAAM driver:
> when cts(cbc(aes)) is executed and cts runs in SW,
> cbc(aes) is offloaded in CAAM; cts encrypts the last block
> in atomic context and CAAM incorrectly decides to use GFP_KERNEL
> for memory allocation.
>
> Fix this by allowing GFP_KERNEL (sleeping) only when MAY_SLEEP flag is
> set, i.e. remove MAY_BACKLOG flag.
>
> We split the fix in two parts - first is sent to -stable, while the
> second is not (since there is no known failure case).
>
> Link: http://lkml.kernel.org/g/[email protected]
> Cc: <[email protected]> # 4.8+
> Reported-by: David Gstir <[email protected]>
> Signed-off-by: Horia Geantă <[email protected]>

Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2017-06-22 09:00:50

[permalink] [raw]

Subject: Re: [PATCH 2/2] crypto: caam - fix gfp allocation flags (part II)

On Mon, Jun 19, 2017 at 11:44:46AM +0300, Horia Geantă wrote:
> This is the 2nd part of fixing the usage of GFP_KERNEL for memory
> allocations, taking care off all the places that haven't caused a real
> problem / failure.
> Again, the issue being fixed is that GFP_KERNEL should be used only when
> MAY_SLEEP flag is set, i.e. MAY_BACKLOG flag usage is orthogonal.
>
> Signed-off-by: Horia Geantă <[email protected]>

Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2017-06-26 05:41:08

[permalink] [raw]

Subject: Re: [RFC PATCH 1/2] crypto: caam - properly set IV after {en,de}crypt

Herbert,

> On 20 Jun 2017, at 03:28, Herbert Xu <[email protected]> wrote:
>
> On Mon, Jun 19, 2017 at 10:31:27AM +0000, Horia Geantă wrote:
>>
>> IIUC, IV update is required only in case of CBC.
>> Since this callback is used also for CTR, we should avoid the copy:
>> if ((ctx->cdata.algtype & OP_ALG_AAI_MASK) == OP_ALG_AAI_CBC) ...
>
> No it is needed for CTR too.

So, am I correct in assuming that it is required for all modes including AEAD modes like GCM?
In that case I'll include a fix for the CAAM GCM mode too.

Thanks,
David

2017-06-26 06:48:17

[permalink] [raw]

Subject: Re: [RFC PATCH 1/2] crypto: caam - properly set IV after {en,de}crypt

On Mon, Jun 26, 2017 at 07:40:58AM +0200, David Gstir wrote:
>
> So, am I correct in assuming that it is required for all modes including AEAD modes like GCM?
> In that case I'll include a fix for the CAAM GCM mode too.

It's only required for skcihper. As we do not do chunking/streaming
with our AEAD interface it is not required for GCM.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2017-06-28 08:32:21

[permalink] [raw]

Subject: Re: [RFC PATCH 1/2] crypto: caam - properly set IV after {en,de}crypt

On 6/19/2017 1:31 PM, Horia Geant? wrote:
> On 6/2/2017 3:25 PM, David Gstir wrote:
>> Certain cipher modes like CTS expect the IV (req->info) of
>> ablkcipher_request (or equivalently req->iv of skcipher_request) to
>> contain the last ciphertext block when the {en,de}crypt operation is done.
>> This is currently not the case for the CAAM driver which in turn breaks
>> e.g. cts(cbc(aes)) when the CAAM driver is enabled.
>>
>> This patch fixes the CAAM driver to properly set the IV after the
>> {en,de}crypt operation of ablkcipher finishes.
>>
>> Signed-off-by: David Gstir <[email protected]>
>> ---
>> drivers/crypto/caam/caamalg.c | 26 ++++++++++++++++++++++++--
>> 1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
>> index 398807d1b77e..d13c1aee4427 100644
>> --- a/drivers/crypto/caam/caamalg.c
>> +++ b/drivers/crypto/caam/caamalg.c
>> @@ -882,10 +882,11 @@ static void ablkcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err,
>> {
>> struct ablkcipher_request *req = context;
>> struct ablkcipher_edesc *edesc;
>> -#ifdef DEBUG
>> struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
>> int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
>> + int nents;
>>
>> +#ifdef DEBUG
>> dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
>> #endif
>>
>> @@ -904,6 +905,19 @@ static void ablkcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err,
>> #endif
[snip]
>> + sg_pcopy_to_buffer(req->dst, nents, req->info, ivsize,
>> + req->nbytes - ivsize);
>
> scatterwalk_map_and_copy() should be used instead.
>
David, IIUC this is the only change needed in this patch (applies both
for encryption and decryption, of course).
Will you formally resubmit?

Thanks,
Horia

2017-06-28 09:03:55

[permalink] [raw]

Subject: Re: [RFC PATCH 1/2] crypto: caam - properly set IV after {en,de}crypt

Horia,

> On 28 Jun 2017, at 10:32, Horia Geantă <[email protected]> wrote:
>
>>> + sg_pcopy_to_buffer(req->dst, nents, req->info, ivsize,
>>> + req->nbytes - ivsize);
>>
>> scatterwalk_map_and_copy() should be used instead.
>>
> David, IIUC this is the only change needed in this patch (applies both
> for encryption and decryption, of course).
> Will you formally resubmit?

Thanks for the reminder. I did not get arround it yet.
Will send the new patch within the next few hours.

Thanks,
David

2017-06-28 13:27:45

[permalink] [raw]

Subject: [PATCH] crypto: caam - properly set IV after {en,de}crypt

Certain cipher modes like CTS expect the IV (req->info) of
ablkcipher_request (or equivalently req->iv of skcipher_request) to
contain the last ciphertext block when the {en,de}crypt operation is done.
This is currently not the case for the CAAM driver which in turn breaks
e.g. cts(cbc(aes)) when the CAAM driver is enabled.

This patch fixes the CAAM driver to properly set the IV after the
{en,de}crypt operation of ablkcipher finishes.

This issue was revealed by the changes in the SW CTS mode in commit
0605c41cc53ca ("crypto: cts - Convert to skcipher")

Cc: <[email protected]> # 4.8+
Signed-off-by: David Gstir <[email protected]>
---
drivers/crypto/caam/caamalg.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index 398807d1b77e..c45b5bf65254 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -882,10 +882,10 @@ static void ablkcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err,
{
struct ablkcipher_request *req = context;
struct ablkcipher_edesc *edesc;
-#ifdef DEBUG
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
int ivsize = crypto_ablkcipher_ivsize(ablkcipher);

+#ifdef DEBUG
dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
#endif

@@ -904,6 +904,14 @@ static void ablkcipher_encrypt_done(struct device *jrdev, u32 *desc, u32 err,
#endif

ablkcipher_unmap(jrdev, edesc, req);
+
+ /*
+ * The crypto API expects us to set the IV (req->info) to the last
+ * ciphertext block. This is used e.g. by the CTS mode.
+ */
+ scatterwalk_map_and_copy(req->info, req->dst, req->nbytes - ivsize,
+ ivsize, 0);
+
kfree(edesc);

ablkcipher_request_complete(req, err);
@@ -914,10 +922,10 @@ static void ablkcipher_decrypt_done(struct device *jrdev, u32 *desc, u32 err,
{
struct ablkcipher_request *req = context;
struct ablkcipher_edesc *edesc;
-#ifdef DEBUG
struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
int ivsize = crypto_ablkcipher_ivsize(ablkcipher);

+#ifdef DEBUG
dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
#endif

@@ -935,6 +943,14 @@ static void ablkcipher_decrypt_done(struct device *jrdev, u32 *desc, u32 err,
#endif

ablkcipher_unmap(jrdev, edesc, req);
+
+ /*
+ * The crypto API expects us to set the IV (req->info) to the last
+ * ciphertext block.
+ */
+ scatterwalk_map_and_copy(req->info, req->src, req->nbytes - ivsize,
+ ivsize, 0);
+
kfree(edesc);

ablkcipher_request_complete(req, err);
--
2.12.3

2017-06-28 13:43:06

[permalink] [raw]

Subject: Re: [PATCH] crypto: caam - properly set IV after {en,de}crypt

On 6/28/2017 4:27 PM, David Gstir wrote:
> Certain cipher modes like CTS expect the IV (req->info) of
> ablkcipher_request (or equivalently req->iv of skcipher_request) to
> contain the last ciphertext block when the {en,de}crypt operation is done.
> This is currently not the case for the CAAM driver which in turn breaks
> e.g. cts(cbc(aes)) when the CAAM driver is enabled.
>
> This patch fixes the CAAM driver to properly set the IV after the
> {en,de}crypt operation of ablkcipher finishes.
>
> This issue was revealed by the changes in the SW CTS mode in commit
> 0605c41cc53ca ("crypto: cts - Convert to skcipher")
>
> Cc: <[email protected]> # 4.8+
> Signed-off-by: David Gstir <[email protected]>
Reviewed-by: Horia Geant? <[email protected]>

Thanks,
Horia

2017-06-29 10:19:13

[permalink] [raw]

Subject: Re: [PATCH] crypto: caam - properly set IV after {en,de}crypt

On 6/28/2017 4:42 PM, Horia Geant? wrote:
> On 6/28/2017 4:27 PM, David Gstir wrote:
>> Certain cipher modes like CTS expect the IV (req->info) of
>> ablkcipher_request (or equivalently req->iv of skcipher_request) to
>> contain the last ciphertext block when the {en,de}crypt operation is done.
>> This is currently not the case for the CAAM driver which in turn breaks
>> e.g. cts(cbc(aes)) when the CAAM driver is enabled.
>>
>> This patch fixes the CAAM driver to properly set the IV after the
>> {en,de}crypt operation of ablkcipher finishes.
>>
>> This issue was revealed by the changes in the SW CTS mode in commit
>> 0605c41cc53ca ("crypto: cts - Convert to skcipher")
>>
>> Cc: <[email protected]> # 4.8+
>> Signed-off-by: David Gstir <[email protected]>
> Reviewed-by: Horia Geant? <[email protected]>
>
Btw, instead of updating the IV in SW, CAAM engine could be programmed
to do it - by saving the Context Register of the AES accelerator.

Unfortunately this would require changes in quite a few places: shared
descriptor, HW S/G generation logic, IV dma (un)mapping and maybe others.

So it's better to have this fix now (which, considering size, is
appropriate for -stable) and later, if needed, offload IV updating in HW.

Regards,
Horia

2017-07-12 10:52:30

[permalink] [raw]

Subject: Re: [PATCH] crypto: caam - properly set IV after {en,de}crypt

On Wed, Jun 28, 2017 at 03:27:10PM +0200, David Gstir wrote:
> Certain cipher modes like CTS expect the IV (req->info) of
> ablkcipher_request (or equivalently req->iv of skcipher_request) to
> contain the last ciphertext block when the {en,de}crypt operation is done.
> This is currently not the case for the CAAM driver which in turn breaks
> e.g. cts(cbc(aes)) when the CAAM driver is enabled.
>
> This patch fixes the CAAM driver to properly set the IV after the
> {en,de}crypt operation of ablkcipher finishes.
>
> This issue was revealed by the changes in the SW CTS mode in commit
> 0605c41cc53ca ("crypto: cts - Convert to skcipher")
>
> Cc: <[email protected]> # 4.8+
> Signed-off-by: David Gstir <[email protected]>

Patch applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2017-08-14 07:59:25

by Gilad Ben-Yossef

[permalink] [raw]

Subject: Re: [PATCH] crypto: caam - properly set IV after {en,de}crypt

Hi,

On Thu, Jun 29, 2017 at 1:19 PM, Horia Geantă <[email protected]> wrote:
> On 6/28/2017 4:42 PM, Horia Geantă wrote:
>> On 6/28/2017 4:27 PM, David Gstir wrote:
>>> Certain cipher modes like CTS expect the IV (req->info) of
>>> ablkcipher_request (or equivalently req->iv of skcipher_request) to
>>> contain the last ciphertext block when the {en,de}crypt operation is done.
>>> This is currently not the case for the CAAM driver which in turn breaks
>>> e.g. cts(cbc(aes)) when the CAAM driver is enabled.
>>>
>>> This patch fixes the CAAM driver to properly set the IV after the
>>> {en,de}crypt operation of ablkcipher finishes.
>>>
>>> This issue was revealed by the changes in the SW CTS mode in commit
>>> 0605c41cc53ca ("crypto: cts - Convert to skcipher")
>>>
>>> Cc: <[email protected]> # 4.8+
>>> Signed-off-by: David Gstir <[email protected]>
>> Reviewed-by: Horia Geantă <[email protected]>
>>
> Btw, instead of updating the IV in SW, CAAM engine could be programmed
> to do it - by saving the Context Register of the AES accelerator.
>
> Unfortunately this would require changes in quite a few places: shared
> descriptor, HW S/G generation logic, IV dma (un)mapping and maybe others.
>
> So it's better to have this fix now (which, considering size, is
> appropriate for -stable) and later, if needed, offload IV updating in HW.
>

My apologies for reviving this thread from the dead, but doesn't the patch fail
for in-place decryption since we are copying from req->dst after
the operation is done, and therefore it no longer contains the ciphertext?

I'm asking since I ran into a similar issue in the ccree driver and thought
to deploy a similar fix but could not convince myself why this works.

Thanks!
Gilad

--
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
-- Jean-Baptiste Queru

2017-09-05 15:33:10

[permalink] [raw]

Subject: Re: [PATCH] crypto: caam - properly set IV after {en,de}crypt

On 8/14/2017 10:59 AM, Gilad Ben-Yossef wrote:
> Hi,
>
> On Thu, Jun 29, 2017 at 1:19 PM, Horia Geant? <[email protected]> wrote:
>> On 6/28/2017 4:42 PM, Horia Geant? wrote:
>>> On 6/28/2017 4:27 PM, David Gstir wrote:
>>>> Certain cipher modes like CTS expect the IV (req->info) of
>>>> ablkcipher_request (or equivalently req->iv of skcipher_request) to
>>>> contain the last ciphertext block when the {en,de}crypt operation is done.
>>>> This is currently not the case for the CAAM driver which in turn breaks
>>>> e.g. cts(cbc(aes)) when the CAAM driver is enabled.
>>>>
>>>> This patch fixes the CAAM driver to properly set the IV after the
>>>> {en,de}crypt operation of ablkcipher finishes.
>>>>
>>>> This issue was revealed by the changes in the SW CTS mode in commit
>>>> 0605c41cc53ca ("crypto: cts - Convert to skcipher")
>>>>
>>>> Cc: <[email protected]> # 4.8+
>>>> Signed-off-by: David Gstir <[email protected]>
>>> Reviewed-by: Horia Geant? <[email protected]>
>>>
>> Btw, instead of updating the IV in SW, CAAM engine could be programmed
>> to do it - by saving the Context Register of the AES accelerator.
>>
>> Unfortunately this would require changes in quite a few places: shared
>> descriptor, HW S/G generation logic, IV dma (un)mapping and maybe others.
>>
>> So it's better to have this fix now (which, considering size, is
>> appropriate for -stable) and later, if needed, offload IV updating in HW.
>>
>
> My apologies for reviving this thread from the dead, but doesn't the patch fail
> for in-place decryption since we are copying from req->dst after
> the operation is done, and therefore it no longer contains the ciphertext?
>
You are right, thanks! Will follow up with a fix.
Though cts(cbc(aes)) in particular is working, see below.

> I'm asking since I ran into a similar issue in the ccree driver and thought
> to deploy a similar fix but could not convince myself why this works.
>
IIUC cts(cbc(aes)) in-place decryption (with cbc(aes) offloaded to CAAM
engine) works since SW implementation of cts, when performing the
ciphertext stealing phase in cts_cbc_decrypt() does not use req->iv, but
a previously value, saved before staring decryption in crypto_cts_decrypt():

if (cbc_blocks <= 1)
memcpy(space, req->iv, bsize);
else
scatterwalk_map_and_copy(space, req->src, offset - 2 * bsize,
bsize, 0);

Horia

2017-09-06 10:14:32

by Gilad Ben-Yossef

[permalink] [raw]

Subject: Re: [PATCH] crypto: caam - properly set IV after {en,de}crypt

On Tue, Sep 5, 2017 at 6:33 PM, Horia Geantă <[email protected]> wrote:
> On 8/14/2017 10:59 AM, Gilad Ben-Yossef wrote:
>> Hi,
>>
>> On Thu, Jun 29, 2017 at 1:19 PM, Horia Geantă <[email protected]> wrote:
>>> On 6/28/2017 4:42 PM, Horia Geantă wrote:
>>>> On 6/28/2017 4:27 PM, David Gstir wrote:
>>>>> Certain cipher modes like CTS expect the IV (req->info) of
>>>>> ablkcipher_request (or equivalently req->iv of skcipher_request) to
>>>>> contain the last ciphertext block when the {en,de}crypt operation is done.
>>>>> This is currently not the case for the CAAM driver which in turn breaks
>>>>> e.g. cts(cbc(aes)) when the CAAM driver is enabled.
>>>>>
>>>>> This patch fixes the CAAM driver to properly set the IV after the
>>>>> {en,de}crypt operation of ablkcipher finishes.
>>>>>
>>>>> This issue was revealed by the changes in the SW CTS mode in commit
>>>>> 0605c41cc53ca ("crypto: cts - Convert to skcipher")
>>>>>
>>>>> Cc: <[email protected]> # 4.8+
>>>>> Signed-off-by: David Gstir <[email protected]>
>>>> Reviewed-by: Horia Geantă <[email protected]>
>>>>
>>> Btw, instead of updating the IV in SW, CAAM engine could be programmed
>>> to do it - by saving the Context Register of the AES accelerator.
>>>
>>> Unfortunately this would require changes in quite a few places: shared
>>> descriptor, HW S/G generation logic, IV dma (un)mapping and maybe others.
>>>
>>> So it's better to have this fix now (which, considering size, is
>>> appropriate for -stable) and later, if needed, offload IV updating in HW.
>>>
>>
>> My apologies for reviving this thread from the dead, but doesn't the patch fail
>> for in-place decryption since we are copying from req->dst after
>> the operation is done, and therefore it no longer contains the ciphertext?
>>
> You are right, thanks! Will follow up with a fix.
> Though cts(cbc(aes)) in particular is working, see below.
>
>> I'm asking since I ran into a similar issue in the ccree driver and thought
>> to deploy a similar fix but could not convince myself why this works.
>>
> IIUC cts(cbc(aes)) in-place decryption (with cbc(aes) offloaded to CAAM
> engine) works since SW implementation of cts, when performing the
> ciphertext stealing phase in cts_cbc_decrypt() does not use req->iv, but
> a previously value, saved before staring decryption in crypto_cts_decrypt():
>
> if (cbc_blocks <= 1)
> memcpy(space, req->iv, bsize);
> else
> scatterwalk_map_and_copy(space, req->src, offset - 2 * bsize,
> bsize, 0);
>

Is that not a performance bug in software CTS than? I mean all those
transformation
drivers doing that extra copy and possibly malloc and free to save the
data for the info
and than have the CTS implementation ignore that and do its own memory copy?

Gilad
--
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
-- Jean-Baptiste Queru

2017-09-07 10:13:06