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: stable@vger.kernel.org Signed-off-by: Christian Lamparter chunkeey@gmail.com --- 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; }
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: stable@vger.kernel.org Signed-off-by: Christian Lamparter chunkeey@gmail.com --- 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 */
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: stable@vger.kernel.org Signed-off-by: Christian Lamparter chunkeey@gmail.com
drivers/crypto/amcc/crypto4xx_alg.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
All applied. Thanks.
linux-stable-mirror@lists.linaro.org
-
Christian Lamparter -
Herbert Xu