diff mbox series

[v3,1/3] crypto: mxs-dcp: Add support for hardware provided keys

Message ID 20230918141826.8139-2-david@sigma-star.at (mailing list archive)
State Changes Requested
Delegated to: Herbert Xu
Headers show
Series DCP as trusted keys backend | expand

Commit Message

David Gstir Sept. 18, 2023, 2:18 p.m. UTC 
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 <richard@nod.at>
Signed-off-by: Richard Weinberger <richard@nod.at>
Co-developed-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
Signed-off-by: David Gstir <david@sigma-star.at>
---
 drivers/crypto/mxs-dcp.c | 104 ++++++++++++++++++++++++++++++++++-----
 include/soc/fsl/dcp.h    |  17 +++++++
 2 files changed, 110 insertions(+), 11 deletions(-)
 create mode 100644 include/soc/fsl/dcp.h

Comments

Jarkko Sakkinen Sept. 25, 2023, 3:22 p.m. UTC | #1 
On Mon Sep 18, 2023 at 5:18 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.

Not sure what the action of feeding key into DCP even means if such
action does not exists.

What you probably would want to describe here is how keys get created
and how they are referenced by the kernel.

For the "use" part please try to avoid academic paper style long
expression starting with "we" pronomine.

So the above paragraph would normalize into "The keys inside DCP
are referenced by one byte identifier". Here of course would be
for the context nice to know what is this set of DCP keys. E.g.
are total 256 keys or some subset?

When using too much prose there can be surprsingly little digestable
information, thus this nitpicking.

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

Unless this patch has anything else than trusted keys this should not
be an open-ended sentence. You want to say roughly that DCP hardware
keys are implemented for the sake to implement trusted keys support,
and exactly and only that.

This description also lacks actions taken by the code changes below,
which is really the beef of any commit description.

BR, Jarkko
David Gstir Sept. 27, 2023, 6:25 a.m. UTC | #2 
Jarkko,

> On 25.09.2023, at 17:22, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Mon Sep 18, 2023 at 5:18 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.
> 
> Not sure what the action of feeding key into DCP even means if such
> action does not exists.
> 
> What you probably would want to describe here is how keys get created
> and how they are referenced by the kernel.
> 
> For the "use" part please try to avoid academic paper style long
> expression starting with "we" pronomine.
> 
> So the above paragraph would normalize into "The keys inside DCP
> are referenced by one byte identifier". Here of course would be
> for the context nice to know what is this set of DCP keys. E.g.
> are total 256 keys or some subset?
> 
> When using too much prose there can be surprsingly little digestable
> information, thus this nitpicking.

Thanks for reviewing that in detail! I’ll rephrase the commit
messages on all patches to get rid of the academic paper style.


> 
>> 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.
> 
> Unless this patch has anything else than trusted keys this should not
> be an open-ended sentence. You want to say roughly that DCP hardware
> keys are implemented for the sake to implement trusted keys support,
> and exactly and only that.
> 
> This description also lacks actions taken by the code changes below,
> which is really the beef of any commit description.

You’re right. I’ll add that.

Thanks,
- David
Jarkko Sakkinen Oct. 2, 2023, 10:50 p.m. UTC | #3 
On Wed Sep 27, 2023 at 9:25 AM EEST, David Gstir wrote:
> Jarkko,
>
> > On 25.09.2023, at 17:22, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > On Mon Sep 18, 2023 at 5:18 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.
> > 
> > Not sure what the action of feeding key into DCP even means if such
> > action does not exists.
> > 
> > What you probably would want to describe here is how keys get created
> > and how they are referenced by the kernel.
> > 
> > For the "use" part please try to avoid academic paper style long
> > expression starting with "we" pronomine.
> > 
> > So the above paragraph would normalize into "The keys inside DCP
> > are referenced by one byte identifier". Here of course would be
> > for the context nice to know what is this set of DCP keys. E.g.
> > are total 256 keys or some subset?
> > 
> > When using too much prose there can be surprsingly little digestable
> > information, thus this nitpicking.
>
> Thanks for reviewing that in detail! I’ll rephrase the commit
> messages on all patches to get rid of the academic paper style.
>
>
> > 
> >> 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.
> > 
> > Unless this patch has anything else than trusted keys this should not
> > be an open-ended sentence. You want to say roughly that DCP hardware
> > keys are implemented for the sake to implement trusted keys support,
> > and exactly and only that.
> > 
> > This description also lacks actions taken by the code changes below,
> > which is really the beef of any commit description.
>
> You’re right. I’ll add that.

Yup, I'm just doing my part of the job, as I'm expected to do it :-)
Thanks for understanding.

> Thanks,
> - David

BR, Jarkko
diff mbox series

Patch

diff --git a/drivers/crypto/mxs-dcp.c b/drivers/crypto/mxs-dcp.c
index f6b7bce0e656..2dc664fb2faf 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 <soc/fsl/dcp.h>
 
 #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				key_referenced;
 };
 
 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 = &sdcp->coh->desc[actx->chan];
 	struct dcp_aes_req_ctx *rctx = skcipher_request_ctx(req);
+	bool key_referenced = actx->key_referenced;
 	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,12 @@  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 +282,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 +299,9 @@  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 +468,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->key_referenced))
 		return mxs_dcp_block_fallback(req, enc);
 
 	rctx->enc = enc;
@@ -500,6 +515,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->key_referenced = false;
 	if (len == AES_KEYSIZE_128) {
 		memcpy(actx->key, key, len);
 		return 0;
@@ -516,6 +532,32 @@  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);
+
+	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);
+		actx->key_len = len;
+		actx->key_referenced = true;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int mxs_dcp_aes_fallback_init_tfm(struct crypto_skcipher *tfm)
 {
 	const char *name = crypto_tfm_alg_name(crypto_skcipher_tfm(tfm));
@@ -539,6 +581,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)
  */
@@ -889,6 +938,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/soc/fsl/dcp.h b/include/soc/fsl/dcp.h
new file mode 100644
index 000000000000..cda89e260c46
--- /dev/null
+++ b/include/soc/fsl/dcp.h
@@ -0,0 +1,17 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2021 sigma star gmbh
+ */
+
+#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 */