Hi Richard,
Let's trade reviews to get the ball rolling?
On 14.06.21 22:16, Richard Weinberger wrote:
> DCP is capable to performing AES with hardware-bound keys.
> These keys are not stored in main memory and are therefore not directly
> accessible by the operating system.
>
> So instead of feeding the key into DCP, we need to place a
> reference to such a key before initiating the crypto operation.
> Keys are referenced by a one byte identifiers.
>
> DCP supports 6 different keys: 4 slots in the secure memory area,
> a one time programmable key which can be burnt via on-chip fuses
> and an unique device key.
>
> Using these keys is restricted to in-kernel users that use them as building
> block for other crypto tools such as trusted keys. Allowing userspace
> (e.g. via AF_ALG) to use these keys to crypt or decrypt data is a security
> risk, because there is no access control mechanism.
>
> Cc: Ahmad Fatoum <[email protected]>
> Cc: David Gstir <[email protected]>
> Cc: David Howells <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: Fabio Estevam <[email protected]>
> Cc: Herbert Xu <[email protected]>
> Cc: James Bottomley <[email protected]>
> Cc: James Morris <[email protected]>
> Cc: Jarkko Sakkinen <[email protected]>
> Cc: Jonathan Corbet <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: Mimi Zohar <[email protected]>
> Cc: NXP Linux Team <[email protected]>
> Cc: Pengutronix Kernel Team <[email protected]>
> Cc: Richard Weinberger <[email protected]>
> Cc: Sascha Hauer <[email protected]>
> Cc: "Serge E. Hallyn" <[email protected]>
> Cc: Shawn Guo <[email protected]>
> Co-developed-by: David Gstir <[email protected]>
> Signed-off-by: David Gstir <[email protected]>
> Signed-off-by: Richard Weinberger <[email protected]>
> ---
> drivers/crypto/mxs-dcp.c | 110 ++++++++++++++++++++++++++++++++++-----
> include/linux/mxs-dcp.h | 19 +++++++
> 2 files changed, 117 insertions(+), 12 deletions(-)
> create mode 100644 include/linux/mxs-dcp.h
>
> diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c
> index d6a7784d2988..c3e0c0ccbc20 100644
> --- a/drivers/crypto/mxs-dcp.c
> +++ b/drivers/crypto/mxs-dcp.c
> @@ -15,6 +15,7 @@
> #include <linux/platform_device.h>
> #include <linux/stmp_device.h>
> #include <linux/clk.h>
> +#include <linux/mxs-dcp.h>
The CAAM specific headers are in <soc/fsl/*.h>.
Should this be done likewise here as well?
>
> #include <crypto/aes.h>
> #include <crypto/sha1.h>
> @@ -101,6 +102,7 @@ struct dcp_async_ctx {
> struct crypto_skcipher *fallback;
> unsigned int key_len;
> uint8_t key[AES_KEYSIZE_128];
> + bool refkey;
> };
>
> struct dcp_aes_req_ctx {
> @@ -155,6 +157,7 @@ static struct dcp *global_sdcp;
> #define MXS_DCP_CONTROL0_HASH_TERM (1 << 13)
> #define MXS_DCP_CONTROL0_HASH_INIT (1 << 12)
> #define MXS_DCP_CONTROL0_PAYLOAD_KEY (1 << 11)
> +#define MXS_DCP_CONTROL0_OTP_KEY (1 << 10)
> #define MXS_DCP_CONTROL0_CIPHER_ENCRYPT (1 << 8)
> #define MXS_DCP_CONTROL0_CIPHER_INIT (1 << 9)
> #define MXS_DCP_CONTROL0_ENABLE_HASH (1 << 6)
> @@ -168,6 +171,8 @@ static struct dcp *global_sdcp;
> #define MXS_DCP_CONTROL1_CIPHER_MODE_ECB (0 << 4)
> #define MXS_DCP_CONTROL1_CIPHER_SELECT_AES128 (0 << 0)
>
> +#define MXS_DCP_CONTROL1_KEY_SELECT_SHIFT 8
> +
> static int mxs_dcp_start_dma(struct dcp_async_ctx *actx)
> {
> struct dcp *sdcp = global_sdcp;
> @@ -219,15 +224,18 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
> struct dcp *sdcp = global_sdcp;
> struct dcp_dma_desc *desc = &sdcp->coh->desc[actx->chan];
> struct dcp_aes_req_ctx *rctx = skcipher_request_ctx(req);
> + dma_addr_t src_phys, dst_phys, key_phys = {0};
Why = {0}; ? dma_addr_t is a scalar type and the value is always
written here before access.
> + bool key_referenced = actx->refkey;
> int ret;
>
> - dma_addr_t key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key,
> - 2 * AES_KEYSIZE_128,
> - DMA_TO_DEVICE);
> - dma_addr_t src_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_in_buf,
> - DCP_BUF_SZ, DMA_TO_DEVICE);
> - dma_addr_t dst_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_out_buf,
> - DCP_BUF_SZ, DMA_FROM_DEVICE);
> + if (!key_referenced) {
> + key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key,
> + 2 * AES_KEYSIZE_128, DMA_TO_DEVICE);
> + }
> + src_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_in_buf, DCP_BUF_SZ,
> + DMA_TO_DEVICE);
> + dst_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_out_buf, DCP_BUF_SZ,
> + DMA_FROM_DEVICE);
>
> if (actx->fill % AES_BLOCK_SIZE) {
> dev_err(sdcp->dev, "Invalid block size!\n");
> @@ -240,8 +248,13 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
> MXS_DCP_CONTROL0_INTERRUPT |
> MXS_DCP_CONTROL0_ENABLE_CIPHER;
>
> - /* Payload contains the key. */
> - desc->control0 |= MXS_DCP_CONTROL0_PAYLOAD_KEY;
> + if (key_referenced) {
> + /* Set OTP key bit to select the key via KEY_SELECT. */
> + desc->control0 |= MXS_DCP_CONTROL0_OTP_KEY;
> + } else {
> + /* Payload contains the key. */
> + desc->control0 |= MXS_DCP_CONTROL0_PAYLOAD_KEY;
> + }
>
> if (rctx->enc)
> desc->control0 |= MXS_DCP_CONTROL0_CIPHER_ENCRYPT;
> @@ -255,6 +268,9 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
> else
> desc->control1 |= MXS_DCP_CONTROL1_CIPHER_MODE_CBC;
>
> + if (key_referenced)
> + desc->control1 |= sdcp->coh->aes_key[0] << MXS_DCP_CONTROL1_KEY_SELECT_SHIFT;
> +
> desc->next_cmd_addr = 0;
> desc->source = src_phys;
> desc->destination = dst_phys;
> @@ -265,8 +281,10 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
> ret = mxs_dcp_start_dma(actx);
>
> aes_done_run:
> - dma_unmap_single(sdcp->dev, key_phys, 2 * AES_KEYSIZE_128,
> - DMA_TO_DEVICE);
> + if (!key_referenced) {
> + dma_unmap_single(sdcp->dev, key_phys, 2 * AES_KEYSIZE_128,
> + DMA_TO_DEVICE);
> + }
> dma_unmap_single(sdcp->dev, src_phys, DCP_BUF_SZ, DMA_TO_DEVICE);
> dma_unmap_single(sdcp->dev, dst_phys, DCP_BUF_SZ, DMA_FROM_DEVICE);
>
> @@ -454,7 +472,7 @@ static int mxs_dcp_aes_enqueue(struct skcipher_request *req, int enc, int ecb)
> struct dcp_aes_req_ctx *rctx = skcipher_request_ctx(req);
> int ret;
>
> - if (unlikely(actx->key_len != AES_KEYSIZE_128))
> + if (unlikely(actx->key_len != AES_KEYSIZE_128 && !actx->refkey))
> return mxs_dcp_block_fallback(req, enc);
>
> rctx->enc = enc;
> @@ -501,6 +519,7 @@ static int mxs_dcp_aes_setkey(struct crypto_skcipher *tfm, const u8 *key,
> * there can still be an operation in progress.
> */
> actx->key_len = len;
> + actx->refkey = false;
> if (len == AES_KEYSIZE_128) {
> memcpy(actx->key, key, len);
> return 0;
> @@ -517,6 +536,33 @@ static int mxs_dcp_aes_setkey(struct crypto_skcipher *tfm, const u8 *key,
> return crypto_skcipher_setkey(actx->fallback, key, len);
> }
>
> +static int mxs_dcp_aes_setrefkey(struct crypto_skcipher *tfm, const u8 *key,
> + unsigned int len)
> +{
> + struct dcp_async_ctx *actx = crypto_skcipher_ctx(tfm);
> + int ret = -EINVAL;
> +
> + if (len != DCP_PAES_KEYSIZE)
> + goto out;
Nitpick: there is no cleanup, so why not return -EINVAL here
and unconditionally return 0 below?
> +
> + actx->key_len = len;
> + actx->refkey = true;
> +
> + switch (key[0]) {
> + case DCP_PAES_KEY_SLOT0:
> + case DCP_PAES_KEY_SLOT1:
> + case DCP_PAES_KEY_SLOT2:
> + case DCP_PAES_KEY_SLOT3:
> + case DCP_PAES_KEY_UNIQUE:
> + case DCP_PAES_KEY_OTP:
> + memcpy(actx->key, key, len);
> + ret = 0;
> + }
In the error case you return -EINVAL below, but you still write
into actx. Is that intentional?
> +
> +out:
> + return ret;
> +}
> +
> static int mxs_dcp_aes_fallback_init_tfm(struct crypto_skcipher *tfm)
> {
> const char *name = crypto_tfm_alg_name(crypto_skcipher_tfm(tfm));
> @@ -540,6 +586,13 @@ static void mxs_dcp_aes_fallback_exit_tfm(struct crypto_skcipher *tfm)
> crypto_free_skcipher(actx->fallback);
> }
>
> +static int mxs_dcp_paes_init_tfm(struct crypto_skcipher *tfm)
> +{
> + crypto_skcipher_set_reqsize(tfm, sizeof(struct dcp_aes_req_ctx));
> +
> + return 0;
> +}
> +
> /*
> * Hashing (SHA1/SHA256)
> */
> @@ -882,6 +935,39 @@ static struct skcipher_alg dcp_aes_algs[] = {
> .ivsize = AES_BLOCK_SIZE,
> .init = mxs_dcp_aes_fallback_init_tfm,
> .exit = mxs_dcp_aes_fallback_exit_tfm,
> + }, {
> + .base.cra_name = "ecb(paes)",
> + .base.cra_driver_name = "ecb-paes-dcp",
> + .base.cra_priority = 401,
> + .base.cra_alignmask = 15,
> + .base.cra_flags = CRYPTO_ALG_ASYNC | CRYPTO_ALG_INTERNAL,
> + .base.cra_blocksize = AES_BLOCK_SIZE,
> + .base.cra_ctxsize = sizeof(struct dcp_async_ctx),
> + .base.cra_module = THIS_MODULE,
> +
> + .min_keysize = DCP_PAES_KEYSIZE,
> + .max_keysize = DCP_PAES_KEYSIZE,
> + .setkey = mxs_dcp_aes_setrefkey,
> + .encrypt = mxs_dcp_aes_ecb_encrypt,
> + .decrypt = mxs_dcp_aes_ecb_decrypt,
> + .init = mxs_dcp_paes_init_tfm,
> + }, {
> + .base.cra_name = "cbc(paes)",
> + .base.cra_driver_name = "cbc-paes-dcp",
> + .base.cra_priority = 401,
> + .base.cra_alignmask = 15,
> + .base.cra_flags = CRYPTO_ALG_ASYNC | CRYPTO_ALG_INTERNAL,
> + .base.cra_blocksize = AES_BLOCK_SIZE,
> + .base.cra_ctxsize = sizeof(struct dcp_async_ctx),
> + .base.cra_module = THIS_MODULE,
> +
> + .min_keysize = DCP_PAES_KEYSIZE,
> + .max_keysize = DCP_PAES_KEYSIZE,
> + .setkey = mxs_dcp_aes_setrefkey,
> + .encrypt = mxs_dcp_aes_cbc_encrypt,
> + .decrypt = mxs_dcp_aes_cbc_decrypt,
> + .ivsize = AES_BLOCK_SIZE,
> + .init = mxs_dcp_paes_init_tfm,
> },
> };
>
> diff --git a/include/linux/mxs-dcp.h b/include/linux/mxs-dcp.h
> new file mode 100644
> index 000000000000..df6678ee10a1
> --- /dev/null
> +++ b/include/linux/mxs-dcp.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2021 sigma star gmbh
> + * Authors: David Gstir <[email protected]>
> + * Richard Weinberger <[email protected]>
> + */
> +
> +#ifndef MXS_DCP_H
> +#define MXS_DCP_H
> +
> +#define DCP_PAES_KEYSIZE 1
> +#define DCP_PAES_KEY_SLOT0 0x00
> +#define DCP_PAES_KEY_SLOT1 0x01
> +#define DCP_PAES_KEY_SLOT2 0x02
> +#define DCP_PAES_KEY_SLOT3 0x03
> +#define DCP_PAES_KEY_UNIQUE 0xfe
> +#define DCP_PAES_KEY_OTP 0xff
> +
> +#endif /* MXS_DCP_H */
Cheers,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Ahmad,
----- Ursprüngliche Mail -----
> Von: "Ahmad Fatoum" <[email protected]>
> Let's trade reviews to get the ball rolling?
Sounds like a fair deal. :-)
[...]
>> --- a/drivers/crypto/mxs-dcp.c
>> +++ b/drivers/crypto/mxs-dcp.c
>> @@ -15,6 +15,7 @@
>> #include <linux/platform_device.h>
>> #include <linux/stmp_device.h>
>> #include <linux/clk.h>
>> +#include <linux/mxs-dcp.h>
>
> The CAAM specific headers are in <soc/fsl/*.h>.
> Should this be done likewise here as well?
I have no preferences. If soc/fsl/ is the way to go, fine by me.
[...]
>> @@ -219,15 +224,18 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
>> struct dcp *sdcp = global_sdcp;
>> struct dcp_dma_desc *desc = &sdcp->coh->desc[actx->chan];
>> struct dcp_aes_req_ctx *rctx = skcipher_request_ctx(req);
>> + dma_addr_t src_phys, dst_phys, key_phys = {0};
>
> Why = {0}; ? dma_addr_t is a scalar type and the value is always
> written here before access.
Initializing a scalar with {} is allowed in C, the braces are optional.
I like the braces because it works even when the underlaying type changes.
But that's just a matter of taste.
key_phys is initialized because it triggered a false positive gcc warning
on one of my targets. Let me re-run again to be sure, the code saw a lot of
refactoring since that.
[...]
>> +static int mxs_dcp_aes_setrefkey(struct crypto_skcipher *tfm, const u8 *key,
>> + unsigned int len)
>> +{
>> + struct dcp_async_ctx *actx = crypto_skcipher_ctx(tfm);
>> + int ret = -EINVAL;
>> +
>> + if (len != DCP_PAES_KEYSIZE)
>> + goto out;
>
> Nitpick: there is no cleanup, so why not return -EINVAL here
> and unconditionally return 0 below?
What is the benefit?
Usually I try to use goto to have a single exit point of a function
but I don't have a strong preference...
>> +
>> + actx->key_len = len;
>> + actx->refkey = true;
>> +
>> + switch (key[0]) {
>> + case DCP_PAES_KEY_SLOT0:
>> + case DCP_PAES_KEY_SLOT1:
>> + case DCP_PAES_KEY_SLOT2:
>> + case DCP_PAES_KEY_SLOT3:
>> + case DCP_PAES_KEY_UNIQUE:
>> + case DCP_PAES_KEY_OTP:
>> + memcpy(actx->key, key, len);
>> + ret = 0;
>> + }
>
> In the error case you return -EINVAL below, but you still write
> into actx. Is that intentional?
You mean acts->key_len and actk->refkey?
Is this a problem?
Thanks,
//richard
Hi,
On 14.07.21 12:39, Richard Weinberger wrote:
> Ahmad,
>
> ----- Ursprüngliche Mail -----
>> Von: "Ahmad Fatoum" <[email protected]>
>> Let's trade reviews to get the ball rolling?
>
> Sounds like a fair deal. :-)
:)
> [...]
>
>>> --- a/drivers/crypto/mxs-dcp.c
>>> +++ b/drivers/crypto/mxs-dcp.c
>>> @@ -15,6 +15,7 @@
>>> #include <linux/platform_device.h>
>>> #include <linux/stmp_device.h>
>>> #include <linux/clk.h>
>>> +#include <linux/mxs-dcp.h>
>>
>> The CAAM specific headers are in <soc/fsl/*.h>.
>> Should this be done likewise here as well?
>
> I have no preferences. If soc/fsl/ is the way to go, fine by me.
I think it's the more appropriate place, but if the maintainers
are fine with <linux/mxs-dcp.h>, I don't mind.
>
> [...]
>
>>> @@ -219,15 +224,18 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
>>> struct dcp *sdcp = global_sdcp;
>>> struct dcp_dma_desc *desc = &sdcp->coh->desc[actx->chan];
>>> struct dcp_aes_req_ctx *rctx = skcipher_request_ctx(req);
>>> + dma_addr_t src_phys, dst_phys, key_phys = {0};
>>
>> Why = {0}; ? dma_addr_t is a scalar type and the value is always
>> written here before access.
>
> Initializing a scalar with {} is allowed in C, the braces are optional.
> I like the braces because it works even when the underlaying type changes.
> But that's just a matter of taste.
>
> key_phys is initialized because it triggered a false positive gcc warning
> on one of my targets. Let me re-run again to be sure, the code saw a lot of
> refactoring since that.
>
> [...]
>
>>> +static int mxs_dcp_aes_setrefkey(struct crypto_skcipher *tfm, const u8 *key,
>>> + unsigned int len)
>>> +{
>>> + struct dcp_async_ctx *actx = crypto_skcipher_ctx(tfm);
>>> + int ret = -EINVAL;
>>> +
>>> + if (len != DCP_PAES_KEYSIZE)
>>> + goto out;
>>
>> Nitpick: there is no cleanup, so why not return -EINVAL here
>> and unconditionally return 0 below?
>
> What is the benefit?
Similar to why you wouldn't write:
if (len == DCP_PAES_KEYSIZE) {
/* longer code block */
}
return ret;
Code is easier to scan through with early-exits.
> Usually I try to use goto to have a single exit point of a function
> but I don't have a strong preference...
It's just a nitpick. I am fine with it either way.
>>> +
>>> + actx->key_len = len;
>>> + actx->refkey = true;
>>> +
>>> + switch (key[0]) {
>>> + case DCP_PAES_KEY_SLOT0:
>>> + case DCP_PAES_KEY_SLOT1:
>>> + case DCP_PAES_KEY_SLOT2:
>>> + case DCP_PAES_KEY_SLOT3:
>>> + case DCP_PAES_KEY_UNIQUE:
>>> + case DCP_PAES_KEY_OTP:
>>> + memcpy(actx->key, key, len);
>>> + ret = 0;
>>> + }
>>
>> In the error case you return -EINVAL below, but you still write
>> into actx. Is that intentional?
>
> You mean acts->key_len and actk->refkey?
> Is this a problem?
It's easier to reason about code when it doesn't leave objects
it operates on in invalid states on failure. Changing key_len,
but leaving actx->key uninitialized is surprising IMO.
I can't judge whether this is a problem in practice, but less
surprises are a worthwhile goal.
Cheers,
Ahmad
>
> Thanks,
> //richard
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |