From: Gary R Hook <gary.hook@amd.com>
To: "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
"herbert@gondor.apana.org.au" <herbert@gondor.apana.org.au>
Cc: "davem@davemloft.net" <davem@davemloft.net>
Subject: Re: [PATCH] crypto: ccp - Fix XTS-AES support on a version 5 CCP
Date: Fri, 18 Aug 2017 11:19:37 -0500 [thread overview]
Message-ID: <f5714d71-4e4a-b942-7e1e-58f478a5ff21@amd.com> (raw)
In-Reply-To: <14a02623-b19e-b4ae-c078-57912a83cd4b@amd.com>
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 <gary.hook@amd.com>
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..?
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 <thomas.lendacky@amd.com>
>> *
>> @@ -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;
>> }
>>
next prev parent reply other threads:[~2017-08-18 16:19 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-17 20:08 [PATCH] crypto: ccp - Fix XTS-AES support on a version 5 CCP Gary R Hook
2017-07-17 21:48 ` Tom Lendacky
2017-07-21 15:48 ` Gary R Hook
2017-08-18 16:19 ` Gary R Hook [this message]
2017-08-18 16:38 ` Gary R Hook
2017-08-18 16:41 ` [PATCH] crypto: ccp - Fix XTS-AES-128 support on v5 CCPs Gary R Hook
2017-08-19 4:02 ` Herbert Xu
2017-08-22 18:24 ` Gary R Hook
2017-07-18 6:28 ` [PATCH] crypto: ccp - Fix XTS-AES support on a version 5 CCP Stephan Müller
2017-07-18 17:39 ` Gary R Hook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f5714d71-4e4a-b942-7e1e-58f478a5ff21@amd.com \
--to=gary.hook@amd.com \
--cc=davem@davemloft.net \
--cc=herbert@gondor.apana.org.au \
--cc=linux-crypto@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox