Commit 8efd972ef96a ("crypto: testmgr - support checking skcipher output IV")
caused the crypto4xx driver to produce the following error:
| ctr-aes-ppc4xx encryption test failed (wrong output IV)
| on test vector 0, cfg="in-place"
This patch fixes this by reworking the crypto4xx_setkey_aes()
function to:
- not save the iv for ECB (as per 18.2.38 CRYP0_SA_CMD_0:
"This bit mut be cleared for DES ECB mode or AES ECB mode,
when no IV is used.")
- instruct the hardware to save the generated IV for all
other modes of operations that have IV and then supply
it back to the callee in pretty much the same way as we
do it for cbc-aes already.
- make it clear that the DIR_(IN|OUT)BOUND is the important
bit that tells the hardware to encrypt or decrypt the data.
(this is cosmetic - but it hopefully prevents me from
getting confused again).
- don't load any bogus hash when we don't use any hash
operation to begin with.
Cc: [email protected]
Signed-off-by: Christian Lamparter <[email protected]>
---
drivers/crypto/amcc/crypto4xx_alg.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/crypto/amcc/crypto4xx_alg.c b/drivers/crypto/amcc/crypto4xx_alg.c
index 4092c2aad8e2..3458c5a085d9 100644
--- a/drivers/crypto/amcc/crypto4xx_alg.c
+++ b/drivers/crypto/amcc/crypto4xx_alg.c
@@ -141,9 +141,10 @@ static int crypto4xx_setkey_aes(struct crypto_skcipher *cipher,
/* Setup SA */
sa = ctx->sa_in;
- set_dynamic_sa_command_0(sa, SA_NOT_SAVE_HASH, (cm == CRYPTO_MODE_CBC ?
- SA_SAVE_IV : SA_NOT_SAVE_IV),
- SA_LOAD_HASH_FROM_SA, SA_LOAD_IV_FROM_STATE,
+ set_dynamic_sa_command_0(sa, SA_NOT_SAVE_HASH, (cm == CRYPTO_MODE_ECB ?
+ SA_NOT_SAVE_IV : SA_SAVE_IV),
+ SA_NOT_LOAD_HASH, (cm == CRYPTO_MODE_ECB ?
+ SA_LOAD_IV_FROM_SA : SA_LOAD_IV_FROM_STATE),
SA_NO_HEADER_PROC, SA_HASH_ALG_NULL,
SA_CIPHER_ALG_AES, SA_PAD_TYPE_ZERO,
SA_OP_GROUP_BASIC, SA_OPCODE_DECRYPT,
@@ -162,6 +163,11 @@ static int crypto4xx_setkey_aes(struct crypto_skcipher *cipher,
memcpy(ctx->sa_out, ctx->sa_in, ctx->sa_len * 4);
sa = ctx->sa_out;
sa->sa_command_0.bf.dir = DIR_OUTBOUND;
+ /*
+ * SA_OPCODE_ENCRYPT is the same value as SA_OPCODE_DECRYPT.
+ * it's the DIR_(IN|OUT)BOUND that matters
+ */
+ sa->sa_command_0.bf.opcode = SA_OPCODE_ENCRYPT;
return 0;
}
--
2.20.1
using_sd is used as a stand-in for sa_command_0.bf.scatter
that we need to set anyway, so we might as well just prevent
double-accounting.
Signed-off-by: Christian Lamparter <[email protected]>
---
drivers/crypto/amcc/crypto4xx_core.c | 6 ++----
drivers/crypto/amcc/crypto4xx_core.h | 1 -
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/crypto/amcc/crypto4xx_core.c b/drivers/crypto/amcc/crypto4xx_core.c
index 3e7d24ff3fa6..3934c2523762 100644
--- a/drivers/crypto/amcc/crypto4xx_core.c
+++ b/drivers/crypto/amcc/crypto4xx_core.c
@@ -539,7 +539,7 @@ static void crypto4xx_cipher_done(struct crypto4xx_device *dev,
req = skcipher_request_cast(pd_uinfo->async_req);
- if (pd_uinfo->using_sd) {
+ if (pd_uinfo->sa_va->sa_command_0.bf.scatter) {
crypto4xx_copy_pkt_to_dst(dev, pd, pd_uinfo,
req->cryptlen, req->dst);
} else {
@@ -593,7 +593,7 @@ static void crypto4xx_aead_done(struct crypto4xx_device *dev,
u32 icv[AES_BLOCK_SIZE];
int err = 0;
- if (pd_uinfo->using_sd) {
+ if (pd_uinfo->sa_va->sa_command_0.bf.scatter) {
crypto4xx_copy_pkt_to_dst(dev, pd, pd_uinfo,
pd->pd_ctl_len.bf.pkt_len,
dst);
@@ -887,7 +887,6 @@ int crypto4xx_build_pd(struct crypto_async_request *req,
* we know application give us dst a whole piece of memory
* no need to use scatter ring.
*/
- pd_uinfo->using_sd = 0;
pd_uinfo->first_sd = 0xffffffff;
sa->sa_command_0.bf.scatter = 0;
pd->dest = (u32)dma_map_page(dev->core_dev->device,
@@ -901,7 +900,6 @@ int crypto4xx_build_pd(struct crypto_async_request *req,
u32 sd_idx = fst_sd;
nbytes = datalen;
sa->sa_command_0.bf.scatter = 1;
- pd_uinfo->using_sd = 1;
pd_uinfo->first_sd = fst_sd;
sd = crypto4xx_get_sdp(dev, &sd_dma, sd_idx);
pd->dest = sd_dma;
diff --git a/drivers/crypto/amcc/crypto4xx_core.h b/drivers/crypto/amcc/crypto4xx_core.h
index 4ecc34fa8ebd..c624f8cd3d2e 100644
--- a/drivers/crypto/amcc/crypto4xx_core.h
+++ b/drivers/crypto/amcc/crypto4xx_core.h
@@ -64,7 +64,6 @@ union shadow_sa_buf {
struct pd_uinfo {
struct crypto4xx_device *dev;
u32 state;
- u32 using_sd;
u32 first_gd; /* first gather discriptor
used by this packet */
u32 num_gd; /* number of gather discriptor
--
2.20.1
This replaces struct crypto_skcipher and the extra request size
with struct crypto_sync_skcipher and SYNC_SKCIPHER_REQUEST_ON_STACK(),
which uses a fixed stack size.
Signed-off-by: Christian Lamparter <[email protected]>
---
drivers/crypto/amcc/crypto4xx_alg.c | 12 ++++++------
drivers/crypto/amcc/crypto4xx_core.c | 11 +++--------
drivers/crypto/amcc/crypto4xx_core.h | 2 +-
3 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/drivers/crypto/amcc/crypto4xx_alg.c b/drivers/crypto/amcc/crypto4xx_alg.c
index 3458c5a085d9..307f5cfa9ba4 100644
--- a/drivers/crypto/amcc/crypto4xx_alg.c
+++ b/drivers/crypto/amcc/crypto4xx_alg.c
@@ -264,10 +264,10 @@ crypto4xx_ctr_crypt(struct skcipher_request *req, bool encrypt)
* overlow.
*/
if (counter + nblks < counter) {
- struct skcipher_request *subreq = skcipher_request_ctx(req);
+ SYNC_SKCIPHER_REQUEST_ON_STACK(subreq, ctx->sw_cipher.cipher);
int ret;
- skcipher_request_set_tfm(subreq, ctx->sw_cipher.cipher);
+ skcipher_request_set_sync_tfm(subreq, ctx->sw_cipher.cipher);
skcipher_request_set_callback(subreq, req->base.flags,
NULL, NULL);
skcipher_request_set_crypt(subreq, req->src, req->dst,
@@ -289,14 +289,14 @@ static int crypto4xx_sk_setup_fallback(struct crypto4xx_ctx *ctx,
{
int rc;
- crypto_skcipher_clear_flags(ctx->sw_cipher.cipher,
+ crypto_sync_skcipher_clear_flags(ctx->sw_cipher.cipher,
CRYPTO_TFM_REQ_MASK);
- crypto_skcipher_set_flags(ctx->sw_cipher.cipher,
+ crypto_sync_skcipher_set_flags(ctx->sw_cipher.cipher,
crypto_skcipher_get_flags(cipher) & CRYPTO_TFM_REQ_MASK);
- rc = crypto_skcipher_setkey(ctx->sw_cipher.cipher, key, keylen);
+ rc = crypto_sync_skcipher_setkey(ctx->sw_cipher.cipher, key, keylen);
crypto_skcipher_clear_flags(cipher, CRYPTO_TFM_RES_MASK);
crypto_skcipher_set_flags(cipher,
- crypto_skcipher_get_flags(ctx->sw_cipher.cipher) &
+ crypto_sync_skcipher_get_flags(ctx->sw_cipher.cipher) &
CRYPTO_TFM_RES_MASK);
return rc;
diff --git a/drivers/crypto/amcc/crypto4xx_core.c b/drivers/crypto/amcc/crypto4xx_core.c
index 920bd5e720b2..3e7d24ff3fa6 100644
--- a/drivers/crypto/amcc/crypto4xx_core.c
+++ b/drivers/crypto/amcc/crypto4xx_core.c
@@ -965,15 +965,10 @@ static int crypto4xx_sk_init(struct crypto_skcipher *sk)
if (alg->base.cra_flags & CRYPTO_ALG_NEED_FALLBACK) {
ctx->sw_cipher.cipher =
- crypto_alloc_skcipher(alg->base.cra_name, 0,
- CRYPTO_ALG_NEED_FALLBACK |
- CRYPTO_ALG_ASYNC);
+ crypto_alloc_sync_skcipher(alg->base.cra_name, 0,
+ CRYPTO_ALG_NEED_FALLBACK);
if (IS_ERR(ctx->sw_cipher.cipher))
return PTR_ERR(ctx->sw_cipher.cipher);
-
- crypto_skcipher_set_reqsize(sk,
- sizeof(struct skcipher_request) + 32 +
- crypto_skcipher_reqsize(ctx->sw_cipher.cipher));
}
amcc_alg = container_of(alg, struct crypto4xx_alg, alg.u.cipher);
@@ -992,7 +987,7 @@ static void crypto4xx_sk_exit(struct crypto_skcipher *sk)
crypto4xx_common_exit(ctx);
if (ctx->sw_cipher.cipher)
- crypto_free_skcipher(ctx->sw_cipher.cipher);
+ crypto_free_sync_skcipher(ctx->sw_cipher.cipher);
}
static int crypto4xx_aead_init(struct crypto_aead *tfm)
diff --git a/drivers/crypto/amcc/crypto4xx_core.h b/drivers/crypto/amcc/crypto4xx_core.h
index 18df695ca6b1..4ecc34fa8ebd 100644
--- a/drivers/crypto/amcc/crypto4xx_core.h
+++ b/drivers/crypto/amcc/crypto4xx_core.h
@@ -131,7 +131,7 @@ struct crypto4xx_ctx {
__le32 iv_nonce;
u32 sa_len;
union {
- struct crypto_skcipher *cipher;
+ struct crypto_sync_skcipher *cipher;
struct crypto_aead *aead;
} sw_cipher;
};
--
2.20.1
Currently, crypto4xx CFB and OFB AES ciphers are
failing testmgr's test vectors.
|cfb-aes-ppc4xx encryption overran dst buffer on test vector 3, cfg="in-place"
|ofb-aes-ppc4xx encryption overran dst buffer on test vector 1, cfg="in-place"
This is because of a very subtile "bug" in the hardware that
gets indirectly mentioned in 18.1.3.5 Encryption/Decryption
of the hardware spec:
the OFB and CFB modes for AES are listed there as operation
modes for >>> "Block ciphers" <<<. Which kind of makes sense,
but we would like them to be considered as stream ciphers just
like the CTR mode.
To workaround this issue and stop the hardware from causing
"overran dst buffer" on crypttexts that are not a multiple
of 16 (AES_BLOCK_SIZE), we force the driver to use the scatter
buffers as the go-between.
As a bonus this patch also kills redundant pd_uinfo->num_gd
and pd_uinfo->num_sd setters since the value has already been
set before.
Cc: [email protected]
Signed-off-by: Christian Lamparter <[email protected]>
---
drivers/crypto/amcc/crypto4xx_core.c | 31 +++++++++++++++++++---------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/drivers/crypto/amcc/crypto4xx_core.c b/drivers/crypto/amcc/crypto4xx_core.c
index 06574a884715..920bd5e720b2 100644
--- a/drivers/crypto/amcc/crypto4xx_core.c
+++ b/drivers/crypto/amcc/crypto4xx_core.c
@@ -714,7 +714,23 @@ int crypto4xx_build_pd(struct crypto_async_request *req,
size_t offset_to_sr_ptr;
u32 gd_idx = 0;
int tmp;
- bool is_busy;
+ bool is_busy, force_sd;
+
+ /*
+ * There's a very subtile/disguised "bug" in the hardware that
+ * gets indirectly mentioned in 18.1.3.5 Encryption/Decryption
+ * of the hardware spec:
+ * *drum roll* the AES/(T)DES OFB and CFB modes are listed as
+ * operation modes for >>> "Block ciphers" <<<.
+ *
+ * To workaround this issue and stop the hardware from causing
+ * "overran dst buffer" on crypttexts that are not a multiple
+ * of 16 (AES_BLOCK_SIZE), we force the driver to use the
+ * scatter buffers.
+ */
+ force_sd = (req_sa->sa_command_1.bf.crypto_mode9_8 == CRYPTO_MODE_CFB
+ || req_sa->sa_command_1.bf.crypto_mode9_8 == CRYPTO_MODE_OFB)
+ && (datalen % AES_BLOCK_SIZE);
/* figure how many gd are needed */
tmp = sg_nents_for_len(src, assoclen + datalen);
@@ -732,7 +748,7 @@ int crypto4xx_build_pd(struct crypto_async_request *req,
}
/* figure how many sd are needed */
- if (sg_is_last(dst)) {
+ if (sg_is_last(dst) && force_sd == false) {
num_sd = 0;
} else {
if (datalen > PPC4XX_SD_BUFFER_SIZE) {
@@ -807,9 +823,10 @@ int crypto4xx_build_pd(struct crypto_async_request *req,
pd->sa_len = sa_len;
pd_uinfo = &dev->pdr_uinfo[pd_entry];
- pd_uinfo->async_req = req;
pd_uinfo->num_gd = num_gd;
pd_uinfo->num_sd = num_sd;
+ pd_uinfo->dest_va = dst;
+ pd_uinfo->async_req = req;
if (iv_len)
memcpy(pd_uinfo->sr_va->save_iv, iv, iv_len);
@@ -828,7 +845,6 @@ int crypto4xx_build_pd(struct crypto_async_request *req,
/* get first gd we are going to use */
gd_idx = fst_gd;
pd_uinfo->first_gd = fst_gd;
- pd_uinfo->num_gd = num_gd;
gd = crypto4xx_get_gdp(dev, &gd_dma, gd_idx);
pd->src = gd_dma;
/* enable gather */
@@ -865,17 +881,14 @@ int crypto4xx_build_pd(struct crypto_async_request *req,
* Indicate gather array is not used
*/
pd_uinfo->first_gd = 0xffffffff;
- pd_uinfo->num_gd = 0;
}
- if (sg_is_last(dst)) {
+ if (!num_sd) {
/*
* we know application give us dst a whole piece of memory
* no need to use scatter ring.
*/
pd_uinfo->using_sd = 0;
pd_uinfo->first_sd = 0xffffffff;
- pd_uinfo->num_sd = 0;
- pd_uinfo->dest_va = dst;
sa->sa_command_0.bf.scatter = 0;
pd->dest = (u32)dma_map_page(dev->core_dev->device,
sg_page(dst), dst->offset,
@@ -889,9 +902,7 @@ int crypto4xx_build_pd(struct crypto_async_request *req,
nbytes = datalen;
sa->sa_command_0.bf.scatter = 1;
pd_uinfo->using_sd = 1;
- pd_uinfo->dest_va = dst;
pd_uinfo->first_sd = fst_sd;
- pd_uinfo->num_sd = num_sd;
sd = crypto4xx_get_sdp(dev, &sd_dma, sd_idx);
pd->dest = sd_dma;
/* setup scatter descriptor */
--
2.20.1
On 4/22/19, Sasha Levin <[email protected]> wrote:
> Hi,
>
> [This is an automated email]
>
> This commit has been processed because it contains a -stable tag.
> The stable tag indicates that it's relevant for the following trees: all
>
> The bot has tested the following trees: v5.0.9, v4.19.36, v4.14.113,
> v4.9.170, v4.4.178, v3.18.138.
>
> v5.0.9: Build OK!
> v4.19.36: Build OK!
> v4.14.113: Failed to apply! Possible dependencies:
> fc340115ffb8 ("crypto: crypto4xx - properly set IV after de- and
> encrypt")
>
> v4.9.170: Failed to apply! Possible dependencies:
> fc340115ffb8 ("crypto: crypto4xx - properly set IV after de- and
> encrypt")
>
> v4.4.178: Failed to apply! Possible dependencies:
> fc340115ffb8 ("crypto: crypto4xx - properly set IV after de- and
> encrypt")
>
> v3.18.138: Failed to apply! Possible dependencies:
> fc340115ffb8 ("crypto: crypto4xx - properly set IV after de- and
> encrypt")
>
>
> How should we proceed with this patch?
I only added AES-CTR support around a year ago.
https://kernel.googlesource.com/pub/scm/linux/kernel/git/stable/linux-stable/+/98e87e3d933b8e504ea41b8857c038d2cd06cddc
(so, this is another dependency)
In my opinion back-porting the patch to 4.14 and older would not do
much since the mode is not available there. But, I'm glad that the
patches got a bit of attention.
Cheers,
Christian
On 4/22/19, Sasha Levin <[email protected]> wrote:
> Hi,
>
> [This is an automated email]
>
> This commit has been processed because it contains a -stable tag.
> The stable tag indicates that it's relevant for the following trees: all
>
> The bot has tested the following trees: v5.0.9, v4.19.36, v4.14.113,
> v4.9.170, v4.4.178, v3.18.138.
>
> v5.0.9: Build OK!
> v4.19.36: Build OK!
> v4.14.113: Failed to apply! Possible dependencies:
[...]
> f2a13e7cba9e ("crypto: crypto4xx - enable AES RFC3686, ECB, CFB and OFB
> offloads")
>
>
> How should we proceed with this patch?
>
Same as with the aes-ctr fix. I guess, I should have added
Fixes: f2a13e7cba9e ("crypto: crypto4xx - enable AES RFC3686, ECB, CFB
and OFB offloads")
Regards,
Christian
On Mon, Apr 22, 2019 at 01:25:58PM +0200, Christian Lamparter wrote:
> Commit 8efd972ef96a ("crypto: testmgr - support checking skcipher output IV")
> caused the crypto4xx driver to produce the following error:
>
> | ctr-aes-ppc4xx encryption test failed (wrong output IV)
> | on test vector 0, cfg="in-place"
>
> This patch fixes this by reworking the crypto4xx_setkey_aes()
> function to:
>
> - not save the iv for ECB (as per 18.2.38 CRYP0_SA_CMD_0:
> "This bit mut be cleared for DES ECB mode or AES ECB mode,
> when no IV is used.")
>
> - instruct the hardware to save the generated IV for all
> other modes of operations that have IV and then supply
> it back to the callee in pretty much the same way as we
> do it for cbc-aes already.
>
> - make it clear that the DIR_(IN|OUT)BOUND is the important
> bit that tells the hardware to encrypt or decrypt the data.
> (this is cosmetic - but it hopefully prevents me from
> getting confused again).
>
> - don't load any bogus hash when we don't use any hash
> operation to begin with.
>
> Cc: [email protected]
> Signed-off-by: Christian Lamparter <[email protected]>
> ---
> drivers/crypto/amcc/crypto4xx_alg.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
All 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