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 <a.fatoum@xxxxxxxxxxxxxx> > Cc: David Gstir <david@xxxxxxxxxxxxx> > Cc: David Howells <dhowells@xxxxxxxxxx> > Cc: "David S. Miller" <davem@xxxxxxxxxxxxx> > Cc: Fabio Estevam <festevam@xxxxxxxxx> > Cc: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> > Cc: James Bottomley <jejb@xxxxxxxxxxxxx> > Cc: James Morris <jmorris@xxxxxxxxx> > Cc: Jarkko Sakkinen <jarkko@xxxxxxxxxx> > Cc: Jonathan Corbet <corbet@xxxxxxx> > Cc: keyrings@xxxxxxxxxxxxxxx > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > Cc: linux-crypto@xxxxxxxxxxxxxxx > Cc: linux-doc@xxxxxxxxxxxxxxx > Cc: linux-integrity@xxxxxxxxxxxxxxx > Cc: linux-kernel@xxxxxxxxxxxxxxx > Cc: linux-security-module@xxxxxxxxxxxxxxx > Cc: Mimi Zohar <zohar@xxxxxxxxxxxxx> > Cc: NXP Linux Team <linux-imx@xxxxxxx> > Cc: Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx> > Cc: Richard Weinberger <richard@xxxxxx> > Cc: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> > Cc: "Serge E. Hallyn" <serge@xxxxxxxxxx> > Cc: Shawn Guo <shawnguo@xxxxxxxxxx> > Co-developed-by: David Gstir <david@xxxxxxxxxxxxx> > Signed-off-by: David Gstir <david@xxxxxxxxxxxxx> > Signed-off-by: Richard Weinberger <richard@xxxxxx> > --- > 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 <david@xxxxxxxxxxxxx> > + * Richard Weinberger <richard@xxxxxxxxxxxxx> > + */ > + > +#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 |