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

[Gary R Hook <gary.hook@amd.com>]

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

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