Re: [PATCH] crypto: ccp - Fix XTS-AES support on a version 5 CCP

[Gary R Hook <gary.hook@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>


Look for a version 2

>> ---
>>   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.

Given the limitations of the CCP (w.r.t. XTS-AES) I thought it more 
clear to look
for only those unit-sizes that are supported, and to use the enumerated 
value
as the index.

> Also, re-arranging the order should be a separate patch if that doesn't
> really fix anything.

Yes, agreed.

>>
>> @@ -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.

Yes.

>
>>        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.

Yes, the structure member needs to be made larger.

>
>>        }
>>        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.

My version two approach will address this.

>
>> -             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.

The changes for a version 5 device require additional checks. Also the 
next patch
version will correct/clarify the logic by which I arrive at the fallback 
choice.