Message-Id: <CZLBJ1HAC4TO.1F1C2IV0Z9RTF@suppilovahvero>
Date: Tue, 05 Mar 2024 00:27:54 +0200
From: "Jarkko Sakkinen" <jarkko@...nel.org>
To: "David Gstir" <david@...ma-star.at>, "Mimi Zohar" <zohar@...ux.ibm.com>,
 "James Bottomley" <jejb@...ux.ibm.com>, "Herbert Xu"
 <herbert@...dor.apana.org.au>, "David S. Miller" <davem@...emloft.net>
Cc: "Shawn Guo" <shawnguo@...nel.org>, "Jonathan Corbet" <corbet@....net>,
 "Sascha Hauer" <s.hauer@...gutronix.de>, "Pengutronix Kernel Team"
 <kernel@...gutronix.de>, "Fabio Estevam" <festevam@...il.com>, "NXP Linux
 Team" <linux-imx@....com>, "Ahmad Fatoum" <a.fatoum@...gutronix.de>, "sigma
 star Kernel Team" <upstream+dcp@...ma-star.at>, "David Howells"
 <dhowells@...hat.com>, "Li Yang" <leoyang.li@....com>, "Paul Moore"
 <paul@...l-moore.com>, "James Morris" <jmorris@...ei.org>, "Serge E.
 Hallyn" <serge@...lyn.com>, "Paul E. McKenney" <paulmck@...nel.org>, "Randy
 Dunlap" <rdunlap@...radead.org>, "Catalin Marinas"
 <catalin.marinas@....com>, "Rafael J. Wysocki"
 <rafael.j.wysocki@...el.com>, "Tejun Heo" <tj@...nel.org>, "Steven Rostedt
 (Google)" <rostedt@...dmis.org>, <linux-doc@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <linux-integrity@...r.kernel.org>,
 <keyrings@...r.kernel.org>, <linux-crypto@...r.kernel.org>,
 <linux-arm-kernel@...ts.infradead.org>, <linuxppc-dev@...ts.ozlabs.org>,
 <linux-security-module@...r.kernel.org>, "Richard Weinberger"
 <richard@....at>, "David Oberhollenzer" <david.oberhollenzer@...ma-star.at>
Subject: Re: [PATCH v5 1/6] crypto: mxs-dcp: Add support for hardware-bound
 keys

Further remarks.

On Fri Dec 15, 2023 at 1:06 PM EET, David Gstir wrote:
> DCP is capable of performing AES with two hardware-bound keys:
>
> - The one-time programmable (OTP) key which is burnt via on-chip fuses
> - The unique key (UK) which is derived from the OTP key
>
> In addition to the two hardware-bound keys, DCP also supports
> storing keys in 4 dedicated key slots within its secure memory area
> (internal SRAM).
>
> These keys are not stored in main memory and are therefore
> not directly accessible by the operating system. To use them
> for AES operations, a one-byte key reference has to supplied
> with the DCP operation descriptor in the control register.
>
> This adds support for using any of these 6 keys through the crypto API
> via their key reference after they have been set up. The main purpose

Please write actions always in imperative form. E.g. instead of "This
adds" you could just as well simply write "Add", right?

Also, "adding support" is somewhat abstract expression tbh. You should
rather point out the driver exactly you are modifying (completely
missing BTW) and what sort of new functionalities this mysetery word
"support" maps into.

More cocrete and dumbed you can ever be, the better is the commit
message and more likely we also get the code changes you are doing.

> is to add support for DCP-backed trusted keys. Other use cases are
> possible too (see similar existing paes implementations), but these
> should carefully be evaluated as e.g. enabling AF_ALG will give
> userspace full access to use keys. In scenarios with untrustworthy
> userspace, this will enable en-/decryption oracles.
>
> Co-developed-by: Richard Weinberger <richard@....at>
> Signed-off-by: Richard Weinberger <richard@....at>
> Co-developed-by: David Oberhollenzer <david.oberhollenzer@...ma-star.at>
> Signed-off-by: David Oberhollenzer <david.oberhollenzer@...ma-star.at>
> Signed-off-by: David Gstir <david@...ma-star.at>
> Acked-by: Herbert Xu <herbert@...dor.apana.org.au>
> ---
>  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
>
> 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

nit: short description of the contents would not harm.

> + */
> +
> +#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 */

BR, Jarkko

Powered by blists - more mailing lists