diff mbox series

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

Message ID 20210614201620.30451-2-richard@nod.at
State New
Headers show
Series DCP as trusted keys backend | expand

Commit Message

Richard Weinberger June 14, 2021, 8:16 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.

Cc: Ahmad Fatoum <a.fatoum@pengutronix.de>
Cc: David Gstir <david@sigma-star.at>
Cc: David Howells <dhowells@redhat.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: James Morris <jmorris@namei.org>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: keyrings@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-crypto@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-integrity@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-security-module@vger.kernel.org
Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Richard Weinberger <richard@nod.at>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Shawn Guo <shawnguo@kernel.org>
Co-developed-by: David Gstir <david@sigma-star.at>
Signed-off-by: David Gstir <david@sigma-star.at>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
 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

Comments

Richard Weinberger June 25, 2021, 12:21 p.m. UTC | #1 
Herbert,

On Mon, Jun 14, 2021 at 10:18 PM Richard Weinberger <richard@nod.at> 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@pengutronix.de>

> Cc: David Gstir <david@sigma-star.at>

> Cc: David Howells <dhowells@redhat.com>

> Cc: "David S. Miller" <davem@davemloft.net>

> Cc: Fabio Estevam <festevam@gmail.com>

> Cc: Herbert Xu <herbert@gondor.apana.org.au>

> Cc: James Bottomley <jejb@linux.ibm.com>

> Cc: James Morris <jmorris@namei.org>

> Cc: Jarkko Sakkinen <jarkko@kernel.org>

> Cc: Jonathan Corbet <corbet@lwn.net>

> Cc: keyrings@vger.kernel.org

> Cc: linux-arm-kernel@lists.infradead.org

> Cc: linux-crypto@vger.kernel.org

> Cc: linux-doc@vger.kernel.org

> Cc: linux-integrity@vger.kernel.org

> Cc: linux-kernel@vger.kernel.org

> Cc: linux-security-module@vger.kernel.org

> Cc: Mimi Zohar <zohar@linux.ibm.com>

> Cc: NXP Linux Team <linux-imx@nxp.com>

> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>

> Cc: Richard Weinberger <richard@nod.at>

> Cc: Sascha Hauer <s.hauer@pengutronix.de>

> Cc: "Serge E. Hallyn" <serge@hallyn.com>

> Cc: Shawn Guo <shawnguo@kernel.org>

> Co-developed-by: David Gstir <david@sigma-star.at>

> Signed-off-by: David Gstir <david@sigma-star.at>

> Signed-off-by: Richard Weinberger <richard@nod.at>

> ---

>  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


This patch was judged as not applicable in your patchwork.
Is something missing? How can we proceed?

-- 
Thanks,
//richard
Herbert Xu June 25, 2021, 12:28 p.m. UTC | #2 
On Fri, Jun 25, 2021 at 02:21:16PM +0200, Richard Weinberger wrote:
>

> This patch was judged as not applicable in your patchwork.

> Is something missing? How can we proceed?


I'm happy to take this patch.  I marked it as not applicable
mainly because the other two patches didn't have acks and I'm
not sure if they were meant for the crypto tree or not.

Would you like me to take just the first patch?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Richard Weinberger June 25, 2021, 1:12 p.m. UTC | #3 
Herbert,

On Fri, Jun 25, 2021 at 2:29 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > This patch was judged as not applicable in your patchwork.

> > Is something missing? How can we proceed?

>

> I'm happy to take this patch.  I marked it as not applicable

> mainly because the other two patches didn't have acks and I'm

> not sure if they were meant for the crypto tree or not.


Maybe we have a chicken/egg situation and integrity folks wait for you. ;-)

> Would you like me to take just the first patch?


IMHO all three patches should go through the integrity tree.
Given that you're fine with the first patch, can you please ack it?

-- 
Thanks,
//richard
Ahmad Fatoum July 14, 2021, 9:24 a.m. UTC | #4 
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@pengutronix.de>

> Cc: David Gstir <david@sigma-star.at>

> Cc: David Howells <dhowells@redhat.com>

> Cc: "David S. Miller" <davem@davemloft.net>

> Cc: Fabio Estevam <festevam@gmail.com>

> Cc: Herbert Xu <herbert@gondor.apana.org.au>

> Cc: James Bottomley <jejb@linux.ibm.com>

> Cc: James Morris <jmorris@namei.org>

> Cc: Jarkko Sakkinen <jarkko@kernel.org>

> Cc: Jonathan Corbet <corbet@lwn.net>

> Cc: keyrings@vger.kernel.org

> Cc: linux-arm-kernel@lists.infradead.org

> Cc: linux-crypto@vger.kernel.org

> Cc: linux-doc@vger.kernel.org

> Cc: linux-integrity@vger.kernel.org

> Cc: linux-kernel@vger.kernel.org

> Cc: linux-security-module@vger.kernel.org

> Cc: Mimi Zohar <zohar@linux.ibm.com>

> Cc: NXP Linux Team <linux-imx@nxp.com>

> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>

> Cc: Richard Weinberger <richard@nod.at>

> Cc: Sascha Hauer <s.hauer@pengutronix.de>

> Cc: "Serge E. Hallyn" <serge@hallyn.com>

> Cc: Shawn Guo <shawnguo@kernel.org>

> Co-developed-by: David Gstir <david@sigma-star.at>

> Signed-off-by: David Gstir <david@sigma-star.at>

> Signed-off-by: Richard Weinberger <richard@nod.at>

> ---

>  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@sigma-star.at>

> + *          Richard Weinberger <richard@sigma-star.at>

> + */

> +

> +#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 |
Richard Weinberger July 14, 2021, 10:39 a.m. UTC | #5 
Ahmad,

----- Ursprüngliche Mail -----
> Von: "Ahmad Fatoum" <a.fatoum@pengutronix.de>

> 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
Ahmad Fatoum July 14, 2021, 11:01 a.m. UTC | #6 
Hi,

On 14.07.21 12:39, Richard Weinberger wrote:
> Ahmad,

> 

> ----- Ursprüngliche Mail -----

>> Von: "Ahmad Fatoum" <a.fatoum@pengutronix.de>

>> 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 |
diff mbox series

Patch

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>
 
 #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};
+	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;
+
+	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;
+	}
+
+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@sigma-star.at>
+ *          Richard Weinberger <richard@sigma-star.at>
+ */
+
+#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 */