Re: [PATCH v2 1/3] crypto: mxs-dcp: Add support for hardware provided keys
Hi Jarkko,
thanks for the review!
> On 12.09.2023, at 19:32, Jarkko Sakkinen wrote:
>
> On Tue Sep 12, 2023 at 2:11 PM EEST, David Gstir wrote:
[...]
>> - /* 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;
>> + }
>
> Remove curly braces (coding style).
Will fix that and all similar cases for v3.
>> +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);
>> +
>> + if (len != DCP_PAES_KEYSIZE)
>> + return -EINVAL;
>> +
>> + 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);
>> + break;
>
> I don't understand why the "commit" is split into two parts
> (memcpy and assignments in different code blocks). You should
> probably rather:
>
> 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);
> actx->key_len = len;
> actx->refkey = true;
> return 0;
>
> default:
> return -EINVAL;
> }
> }
>
> Or alternatively you can move all operations after the switch-case
> statement. IMHO, any state change is better to put into a singular
> location.
Makes sense. I’ll change that.
>> diff --git a/include/soc/fsl/dcp.h b/include/soc/fsl/dcp.h
>> new file mode 100644
>> index ..df6678ee10a1
>> --- /dev/null
>> +++ b/include/soc/fsl/dcp.h
>> @@ -0,0 +1,19 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (C) 2021 sigma star gmbh
>> + * Authors: David Gstir
>> + * Richard Weinberger
>
> Git already has author-field and commit can have co-developed-by so
> this is totally obsolete.
Also noted for v3.
Thanks,
- David
--
sigma star gmbh | Eduard-Bodem-Gasse 6, 6020 Innsbruck, Austria
UID/VAT Nr: ATU 66964118 | FN: 374287y
[PATCH v2 1/3] crypto: mxs-dcp: Add support for hardware provided keys
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.
Co-developed-by: Richard Weinberger
Signed-off-by: Richard Weinberger
Co-developed-by: David Oberhollenzer
Signed-off-by: David Oberhollenzer
Signed-off-by: David Gstir
---
drivers/crypto/mxs-dcp.c | 107 +++
include/soc/fsl/dcp.h| 19 +++
2 files changed, 115 insertions(+), 11 deletions(-)
create mode 100644 include/soc/fsl/dcp.h
diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c
index f6b7bce0e656..d525cb41f2ca 100644
--- a/drivers/crypto/mxs-dcp.c
+++ b/drivers/crypto/mxs-dcp.c
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
@@ -101,6 +102,7 @@ struct dcp_async_ctx {
struct crypto_skcipher *fallback;
unsigned intkey_len;
uint8_t key[AES_KEYSIZE_128];
+ boolrefkey;
};
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)
{
int dma_err;
@@ -224,13 +229,16 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
struct dcp *sdcp = global_sdcp;
struct dcp_dma_desc *desc = >coh->desc[actx->chan];
struct dcp_aes_req_ctx *rctx = skcipher_request_ctx(req);
+ bool key_referenced = actx->refkey;
int ret;
- key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key,
- 2 * AES_KEYSIZE_128, DMA_TO_DEVICE);
- ret = dma_mapping_error(sdcp->dev, key_phys);
- if (ret)
- return ret;
+ if (!key_referenced) {
+ key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key,
+ 2 * AES_KEYSIZE_128, DMA_TO_DEVICE);
+ ret = dma_mapping_error(sdcp->dev, key_phys);
+ if (ret)
+ return ret;
+ }
src_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_in_buf,
DCP_BUF_SZ, DMA_TO_DEVICE);
@@ -255,8 +263,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;
@@ -270,6 +283,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;
@@ -284,9 +300,10 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
err_dst:
dma_unmap_single(sdcp->dev, src_phys, DCP_BUF_SZ, DMA_TO_DEVICE);
err_src:
- 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);
+ }
return ret;
}
@@ -453,7 +470,7 @@
Re: [PATCH v2 1/3] crypto: mxs-dcp: Add support for hardware provided keys
On Tue Sep 12, 2023 at 2:11 PM EEST, David Gstir wrote:
> @@ -101,6 +102,7 @@ struct dcp_async_ctx {
> struct crypto_skcipher *fallback;
> unsigned intkey_len;
> uint8_t key[AES_KEYSIZE_128];
> + boolrefkey;
> };
s/refkey/key_referenced/
No reason to obfuscate it, especially since there is no supporting
documentation.
BR, Jarkko
Re: [PATCH v2 1/3] crypto: mxs-dcp: Add support for hardware provided keys
On Tue Sep 12, 2023 at 2:11 PM EEST, David Gstir 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.
>
> Co-developed-by: Richard Weinberger
> Signed-off-by: Richard Weinberger
> Co-developed-by: David Oberhollenzer
> Signed-off-by: David Oberhollenzer
> Signed-off-by: David Gstir
> ---
> drivers/crypto/mxs-dcp.c | 107 +++
> include/soc/fsl/dcp.h| 19 +++
> 2 files changed, 115 insertions(+), 11 deletions(-)
> create mode 100644 include/soc/fsl/dcp.h
>
> diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c
> index f6b7bce0e656..d525cb41f2ca 100644
> --- a/drivers/crypto/mxs-dcp.c
> +++ b/drivers/crypto/mxs-dcp.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -101,6 +102,7 @@ struct dcp_async_ctx {
> struct crypto_skcipher *fallback;
> unsigned intkey_len;
> uint8_t key[AES_KEYSIZE_128];
> + boolrefkey;
> };
>
> 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_SHIFT8
> +
> static int mxs_dcp_start_dma(struct dcp_async_ctx *actx)
> {
> int dma_err;
> @@ -224,13 +229,16 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
> struct dcp *sdcp = global_sdcp;
> struct dcp_dma_desc *desc = >coh->desc[actx->chan];
> struct dcp_aes_req_ctx *rctx = skcipher_request_ctx(req);
> + bool key_referenced = actx->refkey;
> int ret;
>
> - key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key,
> - 2 * AES_KEYSIZE_128, DMA_TO_DEVICE);
> - ret = dma_mapping_error(sdcp->dev, key_phys);
> - if (ret)
> - return ret;
> + if (!key_referenced) {
> + key_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_key,
> + 2 * AES_KEYSIZE_128, DMA_TO_DEVICE);
> + ret = dma_mapping_error(sdcp->dev, key_phys);
> + if (ret)
> + return ret;
> + }
>
> src_phys = dma_map_single(sdcp->dev, sdcp->coh->aes_in_buf,
> DCP_BUF_SZ, DMA_TO_DEVICE);
> @@ -255,8 +263,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;
> + }
Remove curly braces (coding style).
>
> if (rctx->enc)
> desc->control0 |= MXS_DCP_CONTROL0_CIPHER_ENCRYPT;
> @@ -270,6 +283,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;
> @@ -284,9 +300,10 @@ static int mxs_dcp_run_aes(struct dcp_async_ctx *actx,
> err_dst:
> dma_unmap_single(sdcp->dev, src_phys, DCP_BUF_SZ, DMA_TO_DEVICE);
> err_src:
> - dma_unmap_single(sdcp->dev, key_phys, 2 * AES_KEYSIZE_128,
> -
