From: Gary R Hook Subject: Re: [PATCH] crypto: ccp - Fix XTS-AES support on a version 5 CCP Date: Fri, 18 Aug 2017 11:38:09 -0500 Message-ID: References: <150032210775.102848.1585950131204303137.stgit@sosxen.amd.com> <14a02623-b19e-b4ae-c078-57912a83cd4b@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: "davem@davemloft.net" To: "linux-crypto@vger.kernel.org" , "herbert@gondor.apana.org.au" Return-path: Received: from mail-dm3nam03on0062.outbound.protection.outlook.com ([104.47.41.62]:51474 "EHLO NAM03-DM3-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752563AbdHRQiO (ORCPT ); Fri, 18 Aug 2017 12:38:14 -0400 In-Reply-To: Sender: linux-crypto-owner@vger.kernel.org List-ID: On 08/18/2017 11:19 AM, Gary R Hook wrote: > On 07/17/2017 04:48 PM, Lendacky, Thomas wrote: >> On 7/17/2017 3:08 PM, Gary R Hook wrote: >>> Version 5 CCPs have differing requirements for XTS-AES: key components >>> are stored in a 512-bit vector. The context must be little-endian >>> justified. AES-256 is supported now, so propagate the cipher size to >>> the command descriptor. >>> >>> Signed-off-by: Gary R Hook > > Herbert, > > I see that this patch (and others that add function or fix bugs) has > been added to > cryptodev. I've not seen anything CCP-related pushed to Linux in a > while, however. > > Is there something more I need to do to for these recent patches, to get > them promoted > to the mainline kernel..? Please ignore. I realized (too late) that the referenced items missed the merge window for 4.13. > > Thanks much! > Gary > > >>> --- >>> drivers/crypto/ccp/ccp-crypto-aes-xts.c | 79 ++++++++++++++++--------------- >>> drivers/crypto/ccp/ccp-dev-v5.c | 2 + >>> drivers/crypto/ccp/ccp-dev.h | 2 + >>> drivers/crypto/ccp/ccp-ops.c | 56 ++++++++++++++++++---- >>> 4 files changed, 89 insertions(+), 50 deletions(-) >>> >>> diff --git a/drivers/crypto/ccp/ccp-crypto-aes-xts.c b/drivers/crypto/ccp/ccp-crypto-aes-xts.c >>> index 58a4244b4752..8d248b198e22 100644 >>> --- a/drivers/crypto/ccp/ccp-crypto-aes-xts.c >>> +++ b/drivers/crypto/ccp/ccp-crypto-aes-xts.c >>> @@ -1,7 +1,7 @@ >>> /* >>> * AMD Cryptographic Coprocessor (CCP) AES XTS crypto API support >>> * >>> - * Copyright (C) 2013 Advanced Micro Devices, Inc. >>> + * Copyright (C) 2013,2017 Advanced Micro Devices, Inc. >>> * >>> * Author: Tom Lendacky >>> * >>> @@ -37,46 +37,26 @@ struct ccp_unit_size_map { >>> u32 value; >>> }; >>> >>> -static struct ccp_unit_size_map unit_size_map[] = { >>> +static struct ccp_unit_size_map xts_unit_sizes[] = { >>> { >>> - .size = 4096, >>> - .value = CCP_XTS_AES_UNIT_SIZE_4096, >>> - }, >>> - { >>> - .size = 2048, >>> - .value = CCP_XTS_AES_UNIT_SIZE_2048, >>> - }, >>> - { >>> - .size = 1024, >>> - .value = CCP_XTS_AES_UNIT_SIZE_1024, >>> + .size = 16, >>> + .value = CCP_XTS_AES_UNIT_SIZE_16, >>> }, >>> { >>> - .size = 512, >>> + .size = 512, >>> .value = CCP_XTS_AES_UNIT_SIZE_512, >>> }, >>> { >>> - .size = 256, >>> - .value = CCP_XTS_AES_UNIT_SIZE__LAST, >>> - }, >>> - { >>> - .size = 128, >>> - .value = CCP_XTS_AES_UNIT_SIZE__LAST, >>> - }, >>> - { >>> - .size = 64, >>> - .value = CCP_XTS_AES_UNIT_SIZE__LAST, >>> - }, >>> - { >>> - .size = 32, >>> - .value = CCP_XTS_AES_UNIT_SIZE__LAST, >>> + .size = 1024, >>> + .value = CCP_XTS_AES_UNIT_SIZE_1024, >>> }, >>> { >>> - .size = 16, >>> - .value = CCP_XTS_AES_UNIT_SIZE_16, >>> + .size = 2048, >>> + .value = CCP_XTS_AES_UNIT_SIZE_2048, >>> }, >>> { >>> - .size = 1, >>> - .value = CCP_XTS_AES_UNIT_SIZE__LAST, >>> + .size = 4096, >>> + .value = CCP_XTS_AES_UNIT_SIZE_4096, >>> }, >>> }; >> >> Because of the way the unit size check is performed, you can't delete >> the intermediate size checks. Those must remain so that unit sizes >> that aren't supported by the CCP are sent to the fallback mechanism. >> >> Also, re-arranging the order should be a separate patch if that doesn't >> really fix anything. >> >>> >>> @@ -97,14 +77,20 @@ static int ccp_aes_xts_setkey(struct crypto_ablkcipher *tfm, const u8 *key, >>> unsigned int key_len) >>> { >>> struct ccp_ctx *ctx = crypto_tfm_ctx(crypto_ablkcipher_tfm(tfm)); >>> + unsigned int ccpversion = ccp_version(); >>> >>> /* Only support 128-bit AES key with a 128-bit Tweak key, >>> * otherwise use the fallback >>> */ >>> + >> >> Remove the addition of the blank line and update the above comment to >> indicate the new supported key size added below. >> >>> switch (key_len) { >>> case AES_KEYSIZE_128 * 2: >>> memcpy(ctx->u.aes.key, key, key_len); >>> break; >>> + case AES_KEYSIZE_256 * 2: >>> + if (ccpversion > CCP_VERSION(3, 0)) >>> + memcpy(ctx->u.aes.key, key, key_len); >>> + break; >> >> Isn't u.aes.key defined with a maximum buffer size of AES_MAX_KEY_SIZE >> (which is 32)? I think this will cause a buffer overrun on memcpy. >> >>> } >>> ctx->u.aes.key_len = key_len / 2; >>> sg_init_one(&ctx->u.aes.key_sg, ctx->u.aes.key, key_len); >>> @@ -117,7 +103,10 @@ static int ccp_aes_xts_crypt(struct ablkcipher_request *req, >>> { >>> struct ccp_ctx *ctx = crypto_tfm_ctx(req->base.tfm); >>> struct ccp_aes_req_ctx *rctx = ablkcipher_request_ctx(req); >>> + unsigned int ccpversion = ccp_version(); >>> + unsigned int fallback = 0; >>> unsigned int unit; >>> + u32 block_size = 0; >>> u32 unit_size; >>> int ret; >>> >>> @@ -131,17 +120,29 @@ static int ccp_aes_xts_crypt(struct ablkcipher_request *req, >>> return -EINVAL; >>> >>> unit_size = CCP_XTS_AES_UNIT_SIZE__LAST; >>> - if (req->nbytes <= unit_size_map[0].size) { >> >> This check can't be deleted. It was added specifically to catch cases >> where the size was greater than 4096. >> >>> - for (unit = 0; unit < ARRAY_SIZE(unit_size_map); unit++) { >>> - if (!(req->nbytes & (unit_size_map[unit].size - 1))) { >>> - unit_size = unit_size_map[unit].value; >>> - break; >>> - } >>> + for (unit = ARRAY_SIZE(xts_unit_sizes); unit-- > 0; ) { >>> + if (!(req->nbytes & (xts_unit_sizes[unit].size - 1))) { >>> + unit_size = unit; >>> + block_size = xts_unit_sizes[unit].size; >>> + break; >>> } >>> } >>> >>> - if ((unit_size == CCP_XTS_AES_UNIT_SIZE__LAST) || >>> - (ctx->u.aes.key_len != AES_KEYSIZE_128)) { >>> + /* The CCP has restrictions on block sizes. Also, a version 3 device >>> + * only supports AES-128 operations; version 5 CCPs support both >>> + * AES-128 and -256 operations. >>> + */ >>> + if (unit_size == CCP_XTS_AES_UNIT_SIZE__LAST) >>> + fallback = 1; >>> + if ((ccpversion < CCP_VERSION(5, 0)) && >>> + (ctx->u.aes.key_len != AES_KEYSIZE_128)) >>> + fallback = 1; >>> + if ((ctx->u.aes.key_len != AES_KEYSIZE_128) && >>> + (ctx->u.aes.key_len != AES_KEYSIZE_256)) >>> + fallback = 1; >>> + if (req->nbytes != block_size) >>> + fallback = 1; >> >> I don't believe this is correct. Everything should fall out properly >> for fallback based on the unit size and key size checks above. >> >> Thanks, >> Tom >> >>> + if (fallback) { >>> SKCIPHER_REQUEST_ON_STACK(subreq, ctx->u.aes.tfm_skcipher); >>> >>> /* Use the fallback to process the request for any >>> diff --git a/drivers/crypto/ccp/ccp-dev-v5.c b/drivers/crypto/ccp/ccp-dev-v5.c >>> index b3526336d608..11febe2bd07c 100644 >>> --- a/drivers/crypto/ccp/ccp-dev-v5.c >>> +++ b/drivers/crypto/ccp/ccp-dev-v5.c >>> @@ -144,6 +144,7 @@ union ccp_function { >>> #define CCP_AES_ENCRYPT(p) ((p)->aes.encrypt) >>> #define CCP_AES_MODE(p) ((p)->aes.mode) >>> #define CCP_AES_TYPE(p) ((p)->aes.type) >>> +#define CCP_XTS_TYPE(p) ((p)->aes_xts.type) >>> #define CCP_XTS_SIZE(p) ((p)->aes_xts.size) >>> #define CCP_XTS_ENCRYPT(p) ((p)->aes_xts.encrypt) >>> #define CCP_DES3_SIZE(p) ((p)->des3.size) >>> @@ -344,6 +345,7 @@ static int ccp5_perform_xts_aes(struct ccp_op *op) >>> CCP5_CMD_PROT(&desc) = 0; >>> >>> function.raw = 0; >>> + CCP_XTS_TYPE(&function) = op->u.xts.type; >>> CCP_XTS_ENCRYPT(&function) = op->u.xts.action; >>> CCP_XTS_SIZE(&function) = op->u.xts.unit_size; >>> CCP5_CMD_FUNCTION(&desc) = function.raw; >>> diff --git a/drivers/crypto/ccp/ccp-dev.h b/drivers/crypto/ccp/ccp-dev.h >>> index 9320931d89da..3d51180199ac 100644 >>> --- a/drivers/crypto/ccp/ccp-dev.h >>> +++ b/drivers/crypto/ccp/ccp-dev.h >>> @@ -194,6 +194,7 @@ >>> #define CCP_AES_CTX_SB_COUNT 1 >>> >>> #define CCP_XTS_AES_KEY_SB_COUNT 1 >>> +#define CCP5_XTS_AES_KEY_SB_COUNT 2 >>> #define CCP_XTS_AES_CTX_SB_COUNT 1 >>> >>> #define CCP_DES3_KEY_SB_COUNT 1 >>> @@ -497,6 +498,7 @@ struct ccp_aes_op { >>> }; >>> >>> struct ccp_xts_aes_op { >>> + enum ccp_aes_type type; >>> enum ccp_aes_action action; >>> enum ccp_xts_aes_unit_size unit_size; >>> }; >>> diff --git a/drivers/crypto/ccp/ccp-ops.c b/drivers/crypto/ccp/ccp-ops.c >>> index e23d138fc1ce..d1be07884a9b 100644 >>> --- a/drivers/crypto/ccp/ccp-ops.c >>> +++ b/drivers/crypto/ccp/ccp-ops.c >>> @@ -1038,6 +1038,8 @@ static int ccp_run_xts_aes_cmd(struct ccp_cmd_queue *cmd_q, >>> struct ccp_op op; >>> unsigned int unit_size, dm_offset; >>> bool in_place = false; >>> + unsigned int sb_count = 0; >>> + enum ccp_aes_type aestype; >>> int ret; >>> >>> switch (xts->unit_size) { >>> @@ -1056,12 +1058,15 @@ static int ccp_run_xts_aes_cmd(struct ccp_cmd_queue *cmd_q, >>> case CCP_XTS_AES_UNIT_SIZE_4096: >>> unit_size = 4096; >>> break; >>> - >>> default: >>> return -EINVAL; >>> } >>> >>> - if (xts->key_len != AES_KEYSIZE_128) >>> + if (xts->key_len == AES_KEYSIZE_128) >>> + aestype = CCP_AES_TYPE_128; >>> + else if (xts->key_len == AES_KEYSIZE_256) >>> + aestype = CCP_AES_TYPE_256; >>> + else >>> return -EINVAL; >>> >>> if (!xts->final && (xts->src_len & (AES_BLOCK_SIZE - 1))) >>> @@ -1084,22 +1089,50 @@ static int ccp_run_xts_aes_cmd(struct ccp_cmd_queue *cmd_q, >>> op.sb_ctx = cmd_q->sb_ctx; >>> op.init = 1; >>> op.u.xts.action = xts->action; >>> + op.u.xts.type = aestype; >>> op.u.xts.unit_size = xts->unit_size; >>> >>> - /* All supported key sizes fit in a single (32-byte) SB entry >>> - * and must be in little endian format. Use the 256-bit byte >>> - * swap passthru option to convert from big endian to little >>> - * endian. >>> + /* A version 3 device only supports 128-bit keys, which fits into a >>> + * single SB entry. A version 5 device uses a 512-bit vector, so two >>> + * SB entries. >>> */ >>> + if (cmd_q->ccp->vdata->version == CCP_VERSION(3, 0)) >>> + sb_count = CCP_XTS_AES_KEY_SB_COUNT; >>> + else >>> + sb_count = CCP5_XTS_AES_KEY_SB_COUNT; >>> ret = ccp_init_dm_workarea(&key, cmd_q, >>> - CCP_XTS_AES_KEY_SB_COUNT * CCP_SB_BYTES, >>> + sb_count * CCP_SB_BYTES, >>> DMA_TO_DEVICE); >>> if (ret) >>> return ret; >>> >>> - dm_offset = CCP_SB_BYTES - AES_KEYSIZE_128; >>> - ccp_set_dm_area(&key, dm_offset, xts->key, 0, xts->key_len); >>> - ccp_set_dm_area(&key, 0, xts->key, dm_offset, xts->key_len); >>> + if (cmd_q->ccp->vdata->version == CCP_VERSION(3, 0)) { >>> + /* All supported key sizes * must be in little endian format. >>> + * Use the 256-bit byte swap passthru option to convert from >>> + * big endian to little endian. >>> + */ >>> + dm_offset = CCP_SB_BYTES - AES_KEYSIZE_128; >>> + ccp_set_dm_area(&key, dm_offset, xts->key, 0, xts->key_len); >>> + ccp_set_dm_area(&key, 0, xts->key, xts->key_len, xts->key_len); >>> + } else { >>> + /* The AES key is at the little end and the tweak key is >>> + * at the big end. Since the byteswap operation is only >>> + * 256-bit, load the buffer according to the way things >>> + * will end up. >>> + */ >>> + unsigned int pad; >>> + >>> + op.sb_key = cmd_q->ccp->vdata->perform->sballoc(cmd_q, >>> + sb_count); >>> + if (!op.sb_key) >>> + return -EIO; >>> + >>> + dm_offset = CCP_SB_BYTES; >>> + pad = dm_offset - xts->key_len; >>> + ccp_set_dm_area(&key, pad, xts->key, 0, xts->key_len); >>> + ccp_set_dm_area(&key, dm_offset + pad, xts->key, xts->key_len, >>> + xts->key_len); >>> + } >>> ret = ccp_copy_to_sb(cmd_q, &key, op.jobid, op.sb_key, >>> CCP_PASSTHRU_BYTESWAP_256BIT); >>> if (ret) { >>> @@ -1179,7 +1212,6 @@ static int ccp_run_xts_aes_cmd(struct ccp_cmd_queue *cmd_q, >>> e_dst: >>> if (!in_place) >>> ccp_free_data(&dst, cmd_q); >>> - >>> e_src: >>> ccp_free_data(&src, cmd_q); >>> >>> @@ -1188,6 +1220,8 @@ static int ccp_run_xts_aes_cmd(struct ccp_cmd_queue *cmd_q, >>> >>> e_key: >>> ccp_dm_free(&key); >>> + if (cmd_q->ccp->vdata->version > CCP_VERSION(3, 0)) >>> + cmd_q->ccp->vdata->perform->sbfree(cmd_q, op.sb_key, sb_count); >>> >>> return ret; >>> } >>>