linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v0 2/8] keys-trusted: new cmd line option added
  2022-10-06 13:08 ` [PATCH v0 2/8] keys-trusted: new cmd line option added Pankaj Gupta
@ 2022-10-06 12:37   ` Ben Boeckel
  0 siblings, 0 replies; 31+ messages in thread
From: Ben Boeckel @ 2022-10-06 12:37 UTC (permalink / raw)
  To: Pankaj Gupta
  Cc: jarkko, a.fatoum, gilad, Jason, jejb, zohar, dhowells,
	sumit.garg, david, michael, john.ernberg, jmorris, serge,
	herbert, davem, j.luebbe, ebiggers, richard, keyrings,
	linux-crypto, linux-integrity, linux-kernel,
	linux-security-module, sahil.malhotra, kshitiz.varshney,
	horia.geanta, V.Sethi

On Thu, Oct 06, 2022 at 18:38:31 +0530, Pankaj Gupta wrote:
> Changes done:
> - new cmd line option "hw" needs to be suffix, to generate the
>   hw bound key.

`Documentation/` is silent on this. Can you please add this there?

Other than that, is `hw` really a good name for this? Are there virtual
devices for these things that can make them not hardware in some way?
Is there a better name in such a case? Maybe something "device"
oriented?

--Ben

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v0 6/8] KEYS: trusted: caam based black key
  2022-10-06 13:08 ` [PATCH v0 6/8] KEYS: trusted: caam based black key Pankaj Gupta
@ 2022-10-06 12:42   ` Ben Boeckel
  2022-10-06 12:52     ` James Bottomley
  0 siblings, 1 reply; 31+ messages in thread
From: Ben Boeckel @ 2022-10-06 12:42 UTC (permalink / raw)
  To: Pankaj Gupta
  Cc: jarkko, a.fatoum, gilad, Jason, jejb, zohar, dhowells,
	sumit.garg, david, michael, john.ernberg, jmorris, serge,
	herbert, davem, j.luebbe, ebiggers, richard, keyrings,
	linux-crypto, linux-integrity, linux-kernel,
	linux-security-module, sahil.malhotra, kshitiz.varshney,
	horia.geanta, V.Sethi

On Thu, Oct 06, 2022 at 18:38:35 +0530, Pankaj Gupta wrote:
> - CAAM supports two types of black keys:
>   -- Plain key encrypted with ECB
>   -- Plain key encrypted with CCM

What is a "black key"? Is this described in the documentation or local
comments at all? (I know I'm unfamiliar with CAAM, but maybe this should
be mentioned somewhere?).

>   Note: Due to robustness, default encytption used for black key is CCM.
                                     ^^^^^^^^^^ encryption

What "robustness"? Surely there's some more technical details involved
here?

> - A black key blob is generated, and added to trusted key payload.
>   This is done as part of sealing operation, that was triggered as a result of:
>   -- new key generation
>   -- load key,

It seems that "black keys" are what the uapi calls "hw". I think this
should be mentioned in the commit message (and CAAM docs).

What do other keytypes do if `hw` is requested and it's not possible
(say, `big_key`)?

Thanks,

--Ben

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v0 6/8] KEYS: trusted: caam based black key
  2022-10-06 12:42   ` Ben Boeckel
@ 2022-10-06 12:52     ` James Bottomley
  0 siblings, 0 replies; 31+ messages in thread
From: James Bottomley @ 2022-10-06 12:52 UTC (permalink / raw)
  To: list.lkml.keyrings, Pankaj Gupta
  Cc: jarkko, a.fatoum, gilad, Jason, zohar, dhowells, sumit.garg,
	david, michael, john.ernberg, jmorris, serge, herbert, davem,
	j.luebbe, ebiggers, richard, keyrings, linux-crypto,
	linux-integrity, linux-kernel, linux-security-module,
	sahil.malhotra, kshitiz.varshney, horia.geanta, V.Sethi

On Thu, 2022-10-06 at 08:42 -0400, Ben Boeckel wrote:
> On Thu, Oct 06, 2022 at 18:38:35 +0530, Pankaj Gupta wrote:
> > - CAAM supports two types of black keys:
> >   -- Plain key encrypted with ECB
> >   -- Plain key encrypted with CCM
> 
> What is a "black key"? Is this described in the documentation or
> local comments at all? (I know I'm unfamiliar with CAAM, but maybe
> this should be mentioned somewhere?).
> 
> >   Note: Due to robustness, default encytption used for black key is
> > CCM.
>                                      ^^^^^^^^^^ encryption
> 
> What "robustness"? Surely there's some more technical details
> involved here?

The crypto advice for the past decade or more has been never use ECB
it's insecure, so anything could be regarded as robust compared to it
... however that does beg the question of why ECB is even offered in a
modern system?  Surely it's nothing more than a user trap (choose this
secure option only if you don't want security).

James



^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v0 0/8] Hardware Bound key added to Trusted Key-Ring
@ 2022-10-06 13:08 Pankaj Gupta
  2022-10-06 13:08 ` [PATCH v0 1/8] hw-bound-key: introducing the generic structure Pankaj Gupta
                   ` (7 more replies)
  0 siblings, 8 replies; 31+ messages in thread
From: Pankaj Gupta @ 2022-10-06 13:08 UTC (permalink / raw)
  To: jarkko, a.fatoum, gilad, Jason, jejb, zohar, dhowells,
	sumit.garg, david, michael, john.ernberg, jmorris, serge,
	herbert, davem, j.luebbe, ebiggers, richard, keyrings,
	linux-crypto, linux-integrity, linux-kernel,
	linux-security-module, sahil.malhotra, kshitiz.varshney,
	horia.geanta, V.Sethi
  Cc: Pankaj Gupta

Hardware Bound key(HBK), is never available as a plain key buffer outside of the hardware boundary.
Thus, it is un-usable, even if somehow fetched from kernel memory.
It ensures run-time security.

This patchset establishes a generic bridge between HBK & the kernel's trusted-keyring.
This patchset adds generic support for identifying if the key is HBK.

Note: Copy-paste this text-based block diagram to notepad, for better
clarity.
                      +---------------+
                      |               |
                      |    keyctl     |
                      |               |
                      +-------+-------+
                              |
                              |
      +-----------------------+-------------------------+
      |                trusted keyring                  |
      |                               +---------------+ |
      |   +-----------------+         |trusted-key    | |
      |   |                 |-------->|payload (plain)| |
      |   |                 |         +---------------+ |
      |-->| trusted key     |         +---------------+ |
      |   | source as CAAM  |-------->|  trusted-key  | |
      |   +-----------------+         |  payload (HBK)| |
      |                               +-------^-------+ |
      +---------------------------------------|---------+
                                              |
                                              |
                                              |
 +----------------+  crypt_alloc_tfms +-------|------------------+
 | Kernel         |<------------------|        DM-Crypt          |
 | Crypto-API     |                   | +---------------------+  |
 |                |------------------>| |struct crypto_tfm:   |  |
 +----------------+  crypto_tfm(HBK)  | |- flag-is_hbk        |  |
                                      | |- struct-hbk_info,   |  |
                                      | |is copied from the   |  |
                                      | |tkp structure        |  |
                                      | +---------------------+  |
                                      +------------|-------------+
                                                   |
                                                   |
                                                   |crypto_tfm(HBK)
                                                   |
                                   +---------------|--------------+
                                   | Hardware crypto driver       |
                                   |                              |
                                   | Processing the incoming      |
                                   | key based on the flag        |
                                   | - as plain key, if is_hbk = 0|
                                   | - as HBK, if is_hbk = 1      |
                                   +------------------------------+

Major additions done: (in brief)

- Newly added:
  -- flag-'is_hbk', and
  -- struct hw_bound_key_info hbk_info,
  added to the structures - tfm & trusted key payload.

- Enhanced the 'keyctl' command to generate the hardware bound key
  as trusted key.
  -- at the time of generating the HBK, the values: 'flag - is_hbk'
     & structure 'hbk_info', in the trusted key payload, is set by
     the hw driver, generating or loading the key in the trusted
     key-ring.

- Applications like dm-crypt,
  -- first fetch the key from trusted key-ring. As part of doing this,
     application retains the values of: 'flag - is_hbk' & structure 'hbk_info'.

  -- to use kernel crypto api, after allocating the transformation,
     application sets the 'flag - is_hbk' & structure 'hbk_info', 
     to the tfm allocated from crypto_alloc_tfm().

- Newly added information to tfm, helps to influence the core processing logic
  for the encapsulated algorithm.

First implementation is based on CAAM.

NXP built CAAM IP is the Cryptographic Acceleration and Assurance Module.
This is contain by the i.MX and QorIQ SoCs by NXP.

CAAM is a suitable backend (source) for kernel trusted keys.
This backend source can be used for run-time security as well by generating the hardware bound key.

Along with plain key, the CAAM generates black key. A black key is an encrypted key, which can only be decrypted inside CAAM.
Hence, CAAM's black key can only be used by CAAM. Thus it is declared as a hardware bound key.

Pankaj Gupta (8):
  hw-bound-key: introducing the generic structure
  keys-trusted: new cmd line option added
  crypto: hbk flags & info added to the tfm
  sk_cipher: checking for hw bound operation
  keys-trusted: re-factored caam based trusted key
  KEYS: trusted: caam based black key
  caam alg: symmetric key ciphers are updated
  dm-crypt: consumer-app setting the flag-is_hbk

 crypto/skcipher.c                         |   3 +-
 drivers/crypto/caam/blob_gen.c            | 221 ++++++++++++++++++++--
 drivers/crypto/caam/caamalg.c             |  43 ++++-
 drivers/crypto/caam/caamalg_desc.c        |   8 +-
 drivers/crypto/caam/desc.h                |   8 +-
 drivers/crypto/caam/desc_constr.h         |   6 +-
 drivers/md/dm-crypt.c                     |  12 +-
 include/keys/trusted-type.h               |   4 +
 include/linux/crypto.h                    |   5 +
 include/linux/hw_bound_key.h              |  27 +++
 include/soc/fsl/caam-blob.h               |  38 ++--
 security/keys/trusted-keys/trusted_caam.c |   8 +
 security/keys/trusted-keys/trusted_core.c |  16 ++
 13 files changed, 356 insertions(+), 43 deletions(-)
 create mode 100644 include/linux/hw_bound_key.h

-- 
2.17.1


^ permalink raw reply	[flat|nested] 31+ messages in thread

* [PATCH v0 1/8] hw-bound-key: introducing the generic structure
  2022-10-06 13:08 [PATCH v0 0/8] Hardware Bound key added to Trusted Key-Ring Pankaj Gupta
@ 2022-10-06 13:08 ` Pankaj Gupta
  2022-10-12  8:52   ` Jarkko Sakkinen
  2022-10-12  8:53   ` Jarkko Sakkinen
  2022-10-06 13:08 ` [PATCH v0 2/8] keys-trusted: new cmd line option added Pankaj Gupta
                   ` (6 subsequent siblings)
  7 siblings, 2 replies; 31+ messages in thread
From: Pankaj Gupta @ 2022-10-06 13:08 UTC (permalink / raw)
  To: jarkko, a.fatoum, gilad, Jason, jejb, zohar, dhowells,
	sumit.garg, david, michael, john.ernberg, jmorris, serge,
	herbert, davem, j.luebbe, ebiggers, richard, keyrings,
	linux-crypto, linux-integrity, linux-kernel,
	linux-security-module, sahil.malhotra, kshitiz.varshney,
	horia.geanta, V.Sethi
  Cc: Pankaj Gupta

Hardware bound keys buffer has additional information,
that will be accessed using this new structure.

structure members are:
- flags, flags for hardware specific information.
- key_sz, size of the plain key.

Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
---
 include/linux/hw_bound_key.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 include/linux/hw_bound_key.h

diff --git a/include/linux/hw_bound_key.h b/include/linux/hw_bound_key.h
new file mode 100644
index 000000000000..e7f152410438
--- /dev/null
+++ b/include/linux/hw_bound_key.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0-only
+ *
+ * Copyright 2022 NXP
+ * Author: Pankaj Gupta <pankaj.gupta@nxp.com>
+ */
+
+#ifndef _HW_BOUND_KEY_H
+#define _HW_BOUND_KEY_H
+
+#include "types.h"
+
+struct hw_bound_key_info {
+	/* Key types specific to the hw. [Implementation Defined]
+	 */
+	uint8_t flags;
+	uint8_t reserved;
+	/* Plain key size.
+	 */
+	uint16_t key_sz;
+};
+
+#define set_hbk_info(hbk_info, hw_flags, key_len) do {\
+	hbk_info->flags = hw_flags;\
+	hbk_info->key_sz = key_len;\
+} while (0)
+
+#endif /* _HW_BOUND_KEY_H */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v0 2/8] keys-trusted: new cmd line option added
  2022-10-06 13:08 [PATCH v0 0/8] Hardware Bound key added to Trusted Key-Ring Pankaj Gupta
  2022-10-06 13:08 ` [PATCH v0 1/8] hw-bound-key: introducing the generic structure Pankaj Gupta
@ 2022-10-06 13:08 ` Pankaj Gupta
  2022-10-06 12:37   ` Ben Boeckel
  2022-10-06 13:08 ` [PATCH v0 3/8] crypto: hbk flags & info added to the tfm Pankaj Gupta
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 31+ messages in thread
From: Pankaj Gupta @ 2022-10-06 13:08 UTC (permalink / raw)
  To: jarkko, a.fatoum, gilad, Jason, jejb, zohar, dhowells,
	sumit.garg, david, michael, john.ernberg, jmorris, serge,
	herbert, davem, j.luebbe, ebiggers, richard, keyrings,
	linux-crypto, linux-integrity, linux-kernel,
	linux-security-module, sahil.malhotra, kshitiz.varshney,
	horia.geanta, V.Sethi
  Cc: Pankaj Gupta

Changes done:
- new cmd line option "hw" needs to be suffix, to generate the
  hw bound key.
  for ex:
   $:> keyctl add trusted <KEYNAME> 'new 32 hw' @s
   $:> keyctl add trusted <KEYNAME> 'load $(cat <KEY_BLOB_FILE_NAME>) hw' @s
- Key-payload, is added with two more information element specific to HBK
  -- flag 'is_hw_bound'
  -- structure 'struct hw_bound_key_info hbk_info'

Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
---
 include/keys/trusted-type.h               |  4 ++++
 security/keys/trusted-keys/trusted_core.c | 16 ++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h
index 4eb64548a74f..bf58a204a974 100644
--- a/include/keys/trusted-type.h
+++ b/include/keys/trusted-type.h
@@ -7,6 +7,7 @@
 #ifndef _KEYS_TRUSTED_TYPE_H
 #define _KEYS_TRUSTED_TYPE_H
 
+#include <linux/hw_bound_key.h>
 #include <linux/key.h>
 #include <linux/rcupdate.h>
 #include <linux/tpm.h>
@@ -22,6 +23,7 @@
 #define MAX_BLOB_SIZE			512
 #define MAX_PCRINFO_SIZE		64
 #define MAX_DIGEST_SIZE			64
+#define HW_BOUND_KEY                    1
 
 struct trusted_key_payload {
 	struct rcu_head rcu;
@@ -29,6 +31,8 @@ struct trusted_key_payload {
 	unsigned int blob_len;
 	unsigned char migratable;
 	unsigned char old_format;
+	unsigned char is_hw_bound;
+	struct hw_bound_key_info hbk_info;
 	unsigned char key[MAX_KEY_SIZE + 1];
 	unsigned char blob[MAX_BLOB_SIZE];
 };
diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c
index c6fc50d67214..cb1d56397ed0 100644
--- a/security/keys/trusted-keys/trusted_core.c
+++ b/security/keys/trusted-keys/trusted_core.c
@@ -79,6 +79,8 @@ static int datablob_parse(char **datablob, struct trusted_key_payload *p)
 	int key_cmd;
 	char *c;
 
+	p->is_hw_bound = !HW_BOUND_KEY;
+
 	/* main command */
 	c = strsep(datablob, " \t");
 	if (!c)
@@ -94,6 +96,13 @@ static int datablob_parse(char **datablob, struct trusted_key_payload *p)
 		if (ret < 0 || keylen < MIN_KEY_SIZE || keylen > MAX_KEY_SIZE)
 			return -EINVAL;
 		p->key_len = keylen;
+		do {
+			/* Second argument onwards,
+			 * determine if tied to HW */
+			c = strsep(datablob, " \t");
+			if ((c != NULL) && (strcmp(c, "hw") == 0))
+				p->is_hw_bound = HW_BOUND_KEY;
+		} while (c != NULL);
 		ret = Opt_new;
 		break;
 	case Opt_load:
@@ -107,6 +116,13 @@ static int datablob_parse(char **datablob, struct trusted_key_payload *p)
 		ret = hex2bin(p->blob, c, p->blob_len);
 		if (ret < 0)
 			return -EINVAL;
+		do {
+			/* Second argument onwards,
+			 * determine if tied to HW */
+			c = strsep(datablob, " \t");
+			if ((c != NULL) && (strcmp(c, "hw") == 0))
+				p->is_hw_bound = HW_BOUND_KEY;
+		} while (c != NULL);
 		ret = Opt_load;
 		break;
 	case Opt_update:
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
  2022-10-06 13:08 [PATCH v0 0/8] Hardware Bound key added to Trusted Key-Ring Pankaj Gupta
  2022-10-06 13:08 ` [PATCH v0 1/8] hw-bound-key: introducing the generic structure Pankaj Gupta
  2022-10-06 13:08 ` [PATCH v0 2/8] keys-trusted: new cmd line option added Pankaj Gupta
@ 2022-10-06 13:08 ` Pankaj Gupta
  2022-10-07  6:58   ` Herbert Xu
  2022-10-12  8:57   ` Jarkko Sakkinen
  2022-10-06 13:08 ` [PATCH v0 4/8] sk_cipher: checking for hw bound operation Pankaj Gupta
                   ` (4 subsequent siblings)
  7 siblings, 2 replies; 31+ messages in thread
From: Pankaj Gupta @ 2022-10-06 13:08 UTC (permalink / raw)
  To: jarkko, a.fatoum, gilad, Jason, jejb, zohar, dhowells,
	sumit.garg, david, michael, john.ernberg, jmorris, serge,
	herbert, davem, j.luebbe, ebiggers, richard, keyrings,
	linux-crypto, linux-integrity, linux-kernel,
	linux-security-module, sahil.malhotra, kshitiz.varshney,
	horia.geanta, V.Sethi
  Cc: Pankaj Gupta

Consumer of the kernel crypto api, after allocating
the transformation (tfm), sets the:
- flag 'is_hbk'
- structure 'struct hw_bound_key_info hbk_info'
based on the type of key, the consumer is using.

This helps:

- This helps to influence the core processing logic
  for the encapsulated algorithm.
- This flag is set by the consumer after allocating
  the tfm and before calling the function crypto_xxx_setkey().

Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
---
 include/linux/crypto.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 2324ab6f1846..cd476f8a1cb4 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -19,6 +19,7 @@
 #include <linux/refcount.h>
 #include <linux/slab.h>
 #include <linux/completion.h>
+#include <linux/hw_bound_key.h>
 
 /*
  * Autoloaded crypto modules should only use a prefixed name to avoid allowing
@@ -639,6 +640,10 @@ struct crypto_tfm {
 
 	u32 crt_flags;
 
+	unsigned int is_hbk;
+
+	struct hw_bound_key_info hbk_info;
+
 	int node;
 	
 	void (*exit)(struct crypto_tfm *tfm);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v0 4/8] sk_cipher: checking for hw bound operation
  2022-10-06 13:08 [PATCH v0 0/8] Hardware Bound key added to Trusted Key-Ring Pankaj Gupta
                   ` (2 preceding siblings ...)
  2022-10-06 13:08 ` [PATCH v0 3/8] crypto: hbk flags & info added to the tfm Pankaj Gupta
@ 2022-10-06 13:08 ` Pankaj Gupta
  2022-10-12  8:59   ` Jarkko Sakkinen
  2022-10-06 13:08 ` [PATCH v0 5/8] keys-trusted: re-factored caam based trusted key Pankaj Gupta
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 31+ messages in thread
From: Pankaj Gupta @ 2022-10-06 13:08 UTC (permalink / raw)
  To: jarkko, a.fatoum, gilad, Jason, jejb, zohar, dhowells,
	sumit.garg, david, michael, john.ernberg, jmorris, serge,
	herbert, davem, j.luebbe, ebiggers, richard, keyrings,
	linux-crypto, linux-integrity, linux-kernel,
	linux-security-module, sahil.malhotra, kshitiz.varshney,
	horia.geanta, V.Sethi
  Cc: Pankaj Gupta

Checking for hw bound key. If yes,
 - skipping the key-length validation to fall in min-max range.

Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
---
 crypto/skcipher.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/crypto/skcipher.c b/crypto/skcipher.c
index 418211180cee..0f2d0228d73e 100644
--- a/crypto/skcipher.c
+++ b/crypto/skcipher.c
@@ -598,7 +598,8 @@ int crypto_skcipher_setkey(struct crypto_skcipher *tfm, const u8 *key,
 	unsigned long alignmask = crypto_skcipher_alignmask(tfm);
 	int err;
 
-	if (keylen < cipher->min_keysize || keylen > cipher->max_keysize)
+	if ((!tfm->base.is_hbk)
+	    && (keylen < cipher->min_keysize || keylen > cipher->max_keysize))
 		return -EINVAL;
 
 	if ((unsigned long)key & alignmask)
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v0 5/8] keys-trusted: re-factored caam based trusted key
  2022-10-06 13:08 [PATCH v0 0/8] Hardware Bound key added to Trusted Key-Ring Pankaj Gupta
                   ` (3 preceding siblings ...)
  2022-10-06 13:08 ` [PATCH v0 4/8] sk_cipher: checking for hw bound operation Pankaj Gupta
@ 2022-10-06 13:08 ` Pankaj Gupta
  2022-10-06 13:08 ` [PATCH v0 6/8] KEYS: trusted: caam based black key Pankaj Gupta
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 31+ messages in thread
From: Pankaj Gupta @ 2022-10-06 13:08 UTC (permalink / raw)
  To: jarkko, a.fatoum, gilad, Jason, jejb, zohar, dhowells,
	sumit.garg, david, michael, john.ernberg, jmorris, serge,
	herbert, davem, j.luebbe, ebiggers, richard, keyrings,
	linux-crypto, linux-integrity, linux-kernel,
	linux-security-module, sahil.malhotra, kshitiz.varshney,
	horia.geanta, V.Sethi
  Cc: Pankaj Gupta

Re-factored caam based trusted key code:
- Two separate definition for encap and decap having
  separate code for creating CAAM job descriptor.

Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
---
 drivers/crypto/caam/blob_gen.c | 118 ++++++++++++++++++++++++++++++---
 include/soc/fsl/caam-blob.h    |  23 ++-----
 2 files changed, 114 insertions(+), 27 deletions(-)

diff --git a/drivers/crypto/caam/blob_gen.c b/drivers/crypto/caam/blob_gen.c
index 6345c7269eb0..36683ec9aee0 100644
--- a/drivers/crypto/caam/blob_gen.c
+++ b/drivers/crypto/caam/blob_gen.c
@@ -2,6 +2,7 @@
 /*
  * Copyright (C) 2015 Pengutronix, Steffen Trumtrar <kernel@pengutronix.de>
  * Copyright (C) 2021 Pengutronix, Ahmad Fatoum <kernel@pengutronix.de>
+ * Copyright 2022 NXP, Pankaj Gupta <pankaj.gupta@nxp.com>
  */
 
 #define pr_fmt(fmt) "caam blob_gen: " fmt
@@ -58,8 +59,19 @@ static void caam_blob_job_done(struct device *dev, u32 *desc, u32 err, void *con
 	complete(&res->completion);
 }
 
-int caam_process_blob(struct caam_blob_priv *priv,
-		      struct caam_blob_info *info, bool encap)
+
+
+/** caam_encap_blob - encapsulate blob
+ *
+ * @priv:   instance returned by caam_blob_gen_init
+ * @info:   pointer to blobbing info describing input key,
+ *          output blob and key modifier buffers.
+ *
+ * returns 0 and sets info->output_len on success and returns
+ * a negative error code otherwise.
+ */
+int caam_encap_blob(struct caam_blob_priv *priv,
+		    struct caam_blob_info *info)
 {
 	struct caam_blob_job_result testres;
 	struct device *jrdev = &priv->jrdev;
@@ -72,14 +84,102 @@ int caam_process_blob(struct caam_blob_priv *priv,
 	if (info->key_mod_len > CAAM_BLOB_KEYMOD_LENGTH)
 		return -EINVAL;
 
-	if (encap) {
-		op |= OP_TYPE_ENCAP_PROTOCOL;
-		output_len = info->input_len + CAAM_BLOB_OVERHEAD;
-	} else {
-		op |= OP_TYPE_DECAP_PROTOCOL;
-		output_len = info->input_len - CAAM_BLOB_OVERHEAD;
+	op |= OP_TYPE_ENCAP_PROTOCOL;
+	output_len = info->input_len + CAAM_BLOB_OVERHEAD;
+
+	desc = kzalloc(CAAM_BLOB_DESC_BYTES_MAX, GFP_KERNEL | GFP_DMA);
+	if (!desc)
+		return -ENOMEM;
+
+	dma_in = dma_map_single(jrdev, info->input, info->input_len,
+				DMA_TO_DEVICE);
+	if (dma_mapping_error(jrdev, dma_in)) {
+		dev_err(jrdev, "unable to map input DMA buffer\n");
+		ret = -ENOMEM;
+		goto out_free;
+	}
+
+	dma_out = dma_map_single(jrdev, info->output, output_len,
+				 DMA_FROM_DEVICE);
+	if (dma_mapping_error(jrdev, dma_out)) {
+		dev_err(jrdev, "unable to map output DMA buffer\n");
+		ret = -ENOMEM;
+		goto out_unmap_in;
+	}
+
+	/*
+	 * A data blob is encrypted using a blob key (BK); a random number.
+	 * The BK is used as an AES-CCM key. The initial block (B0) and the
+	 * initial counter (Ctr0) are generated automatically and stored in
+	 * Class 1 Context DWords 0+1+2+3. The random BK is stored in the
+	 * Class 1 Key Register. Operation Mode is set to AES-CCM.
+	 */
+
+	init_job_desc(desc, 0);
+	append_key_as_imm(desc, info->key_mod, info->key_mod_len,
+			  info->key_mod_len, CLASS_2 | KEY_DEST_CLASS_REG);
+	append_seq_in_ptr_intlen(desc, dma_in, info->input_len, 0);
+	append_seq_out_ptr_intlen(desc, dma_out, output_len, 0);
+	append_operation(desc, op);
+
+	print_hex_dump_debug("data@"__stringify(__LINE__)": ",
+			     DUMP_PREFIX_ADDRESS, 16, 1, info->input,
+			     info->input_len, false);
+	print_hex_dump_debug("jobdesc@"__stringify(__LINE__)": ",
+			     DUMP_PREFIX_ADDRESS, 16, 1, desc,
+			     desc_bytes(desc), false);
+
+	testres.err = 0;
+	init_completion(&testres.completion);
+
+	ret = caam_jr_enqueue(jrdev, desc, caam_blob_job_done, &testres);
+	if (ret == -EINPROGRESS) {
+		wait_for_completion(&testres.completion);
+		ret = testres.err;
+		print_hex_dump_debug("output@"__stringify(__LINE__)": ",
+				     DUMP_PREFIX_ADDRESS, 16, 1, info->output,
+				     output_len, false);
 	}
 
+	if (ret == 0)
+		info->output_len = output_len;
+
+	dma_unmap_single(jrdev, dma_out, output_len, DMA_FROM_DEVICE);
+out_unmap_in:
+	dma_unmap_single(jrdev, dma_in, info->input_len, DMA_TO_DEVICE);
+out_free:
+	kfree(desc);
+
+	return ret;
+}
+EXPORT_SYMBOL(caam_encap_blob);
+
+/** caam_decap_blob - decapsulate blob
+ *
+ * @priv:   instance returned by caam_blob_gen_init
+ * @info:   pointer to blobbing info describing output key,
+ *          input blob and key modifier buffers.
+ *
+ * returns 0 and sets info->output_len on success and returns
+ * a negative error code otherwise.
+ */
+int caam_decap_blob(struct caam_blob_priv *priv,
+		    struct caam_blob_info *info)
+{
+	struct caam_blob_job_result testres;
+	struct device *jrdev = &priv->jrdev;
+	dma_addr_t dma_in, dma_out;
+	int op = OP_PCLID_BLOB;
+	size_t output_len;
+	u32 *desc;
+	int ret;
+
+	if (info->key_mod_len > CAAM_BLOB_KEYMOD_LENGTH)
+		return -EINVAL;
+
+	op |= OP_TYPE_DECAP_PROTOCOL;
+	output_len = info->input_len - CAAM_BLOB_OVERHEAD;
+
 	desc = kzalloc(CAAM_BLOB_DESC_BYTES_MAX, GFP_KERNEL | GFP_DMA);
 	if (!desc)
 		return -ENOMEM;
@@ -145,7 +245,7 @@ int caam_process_blob(struct caam_blob_priv *priv,
 
 	return ret;
 }
-EXPORT_SYMBOL(caam_process_blob);
+EXPORT_SYMBOL(caam_decap_blob);
 
 struct caam_blob_priv *caam_blob_gen_init(void)
 {
diff --git a/include/soc/fsl/caam-blob.h b/include/soc/fsl/caam-blob.h
index 937cac52f36d..de507e2a9555 100644
--- a/include/soc/fsl/caam-blob.h
+++ b/include/soc/fsl/caam-blob.h
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 /*
  * Copyright (C) 2020 Pengutronix, Ahmad Fatoum <kernel@pengutronix.de>
+ * Copyright 2022 NXP, Pankaj Gupta <pankaj.gupta@nxp.com>
  */
 
 #ifndef __CAAM_BLOB_GEN
@@ -72,15 +73,8 @@ int caam_process_blob(struct caam_blob_priv *priv,
  * Return: %0 and sets ``info->output_len`` on success and
  * a negative error code otherwise.
  */
-static inline int caam_encap_blob(struct caam_blob_priv *priv,
-				  struct caam_blob_info *info)
-{
-	if (info->output_len < info->input_len + CAAM_BLOB_OVERHEAD)
-		return -EINVAL;
-
-	return caam_process_blob(priv, info, true);
-}
-
+int caam_encap_blob(struct caam_blob_priv *priv,
+		    struct caam_blob_info *info);
 /**
  * caam_decap_blob - decapsulate blob
  * @priv:   instance returned by caam_blob_gen_init()
@@ -90,14 +84,7 @@ static inline int caam_encap_blob(struct caam_blob_priv *priv,
  * Return: %0 and sets ``info->output_len`` on success and
  * a negative error code otherwise.
  */
-static inline int caam_decap_blob(struct caam_blob_priv *priv,
-				  struct caam_blob_info *info)
-{
-	if (info->input_len < CAAM_BLOB_OVERHEAD ||
-	    info->output_len < info->input_len - CAAM_BLOB_OVERHEAD)
-		return -EINVAL;
-
-	return caam_process_blob(priv, info, false);
-}
+int caam_decap_blob(struct caam_blob_priv *priv,
+		    struct caam_blob_info *info);
 
 #endif
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v0 6/8] KEYS: trusted: caam based black key
  2022-10-06 13:08 [PATCH v0 0/8] Hardware Bound key added to Trusted Key-Ring Pankaj Gupta
                   ` (4 preceding siblings ...)
  2022-10-06 13:08 ` [PATCH v0 5/8] keys-trusted: re-factored caam based trusted key Pankaj Gupta
@ 2022-10-06 13:08 ` Pankaj Gupta
  2022-10-06 12:42   ` Ben Boeckel
  2022-10-06 13:08 ` [PATCH v0 7/8] caam alg: symmetric key ciphers are updated Pankaj Gupta
  2022-10-06 13:08 ` [PATCH v0 8/8] dm-crypt: consumer-app setting the flag-is_hbk Pankaj Gupta
  7 siblings, 1 reply; 31+ messages in thread
From: Pankaj Gupta @ 2022-10-06 13:08 UTC (permalink / raw)
  To: jarkko, a.fatoum, gilad, Jason, jejb, zohar, dhowells,
	sumit.garg, david, michael, john.ernberg, jmorris, serge,
	herbert, davem, j.luebbe, ebiggers, richard, keyrings,
	linux-crypto, linux-integrity, linux-kernel,
	linux-security-module, sahil.malhotra, kshitiz.varshney,
	horia.geanta, V.Sethi
  Cc: Pankaj Gupta

- CAAM supports two types of black keys:
  -- Plain key encrypted with ECB
  -- Plain key encrypted with CCM
  Note: Due to robustness, default encytption used for black key is CCM.

- A black key blob is generated, and added to trusted key payload.
  This is done as part of sealing operation, that was triggered as a result of:
  -- new key generation
  -- load key,

Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
---
 drivers/crypto/caam/blob_gen.c            | 123 +++++++++++++++++++---
 drivers/crypto/caam/desc.h                |   8 +-
 include/soc/fsl/caam-blob.h               |  15 +++
 security/keys/trusted-keys/trusted_caam.c |   8 ++
 4 files changed, 136 insertions(+), 18 deletions(-)

diff --git a/drivers/crypto/caam/blob_gen.c b/drivers/crypto/caam/blob_gen.c
index 36683ec9aee0..93e05557dcaa 100644
--- a/drivers/crypto/caam/blob_gen.c
+++ b/drivers/crypto/caam/blob_gen.c
@@ -8,6 +8,8 @@
 #define pr_fmt(fmt) "caam blob_gen: " fmt
 
 #include <linux/device.h>
+#include <linux/hw_bound_key.h>
+#include <keys/trusted-type.h>
 #include <soc/fsl/caam-blob.h>
 
 #include "compat.h"
@@ -32,6 +34,9 @@
 
 struct caam_blob_priv {
 	struct device jrdev;
+	/* Flags: whether generated trusted key, is ECB or CCM encrypted.*/
+	uint8_t hbk_flags;
+	uint8_t rsv[3];
 };
 
 struct caam_blob_job_result {
@@ -78,8 +83,13 @@ int caam_encap_blob(struct caam_blob_priv *priv,
 	dma_addr_t dma_in, dma_out;
 	int op = OP_PCLID_BLOB;
 	size_t output_len;
+	dma_addr_t dma_blk;
 	u32 *desc;
 	int ret;
+	int hwbk_caam_ovhd = 0;
+
+	if (info->output_len < info->input_len + CAAM_BLOB_OVERHEAD)
+		return -EINVAL;
 
 	if (info->key_mod_len > CAAM_BLOB_KEYMOD_LENGTH)
 		return -EINVAL;
@@ -87,6 +97,21 @@ int caam_encap_blob(struct caam_blob_priv *priv,
 	op |= OP_TYPE_ENCAP_PROTOCOL;
 	output_len = info->input_len + CAAM_BLOB_OVERHEAD;
 
+	if (info->is_hw_bound == 1) {
+		op |= OP_PCL_BLOB_BLACK;
+		if (priv->hbk_flags & HWBK_FLAGS_CAAM_CCM_ALGO_MASK) {
+			op |= OP_PCL_BLOB_EKT;
+			hwbk_caam_ovhd = CCM_OVERHEAD;
+		}
+
+		if ((info->input_len + hwbk_caam_ovhd) > MAX_KEY_SIZE)
+			return -EINVAL;
+
+		set_hbk_info(info->hbk_info,
+			     priv->hbk_flags,
+			     info->input_len);
+	}
+
 	desc = kzalloc(CAAM_BLOB_DESC_BYTES_MAX, GFP_KERNEL | GFP_DMA);
 	if (!desc)
 		return -ENOMEM;
@@ -99,12 +124,26 @@ int caam_encap_blob(struct caam_blob_priv *priv,
 		goto out_free;
 	}
 
+	if (info->is_hw_bound == 1) {
+		dma_blk = dma_map_single(jrdev, info->input,
+					 info->input_len + hwbk_caam_ovhd,
+					 DMA_FROM_DEVICE);
+		if (dma_mapping_error(jrdev, dma_out)) {
+			dev_err(jrdev, "unable to map output DMA buffer\n");
+			ret = -ENOMEM;
+			goto out_unmap_in;
+		}
+	}
+
 	dma_out = dma_map_single(jrdev, info->output, output_len,
 				 DMA_FROM_DEVICE);
 	if (dma_mapping_error(jrdev, dma_out)) {
 		dev_err(jrdev, "unable to map output DMA buffer\n");
 		ret = -ENOMEM;
-		goto out_unmap_in;
+		if (info->is_hw_bound == 1)
+			goto out_unmap_blk;
+		else
+			goto out_unmap_in;
 	}
 
 	/*
@@ -116,15 +155,40 @@ int caam_encap_blob(struct caam_blob_priv *priv,
 	 */
 
 	init_job_desc(desc, 0);
+
+	if (info->is_hw_bound == 1) {
+		/*!1. key command used to load class 1 key register
+		 *    from input plain key.
+		 */
+		append_key(desc, dma_in, info->input_len,
+						CLASS_1 | KEY_DEST_CLASS_REG);
+
+		/*!2. Fifostore to store black key from class 1 key register. */
+		append_fifo_store(desc, dma_blk, info->input_len,
+				  LDST_CLASS_1_CCB | FIFOST_TYPE_KEY_CCM_JKEK);
+
+		append_jump(desc, JUMP_COND_NOP | 1);
+	}
+	/*!3. Load class 2 key with key modifier. */
 	append_key_as_imm(desc, info->key_mod, info->key_mod_len,
 			  info->key_mod_len, CLASS_2 | KEY_DEST_CLASS_REG);
-	append_seq_in_ptr_intlen(desc, dma_in, info->input_len, 0);
+
+	/*!4. SEQ IN PTR Command. */
+	if (info->is_hw_bound == 1) {
+		append_seq_in_ptr_intlen(desc, dma_blk, info->input_len, 0);
+	} else {
+		append_seq_in_ptr_intlen(desc, dma_in, info->input_len, 0);
+	}
+
+	/*!5. SEQ OUT PTR Command. */
 	append_seq_out_ptr_intlen(desc, dma_out, output_len, 0);
+
+	/*!6. BlackBlob encapsulation PROTOCOL Command. */
 	append_operation(desc, op);
 
 	print_hex_dump_debug("data@"__stringify(__LINE__)": ",
 			     DUMP_PREFIX_ADDRESS, 16, 1, info->input,
-			     info->input_len, false);
+			     info->input_len + hwbk_caam_ovhd, false);
 	print_hex_dump_debug("jobdesc@"__stringify(__LINE__)": ",
 			     DUMP_PREFIX_ADDRESS, 16, 1, desc,
 			     desc_bytes(desc), false);
@@ -140,11 +204,15 @@ int caam_encap_blob(struct caam_blob_priv *priv,
 				     DUMP_PREFIX_ADDRESS, 16, 1, info->output,
 				     output_len, false);
 	}
-
-	if (ret == 0)
+	if (ret == 0) {
+		info->input_len += hwbk_caam_ovhd;
 		info->output_len = output_len;
-
+	}
 	dma_unmap_single(jrdev, dma_out, output_len, DMA_FROM_DEVICE);
+out_unmap_blk:
+	if (info->is_hw_bound == 1) {
+		dma_unmap_single(jrdev, dma_blk, info->input_len, DMA_TO_DEVICE);
+	}
 out_unmap_in:
 	dma_unmap_single(jrdev, dma_in, info->input_len, DMA_TO_DEVICE);
 out_free:
@@ -170,15 +238,35 @@ int caam_decap_blob(struct caam_blob_priv *priv,
 	struct device *jrdev = &priv->jrdev;
 	dma_addr_t dma_in, dma_out;
 	int op = OP_PCLID_BLOB;
-	size_t output_len;
 	u32 *desc;
 	int ret;
+	int hwbk_caam_ovhd = 0;
+
+	if (info->input_len < CAAM_BLOB_OVERHEAD)
+		return -EINVAL;
 
 	if (info->key_mod_len > CAAM_BLOB_KEYMOD_LENGTH)
 		return -EINVAL;
 
 	op |= OP_TYPE_DECAP_PROTOCOL;
-	output_len = info->input_len - CAAM_BLOB_OVERHEAD;
+	info->output_len = info->input_len - CAAM_BLOB_OVERHEAD;
+
+	if (info->is_hw_bound == 1) {
+		op |= OP_PCL_BLOB_BLACK;
+		if (priv->hbk_flags & HWBK_FLAGS_CAAM_CCM_ALGO_MASK) {
+			op |= OP_PCL_BLOB_EKT;
+			hwbk_caam_ovhd = CCM_OVERHEAD;
+		}
+
+		if ((info->output_len + hwbk_caam_ovhd) > MAX_KEY_SIZE)
+			return -EINVAL;
+
+		set_hbk_info(info->hbk_info,
+			     priv->hbk_flags,
+			     info->output_len);
+
+		info->output_len += hwbk_caam_ovhd;
+	}
 
 	desc = kzalloc(CAAM_BLOB_DESC_BYTES_MAX, GFP_KERNEL | GFP_DMA);
 	if (!desc)
@@ -192,7 +280,7 @@ int caam_decap_blob(struct caam_blob_priv *priv,
 		goto out_free;
 	}
 
-	dma_out = dma_map_single(jrdev, info->output, output_len,
+	dma_out = dma_map_single(jrdev, info->output, info->output_len,
 				 DMA_FROM_DEVICE);
 	if (dma_mapping_error(jrdev, dma_out)) {
 		dev_err(jrdev, "unable to map output DMA buffer\n");
@@ -211,8 +299,8 @@ int caam_decap_blob(struct caam_blob_priv *priv,
 	init_job_desc(desc, 0);
 	append_key_as_imm(desc, info->key_mod, info->key_mod_len,
 			  info->key_mod_len, CLASS_2 | KEY_DEST_CLASS_REG);
-	append_seq_in_ptr_intlen(desc, dma_in, info->input_len, 0);
-	append_seq_out_ptr_intlen(desc, dma_out, output_len, 0);
+	append_seq_in_ptr(desc, dma_in, info->input_len, 0);
+	append_seq_out_ptr(desc, dma_out, info->output_len, 0);
 	append_operation(desc, op);
 
 	print_hex_dump_debug("data@"__stringify(__LINE__)": ",
@@ -231,13 +319,10 @@ int caam_decap_blob(struct caam_blob_priv *priv,
 		ret = testres.err;
 		print_hex_dump_debug("output@"__stringify(__LINE__)": ",
 				     DUMP_PREFIX_ADDRESS, 16, 1, info->output,
-				     output_len, false);
+				     info->output_len, false);
 	}
 
-	if (ret == 0)
-		info->output_len = output_len;
-
-	dma_unmap_single(jrdev, dma_out, output_len, DMA_FROM_DEVICE);
+	dma_unmap_single(jrdev, dma_out, info->output_len, DMA_FROM_DEVICE);
 out_unmap_in:
 	dma_unmap_single(jrdev, dma_in, info->input_len, DMA_TO_DEVICE);
 out_free:
@@ -251,6 +336,7 @@ struct caam_blob_priv *caam_blob_gen_init(void)
 {
 	struct caam_drv_private *ctrlpriv;
 	struct device *jrdev;
+	struct caam_blob_priv *blob_priv;
 
 	/*
 	 * caam_blob_gen_init() may expectedly fail with -ENODEV, e.g. when
@@ -271,7 +357,10 @@ struct caam_blob_priv *caam_blob_gen_init(void)
 		return ERR_PTR(-ENODEV);
 	}
 
-	return container_of(jrdev, struct caam_blob_priv, jrdev);
+	blob_priv = container_of(jrdev, struct caam_blob_priv, jrdev);
+	blob_priv->hbk_flags = HWBK_FLAGS_CAAM_CCM_ALGO_MASK;
+
+	return blob_priv;
 }
 EXPORT_SYMBOL(caam_blob_gen_init);
 
diff --git a/drivers/crypto/caam/desc.h b/drivers/crypto/caam/desc.h
index e13470901586..41b2d0226bdf 100644
--- a/drivers/crypto/caam/desc.h
+++ b/drivers/crypto/caam/desc.h
@@ -4,7 +4,7 @@
  * Definitions to support CAAM descriptor instruction generation
  *
  * Copyright 2008-2011 Freescale Semiconductor, Inc.
- * Copyright 2018 NXP
+ * Copyright 2018-2022 NXP
  */
 
 #ifndef DESC_H
@@ -403,6 +403,7 @@
 #define FIFOST_TYPE_PKHA_N	 (0x08 << FIFOST_TYPE_SHIFT)
 #define FIFOST_TYPE_PKHA_A	 (0x0c << FIFOST_TYPE_SHIFT)
 #define FIFOST_TYPE_PKHA_B	 (0x0d << FIFOST_TYPE_SHIFT)
+#define FIFOST_TYPE_KEY_CCM_JKEK (0x14 << FIFOST_TYPE_SHIFT)
 #define FIFOST_TYPE_AF_SBOX_JKEK (0x20 << FIFOST_TYPE_SHIFT)
 #define FIFOST_TYPE_AF_SBOX_TKEK (0x21 << FIFOST_TYPE_SHIFT)
 #define FIFOST_TYPE_PKHA_E_JKEK	 (0x22 << FIFOST_TYPE_SHIFT)
@@ -1001,6 +1002,11 @@
 #define OP_PCL_TLS12_AES_256_CBC_SHA384		 0xff63
 #define OP_PCL_TLS12_AES_256_CBC_SHA512		 0xff65
 
+/* Blob protocol protinfo bits */
+
+#define OP_PCL_BLOB_BLACK                        0x0004
+#define OP_PCL_BLOB_EKT                          0x0100
+
 /* For DTLS - OP_PCLID_DTLS */
 
 #define OP_PCL_DTLS_AES_128_CBC_SHA		 0x002f
diff --git a/include/soc/fsl/caam-blob.h b/include/soc/fsl/caam-blob.h
index de507e2a9555..8d9f6b209418 100644
--- a/include/soc/fsl/caam-blob.h
+++ b/include/soc/fsl/caam-blob.h
@@ -9,7 +9,19 @@
 
 #include <linux/types.h>
 #include <linux/errno.h>
+#include <linux/hw_bound_key.h>
 
+#define HWBK_FLAGS_CAAM_CCM_ALGO_MASK   0x01
+
+/*
+ * CCM-Black Key will always be at least 12 bytes longer,
+ * since the encapsulation uses a 6-byte nonce and adds
+ * a 6-byte ICV. But first, the key is padded as necessary so
+ * that CCM-Black Key is a multiple of 8 bytes long.
+ */
+#define NONCE_SIZE 6
+#define ICV_SIZE 6
+#define CCM_OVERHEAD (NONCE_SIZE + ICV_SIZE)
 #define CAAM_BLOB_KEYMOD_LENGTH		16
 #define CAAM_BLOB_OVERHEAD		(32 + 16)
 #define CAAM_BLOB_MAX_LEN		4096
@@ -35,6 +47,9 @@ struct caam_blob_info {
 
 	const void *key_mod;
 	size_t key_mod_len;
+
+	const char is_hw_bound;
+	struct hw_bound_key_info *hbk_info;
 };
 
 /**
diff --git a/security/keys/trusted-keys/trusted_caam.c b/security/keys/trusted-keys/trusted_caam.c
index e3415c520c0a..60e50bed7014 100644
--- a/security/keys/trusted-keys/trusted_caam.c
+++ b/security/keys/trusted-keys/trusted_caam.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
  * Copyright (C) 2021 Pengutronix, Ahmad Fatoum <kernel@pengutronix.de>
+ * Copyright 2022 NXP, Pankaj Gupta <pankaj.gupta@nxp.com>
  */
 
 #include <keys/trusted_caam.h>
@@ -23,6 +24,7 @@ static int trusted_caam_seal(struct trusted_key_payload *p, char *datablob)
 		.input  = p->key,  .input_len   = p->key_len,
 		.output = p->blob, .output_len  = MAX_BLOB_SIZE,
 		.key_mod = KEYMOD, .key_mod_len = sizeof(KEYMOD) - 1,
+		.is_hw_bound = p->is_hw_bound, .hbk_info = &p->hbk_info,
 	};
 
 	ret = caam_encap_blob(blobifier, &info);
@@ -30,6 +32,10 @@ static int trusted_caam_seal(struct trusted_key_payload *p, char *datablob)
 		return ret;
 
 	p->blob_len = info.output_len;
+
+	if (p->is_hw_bound)
+		p->key_len = info.input_len;
+
 	return 0;
 }
 
@@ -40,6 +46,7 @@ static int trusted_caam_unseal(struct trusted_key_payload *p, char *datablob)
 		.input   = p->blob,  .input_len  = p->blob_len,
 		.output  = p->key,   .output_len = MAX_KEY_SIZE,
 		.key_mod = KEYMOD,  .key_mod_len = sizeof(KEYMOD) - 1,
+		.is_hw_bound = p->is_hw_bound, .hbk_info = &p->hbk_info,
 	};
 
 	ret = caam_decap_blob(blobifier, &info);
@@ -47,6 +54,7 @@ static int trusted_caam_unseal(struct trusted_key_payload *p, char *datablob)
 		return ret;
 
 	p->key_len = info.output_len;
+
 	return 0;
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v0 7/8] caam alg: symmetric key ciphers are updated
  2022-10-06 13:08 [PATCH v0 0/8] Hardware Bound key added to Trusted Key-Ring Pankaj Gupta
                   ` (5 preceding siblings ...)
  2022-10-06 13:08 ` [PATCH v0 6/8] KEYS: trusted: caam based black key Pankaj Gupta
@ 2022-10-06 13:08 ` Pankaj Gupta
  2022-10-12  9:01   ` Jarkko Sakkinen
  2022-10-06 13:08 ` [PATCH v0 8/8] dm-crypt: consumer-app setting the flag-is_hbk Pankaj Gupta
  7 siblings, 1 reply; 31+ messages in thread
From: Pankaj Gupta @ 2022-10-06 13:08 UTC (permalink / raw)
  To: jarkko, a.fatoum, gilad, Jason, jejb, zohar, dhowells,
	sumit.garg, david, michael, john.ernberg, jmorris, serge,
	herbert, davem, j.luebbe, ebiggers, richard, keyrings,
	linux-crypto, linux-integrity, linux-kernel,
	linux-security-module, sahil.malhotra, kshitiz.varshney,
	horia.geanta, V.Sethi
  Cc: Pankaj Gupta

Changes to enable:
- To work both with black key and plain key.
- It is supported in context of trusted key only.
  - as meta-data is added as part of trusted key generation.
  - otherwise, work as previously.

Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
---
 drivers/crypto/caam/caamalg.c      | 43 ++++++++++++++++++++++++++++--
 drivers/crypto/caam/caamalg_desc.c |  8 +++---
 drivers/crypto/caam/desc_constr.h  |  6 ++++-
 3 files changed, 51 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
index d3d8bb0a6990..94e971297a9d 100644
--- a/drivers/crypto/caam/caamalg.c
+++ b/drivers/crypto/caam/caamalg.c
@@ -3,7 +3,7 @@
  * caam - Freescale FSL CAAM support for crypto API
  *
  * Copyright 2008-2011 Freescale Semiconductor, Inc.
- * Copyright 2016-2019 NXP
+ * Copyright 2016-2022 NXP
  *
  * Based on talitos crypto API driver.
  *
@@ -59,6 +59,8 @@
 #include <crypto/engine.h>
 #include <crypto/xts.h>
 #include <asm/unaligned.h>
+#include <linux/hw_bound_key.h>
+#include <soc/fsl/caam-blob.h>
 
 /*
  * crypto alg
@@ -741,9 +743,25 @@ static int skcipher_setkey(struct crypto_skcipher *skcipher, const u8 *key,
 	print_hex_dump_debug("key in @"__stringify(__LINE__)": ",
 			     DUMP_PREFIX_ADDRESS, 16, 4, key, keylen, 1);
 
+	/* Here keylen is actual key length */
 	ctx->cdata.keylen = keylen;
 	ctx->cdata.key_virt = key;
 	ctx->cdata.key_inline = true;
+	/* Here real key len is plain key length */
+	ctx->cdata.key_real_len = keylen;
+	ctx->cdata.key_cmd_opt = 0;
+
+	/* check if the key is HBK */
+	if (skcipher->base.is_hbk) {
+		ctx->cdata.key_cmd_opt |= KEY_ENC;
+
+		/* check if the HBK is CCM key */
+		if (skcipher->base.hbk_info.flags
+				& HWBK_FLAGS_CAAM_CCM_ALGO_MASK)
+			ctx->cdata.key_cmd_opt |= KEY_EKT;
+
+		ctx->cdata.key_real_len = skcipher->base.hbk_info.key_sz;
+	}
 
 	/* skcipher_encrypt shared descriptor */
 	desc = ctx->sh_desc_enc;
@@ -762,12 +780,33 @@ static int skcipher_setkey(struct crypto_skcipher *skcipher, const u8 *key,
 	return 0;
 }
 
+static int caam_hbk_check_keylen(struct hw_bound_key_info *hbk_info,
+				 unsigned int keylen)
+{
+	u32 overhead = 0;
+
+	if (hbk_info->flags & HWBK_FLAGS_CAAM_CCM_ALGO_MASK)
+		overhead += CCM_OVERHEAD;
+
+	/* deduce the hb_key_len, by adding plain-key len
+	 * and encryption overhead.
+	 */
+	if (keylen != (hbk_info->key_sz + overhead))
+		return -EINVAL;
+
+	return aes_check_keylen(hbk_info->key_sz);
+}
+
 static int aes_skcipher_setkey(struct crypto_skcipher *skcipher,
 			       const u8 *key, unsigned int keylen)
 {
 	int err;
 
-	err = aes_check_keylen(keylen);
+	if (skcipher->base.is_hbk)
+		err = caam_hbk_check_keylen(&(skcipher->base.hbk_info), keylen);
+	else
+		err = aes_check_keylen(keylen);
+
 	if (err)
 		return err;
 
diff --git a/drivers/crypto/caam/caamalg_desc.c b/drivers/crypto/caam/caamalg_desc.c
index 7571e1ac913b..784acae8c9b7 100644
--- a/drivers/crypto/caam/caamalg_desc.c
+++ b/drivers/crypto/caam/caamalg_desc.c
@@ -2,7 +2,7 @@
 /*
  * Shared descriptors for aead, skcipher algorithms
  *
- * Copyright 2016-2019 NXP
+ * Copyright 2016-2022 NXP
  */
 
 #include "compat.h"
@@ -1391,7 +1391,8 @@ void cnstr_shdsc_skcipher_encap(u32 * const desc, struct alginfo *cdata,
 
 	/* Load class1 key only */
 	append_key_as_imm(desc, cdata->key_virt, cdata->keylen,
-			  cdata->keylen, CLASS_1 | KEY_DEST_CLASS_REG);
+			  cdata->key_real_len, CLASS_1 | KEY_DEST_CLASS_REG
+					       | cdata->key_cmd_opt);
 
 	/* Load nonce into CONTEXT1 reg */
 	if (is_rfc3686) {
@@ -1466,7 +1467,8 @@ void cnstr_shdsc_skcipher_decap(u32 * const desc, struct alginfo *cdata,
 
 	/* Load class1 key only */
 	append_key_as_imm(desc, cdata->key_virt, cdata->keylen,
-			  cdata->keylen, CLASS_1 | KEY_DEST_CLASS_REG);
+			  cdata->key_real_len, CLASS_1 | KEY_DEST_CLASS_REG
+					       | cdata->key_cmd_opt);
 
 	/* Load nonce into CONTEXT1 reg */
 	if (is_rfc3686) {
diff --git a/drivers/crypto/caam/desc_constr.h b/drivers/crypto/caam/desc_constr.h
index 62ce6421bb3f..d652bdbf3f91 100644
--- a/drivers/crypto/caam/desc_constr.h
+++ b/drivers/crypto/caam/desc_constr.h
@@ -3,7 +3,7 @@
  * caam descriptor construction helper functions
  *
  * Copyright 2008-2012 Freescale Semiconductor, Inc.
- * Copyright 2019 NXP
+ * Copyright 2019-2022 NXP
  */
 
 #ifndef DESC_CONSTR_H
@@ -500,6 +500,8 @@ do { \
  * @key_virt: virtual address where algorithm key resides
  * @key_inline: true - key can be inlined in the descriptor; false - key is
  *              referenced by the descriptor
+ * @key_real_len: size of the key to be loaded by the CAAM
+ * @key_cmd_opt: optional parameters for KEY command
  */
 struct alginfo {
 	u32 algtype;
@@ -508,6 +510,8 @@ struct alginfo {
 	dma_addr_t key_dma;
 	const void *key_virt;
 	bool key_inline;
+	u32 key_real_len;
+	u32 key_cmd_opt;
 };
 
 /**
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* [PATCH v0 8/8] dm-crypt: consumer-app setting the flag-is_hbk
  2022-10-06 13:08 [PATCH v0 0/8] Hardware Bound key added to Trusted Key-Ring Pankaj Gupta
                   ` (6 preceding siblings ...)
  2022-10-06 13:08 ` [PATCH v0 7/8] caam alg: symmetric key ciphers are updated Pankaj Gupta
@ 2022-10-06 13:08 ` Pankaj Gupta
  7 siblings, 0 replies; 31+ messages in thread
From: Pankaj Gupta @ 2022-10-06 13:08 UTC (permalink / raw)
  To: jarkko, a.fatoum, gilad, Jason, jejb, zohar, dhowells,
	sumit.garg, david, michael, john.ernberg, jmorris, serge,
	herbert, davem, j.luebbe, ebiggers, richard, keyrings,
	linux-crypto, linux-integrity, linux-kernel,
	linux-security-module, sahil.malhotra, kshitiz.varshney,
	horia.geanta, V.Sethi
  Cc: Pankaj Gupta

Consumer application:
- Adding a flag 'is_hbk', in its "struct crypto_config".

- After fetching the keys, it is setting the above
  mentioned flag, based on the key fetched.
  -- Note: Supported for trusted keys only.

- After allocating the tfm, and before calling
  crypto_xxx_setkey(), setting the:
  -- tfm flag 'is_hbk':
          cc->cipher_tfm.tfms[i]->base.is_hbk = cc->is_hbk;
  -- tfm hbk_info, if cc->is_hbk, is non-zero.

  Note: HBK Supported for symmetric-key ciphers only.

Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
---
 drivers/md/dm-crypt.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 159c6806c19b..d28c4af2904e 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -221,6 +221,8 @@ struct crypt_config {
 	struct mutex bio_alloc_lock;
 
 	u8 *authenc_key; /* space for keys in authenc() format (if used) */
+	unsigned int is_hbk;
+	struct hw_bound_key_info hbk_info;
 	u8 key[];
 };
 
@@ -2397,10 +2399,16 @@ static int crypt_setkey(struct crypt_config *cc)
 			r = crypto_aead_setkey(cc->cipher_tfm.tfms_aead[i],
 					       cc->key + (i * subkey_size),
 					       subkey_size);
-		else
+		else {
+			cc->cipher_tfm.tfms[i]->base.is_hbk = cc->is_hbk;
+			if (cc->is_hbk)
+				memcpy(&(cc->cipher_tfm.tfms[i]->base.hbk_info),
+				       &(cc->hbk_info),
+				       sizeof(struct hw_bound_key_info));
 			r = crypto_skcipher_setkey(cc->cipher_tfm.tfms[i],
 						   cc->key + (i * subkey_size),
 						   subkey_size);
+		}
 		if (r)
 			err = r;
 	}
@@ -2461,9 +2469,11 @@ static int set_key_trusted(struct crypt_config *cc, struct key *key)
 	if (!tkp)
 		return -EKEYREVOKED;
 
+	cc->is_hbk = tkp->is_hw_bound;
 	if (cc->key_size != tkp->key_len)
 		return -EINVAL;
 
+	memcpy(&(cc->hbk_info), &(tkp->hbk_info), sizeof(struct hw_bound_key_info));
 	memcpy(cc->key, tkp->key, cc->key_size);
 
 	return 0;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 31+ messages in thread

* Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
  2022-10-06 13:08 ` [PATCH v0 3/8] crypto: hbk flags & info added to the tfm Pankaj Gupta
@ 2022-10-07  6:58   ` Herbert Xu
  2022-10-10 11:15     ` [EXT] " Pankaj Gupta
  2022-10-12  8:57   ` Jarkko Sakkinen
  1 sibling, 1 reply; 31+ messages in thread
From: Herbert Xu @ 2022-10-07  6:58 UTC (permalink / raw)
  To: Pankaj Gupta
  Cc: jarkko, a.fatoum, gilad, Jason, jejb, zohar, dhowells,
	sumit.garg, david, michael, john.ernberg, jmorris, serge, davem,
	j.luebbe, ebiggers, richard, keyrings, linux-crypto,
	linux-integrity, linux-kernel, linux-security-module,
	sahil.malhotra, kshitiz.varshney, horia.geanta, V.Sethi

On Thu, Oct 06, 2022 at 06:38:32PM +0530, Pankaj Gupta wrote:
> Consumer of the kernel crypto api, after allocating
> the transformation (tfm), sets the:
> - flag 'is_hbk'
> - structure 'struct hw_bound_key_info hbk_info'
> based on the type of key, the consumer is using.
> 
> This helps:
> 
> - This helps to influence the core processing logic
>   for the encapsulated algorithm.
> - This flag is set by the consumer after allocating
>   the tfm and before calling the function crypto_xxx_setkey().
> 
> Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
> ---
>  include/linux/crypto.h | 5 +++++
>  1 file changed, 5 insertions(+)

Nack.  You still have not provided a convincing argument why
this is necessary since there are plenty of existing drivers in
the kernel already providing similar features.

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
  2022-10-07  6:58   ` Herbert Xu
@ 2022-10-10 11:15     ` Pankaj Gupta
  2022-10-10 15:15       ` Jason A. Donenfeld
  0 siblings, 1 reply; 31+ messages in thread
From: Pankaj Gupta @ 2022-10-10 11:15 UTC (permalink / raw)
  To: 'Herbert Xu'
  Cc: jarkko, a.fatoum, gilad, Jason, jejb, zohar, dhowells,
	sumit.garg, david, michael, john.ernberg, jmorris, serge, davem,
	j.luebbe, ebiggers, richard, keyrings, linux-crypto,
	linux-integrity, linux-kernel, linux-security-module,
	Sahil Malhotra, Kshitiz Varshney, Horia Geanta, Varun Sethi



> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: Friday, October 7, 2022 12:28 PM
> To: Pankaj Gupta <pankaj.gupta@nxp.com>
> Cc: jarkko@kernel.org; a.fatoum@pengutronix.de; gilad@benyossef.com;
> Jason@zx2c4.com; jejb@linux.ibm.com; zohar@linux.ibm.com;
> dhowells@redhat.com; sumit.garg@linaro.org; david@sigma-star.at;
> michael@walle.cc; john.ernberg@actia.se; jmorris@namei.org;
> serge@hallyn.com; davem@davemloft.net; j.luebbe@pengutronix.de;
> ebiggers@kernel.org; richard@nod.at; keyrings@vger.kernel.org; linux-
> crypto@vger.kernel.org; linux-integrity@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-security-module@vger.kernel.org; Sahil Malhotra
> <sahil.malhotra@nxp.com>; Kshitiz Varshney <kshitiz.varshney@nxp.com>;
> Horia Geanta <horia.geanta@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> Subject: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
> 
> Caution: EXT Email
> 
> On Thu, Oct 06, 2022 at 06:38:32PM +0530, Pankaj Gupta wrote:
> > Consumer of the kernel crypto api, after allocating the transformation
> > (tfm), sets the:
> > - flag 'is_hbk'
> > - structure 'struct hw_bound_key_info hbk_info'
> > based on the type of key, the consumer is using.
> >
> > This helps:
> >
> > - This helps to influence the core processing logic
> >   for the encapsulated algorithm.
> > - This flag is set by the consumer after allocating
> >   the tfm and before calling the function crypto_xxx_setkey().
> >
> > Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
> > ---
> >  include/linux/crypto.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> 
> Nack.  You still have not provided a convincing argument why this is necessary
> since there are plenty of existing drivers in the kernel already providing similar
> features.
> 
CAAM is used as a trusted source for trusted keyring. CAAM can expose these keys either as plain key or HBK(hardware bound key- managed by the hardware only and never visible in plain outside of hardware).

Thus, Keys that are inside CAAM-backed-trusted-keyring, can either be plain key or HBK. So the trusted-key-payload requires additional flag & info(key-encryption-protocol)  to help differentiate it from each other. Now when CAAM trusted-key is presented to the kernel crypto framework, the additional information associated with the key, needs to be passed to the hardware driver. Currently the kernel keyring and kernel crypto frameworks are associated for plain key, but completely dis-associated for HBK. This patch addresses this problem.

Similar capabilities (trusted source), are there in other crypto accelerators on NXP SoC(s). Having hardware specific crypto algorithm name, does not seems to be a scalable solution.

> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgondor.ap
> ana.org.au%2F~herbert%2F&amp;data=05%7C01%7Cpankaj.gupta%40nxp.com
> %7C33055110772a4d4bb97508daa8317e93%7C686ea1d3bc2b4c6fa92cd99c5c3
> 01635%7C0%7C0%7C638007227793511347%7CUnknown%7CTWFpbGZsb3d8ey
> JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> 3000%7C%7C%7C&amp;sdata=H0fzzxQhsV%2FyPphBAHBDmzQfTFnjDE7jYstTM
> ok%2F09I%3D&amp;reserved=0
> PGP Key:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgondor.ap
> ana.org.au%2F~herbert%2Fpubkey.txt&amp;data=05%7C01%7Cpankaj.gupta%4
> 0nxp.com%7C33055110772a4d4bb97508daa8317e93%7C686ea1d3bc2b4c6fa9
> 2cd99c5c301635%7C0%7C0%7C638007227793667554%7CUnknown%7CTWFpb
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6M
> n0%3D%7C3000%7C%7C%7C&amp;sdata=SclJ9G7jBWhOW%2Fm3Gt0jP1oicqVp
> 5ghH%2BDT8Vd5maag%3D&amp;reserved=0

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
  2022-10-10 11:15     ` [EXT] " Pankaj Gupta
@ 2022-10-10 15:15       ` Jason A. Donenfeld
  2022-10-10 21:35         ` [EXT] " David Gstir
                           ` (2 more replies)
  0 siblings, 3 replies; 31+ messages in thread
From: Jason A. Donenfeld @ 2022-10-10 15:15 UTC (permalink / raw)
  To: Pankaj Gupta
  Cc: 'Herbert Xu',
	jarkko, a.fatoum, gilad, jejb, zohar, dhowells, sumit.garg,
	david, michael, john.ernberg, jmorris, serge, davem, j.luebbe,
	ebiggers, richard, keyrings, linux-crypto, linux-integrity,
	linux-kernel, linux-security-module, Sahil Malhotra,
	Kshitiz Varshney, Horia Geanta, Varun Sethi

On Mon, Oct 10, 2022 at 11:15:00AM +0000, Pankaj Gupta wrote:
> > Nack.  You still have not provided a convincing argument why this is necessary
> > since there are plenty of existing drivers in the kernel already providing similar
> > features.
> > 
> CAAM is used as a trusted source for trusted keyring. CAAM can expose
> these keys either as plain key or HBK(hardware bound key- managed by
> the hardware only and never visible in plain outside of hardware).
> 
> Thus, Keys that are inside CAAM-backed-trusted-keyring, can either be
> plain key or HBK. So the trusted-key-payload requires additional flag
> & info(key-encryption-protocol)  to help differentiate it from each
> other. Now when CAAM trusted-key is presented to the kernel crypto
> framework, the additional information associated with the key, needs
> to be passed to the hardware driver. Currently the kernel keyring and
> kernel crypto frameworks are associated for plain key, but completely
> dis-associated for HBK. This patch addresses this problem.
> 
> Similar capabilities (trusted source), are there in other crypto
> accelerators on NXP SoC(s). Having hardware specific crypto algorithm
> name, does not seems to be a scalable solution.

Do you mean to say that other drivers that use hardware-backed keys do
so by setting "cra_name" to something particular? Like instead of "aes"
it'd be "aes-but-special-for-this-driver"? If so, that would seem to
break the design of the crypto API. Which driver did you see that does
this? Or perhaps, more generally, what are the drivers that Herbert is
talking about when he mentions the "plenty of existing drivers" that
already do this?

Jason

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [EXT] [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
  2022-10-10 15:15       ` Jason A. Donenfeld
@ 2022-10-10 21:35         ` David Gstir
  2022-10-11  9:03         ` [EXT] " Herbert Xu
  2022-10-11 11:05         ` Pankaj Gupta
  2 siblings, 0 replies; 31+ messages in thread
From: David Gstir @ 2022-10-10 21:35 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Pankaj Gupta, Herbert Xu, Jarkko Sakkinen, Ahmad Fatoum, gilad,
	James Bottomley, Mimi Zohar, David Howells, Sumit Garg, michael,
	john.ernberg, James Morris, Serge E. Hallyn, David S. Miller,
	Jan Luebbe, Eric Biggers, Richard Weinberger, keyrings,
	linux-crypto, linux-integrity, linux-kernel,
	linux-security-module, Sahil Malhotra, Kshitiz Varshney,
	Horia Geantă,
	Varun Sethi



> On 10.10.2022, at 17:15, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> 
> On Mon, Oct 10, 2022 at 11:15:00AM +0000, Pankaj Gupta wrote:
>>> Nack.  You still have not provided a convincing argument why this is necessary
>>> since there are plenty of existing drivers in the kernel already providing similar
>>> features.
>>> 
>> CAAM is used as a trusted source for trusted keyring. CAAM can expose
>> these keys either as plain key or HBK(hardware bound key- managed by
>> the hardware only and never visible in plain outside of hardware).
>> 
>> Thus, Keys that are inside CAAM-backed-trusted-keyring, can either be
>> plain key or HBK. So the trusted-key-payload requires additional flag
>> & info(key-encryption-protocol)  to help differentiate it from each
>> other. Now when CAAM trusted-key is presented to the kernel crypto
>> framework, the additional information associated with the key, needs
>> to be passed to the hardware driver. Currently the kernel keyring and
>> kernel crypto frameworks are associated for plain key, but completely
>> dis-associated for HBK. This patch addresses this problem.
>> 
>> Similar capabilities (trusted source), are there in other crypto
>> accelerators on NXP SoC(s). Having hardware specific crypto algorithm
>> name, does not seems to be a scalable solution.
> 
> Do you mean to say that other drivers that use hardware-backed keys do
> so by setting "cra_name" to something particular? Like instead of "aes"
> it'd be "aes-but-special-for-this-driver"? If so, that would seem to
> break the design of the crypto API. Which driver did you see that does
> this? Or perhaps, more generally, what are the drivers that Herbert is
> talking about when he mentions the "plenty of existing drivers" that
> already do this?

I believe what Herbert means are drivers registered with the cipher name 
prefix “p”. E.g. [1] registers multiple “paes” variants. There was a
previous patch set for CAAM where this was suggested as well [2].

- David

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/crypto/ccree/cc_cipher.c#n1011
[2] https://lore.kernel.org/linux-crypto/20200716073610.GA28215@gondor.apana.org.au/

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
  2022-10-10 15:15       ` Jason A. Donenfeld
  2022-10-10 21:35         ` [EXT] " David Gstir
@ 2022-10-11  9:03         ` Herbert Xu
  2022-10-11 11:32           ` Pankaj Gupta
  2022-10-11 20:01           ` Jason A. Donenfeld
  2022-10-11 11:05         ` Pankaj Gupta
  2 siblings, 2 replies; 31+ messages in thread
From: Herbert Xu @ 2022-10-11  9:03 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Pankaj Gupta, jarkko, a.fatoum, gilad, jejb, zohar, dhowells,
	sumit.garg, david, michael, john.ernberg, jmorris, serge, davem,
	j.luebbe, ebiggers, richard, keyrings, linux-crypto,
	linux-integrity, linux-kernel, linux-security-module,
	Sahil Malhotra, Kshitiz Varshney, Horia Geanta, Varun Sethi

On Mon, Oct 10, 2022 at 09:15:48AM -0600, Jason A. Donenfeld wrote:
>
> Do you mean to say that other drivers that use hardware-backed keys do
> so by setting "cra_name" to something particular? Like instead of "aes"
> it'd be "aes-but-special-for-this-driver"? If so, that would seem to
> break the design of the crypto API. Which driver did you see that does
> this? Or perhaps, more generally, what are the drivers that Herbert is
> talking about when he mentions the "plenty of existing drivers" that
> already do this?

Grep for paes for the existing drivers that support this.  I don't
have anything against this feature per se, but the last thing we
want is a proliferation of different ways of doing the same thing.

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
  2022-10-10 15:15       ` Jason A. Donenfeld
  2022-10-10 21:35         ` [EXT] " David Gstir
  2022-10-11  9:03         ` [EXT] " Herbert Xu
@ 2022-10-11 11:05         ` Pankaj Gupta
  2 siblings, 0 replies; 31+ messages in thread
From: Pankaj Gupta @ 2022-10-11 11:05 UTC (permalink / raw)
  To: 'Jason A. Donenfeld'
  Cc: 'Herbert Xu',
	jarkko, a.fatoum, gilad, jejb, zohar, dhowells, sumit.garg,
	david, michael, john.ernberg, jmorris, serge, davem, j.luebbe,
	ebiggers, richard, keyrings, linux-crypto, linux-integrity,
	linux-kernel, linux-security-module, Sahil Malhotra,
	Kshitiz Varshney, Horia Geanta, Varun Sethi

> -----Original Message-----
> From: Jason A. Donenfeld <Jason@zx2c4.com>
> Sent: Monday, October 10, 2022 8:46 PM
> To: Pankaj Gupta <pankaj.gupta@nxp.com>
> Cc: 'Herbert Xu' <herbert@gondor.apana.org.au>; jarkko@kernel.org;
> a.fatoum@pengutronix.de; gilad@benyossef.com; jejb@linux.ibm.com;
> zohar@linux.ibm.com; dhowells@redhat.com; sumit.garg@linaro.org;
> david@sigma-star.at; michael@walle.cc; john.ernberg@actia.se;
> jmorris@namei.org; serge@hallyn.com; davem@davemloft.net;
> j.luebbe@pengutronix.de; ebiggers@kernel.org; richard@nod.at;
> keyrings@vger.kernel.org; linux-crypto@vger.kernel.org; linux-
> integrity@vger.kernel.org; linux-kernel@vger.kernel.org; linux-security-
> module@vger.kernel.org; Sahil Malhotra <sahil.malhotra@nxp.com>; Kshitiz
> Varshney <kshitiz.varshney@nxp.com>; Horia Geanta
> <horia.geanta@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> Subject: Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the
> tfm
> 
> Caution: EXT Email
> 
> On Mon, Oct 10, 2022 at 11:15:00AM +0000, Pankaj Gupta wrote:
> > > Nack.  You still have not provided a convincing argument why this is
> > > necessary since there are plenty of existing drivers in the kernel
> > > already providing similar features.
> > >
> > CAAM is used as a trusted source for trusted keyring. CAAM can expose
> > these keys either as plain key or HBK(hardware bound key- managed by
> > the hardware only and never visible in plain outside of hardware).
> >
> > Thus, Keys that are inside CAAM-backed-trusted-keyring, can either be
> > plain key or HBK. So the trusted-key-payload requires additional flag
> > & info(key-encryption-protocol)  to help differentiate it from each
> > other. Now when CAAM trusted-key is presented to the kernel crypto
> > framework, the additional information associated with the key, needs
> > to be passed to the hardware driver. Currently the kernel keyring and
> > kernel crypto frameworks are associated for plain key, but completely
> > dis-associated for HBK. This patch addresses this problem.
> >
> > Similar capabilities (trusted source), are there in other crypto
> > accelerators on NXP SoC(s). Having hardware specific crypto algorithm
> > name, does not seems to be a scalable solution.
> 
> Do you mean to say that other drivers that use hardware-backed keys do so
> by setting "cra_name" to something particular? 

Yes.

> Like instead of "aes"
> it'd be "aes-but-special-for-this-driver"?

For example: ARM-Crypto-Cell prepends 'p':
- xts(paes) for xts(aes)
- xts(paes) for xts(aes)...etc.

 > If so, that would seem to break the
> design of the crypto API. Which driver did you see that does this?  Or perhaps,
> more generally, what are the drivers that Herbert is talking about when he
> mentions the "plenty of existing drivers" that already do this?
I could find this driver " drivers/crypto/ccree/".
Reference file name is " drivers/crypto/ccree/cc_cipher.c"

Likewise, CAAM being a trust source, new cra_name would be need to deal with CAAM generated HBKs too.
We need to come up with something unique like: for eg:   p(xts(aes)) for xts(aes)             
   
Another trust source from NXP called DCP(drivers/crypto/mxs-dcp.c),  new cra_name would be needed for that too.
There are work in progress for other trust sources from NXP.

So, our approach is too provide generic solution, which can be extended to any trust source generating HBK.


> 
> Jason

^ permalink raw reply	[flat|nested] 31+ messages in thread

* RE: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
  2022-10-11  9:03         ` [EXT] " Herbert Xu
@ 2022-10-11 11:32           ` Pankaj Gupta
  2022-10-11 20:01           ` Jason A. Donenfeld
  1 sibling, 0 replies; 31+ messages in thread
From: Pankaj Gupta @ 2022-10-11 11:32 UTC (permalink / raw)
  To: 'Herbert Xu', Jason A. Donenfeld
  Cc: jarkko, a.fatoum, gilad, jejb, zohar, dhowells, sumit.garg,
	david, michael, john.ernberg, jmorris, serge, davem, j.luebbe,
	ebiggers, richard, keyrings, linux-crypto, linux-integrity,
	linux-kernel, linux-security-module, Sahil Malhotra,
	Kshitiz Varshney, Horia Geanta, Varun Sethi



> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: Tuesday, October 11, 2022 2:34 PM
> To: Jason A. Donenfeld <Jason@zx2c4.com>
> Cc: Pankaj Gupta <pankaj.gupta@nxp.com>; jarkko@kernel.org;
> a.fatoum@pengutronix.de; gilad@benyossef.com; jejb@linux.ibm.com;
> zohar@linux.ibm.com; dhowells@redhat.com; sumit.garg@linaro.org;
> david@sigma-star.at; michael@walle.cc; john.ernberg@actia.se;
> jmorris@namei.org; serge@hallyn.com; davem@davemloft.net;
> j.luebbe@pengutronix.de; ebiggers@kernel.org; richard@nod.at;
> keyrings@vger.kernel.org; linux-crypto@vger.kernel.org; linux-
> integrity@vger.kernel.org; linux-kernel@vger.kernel.org; linux-security-
> module@vger.kernel.org; Sahil Malhotra <sahil.malhotra@nxp.com>; Kshitiz
> Varshney <kshitiz.varshney@nxp.com>; Horia Geanta
> <horia.geanta@nxp.com>; Varun Sethi <V.Sethi@nxp.com>
> Subject: Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the
> tfm
> 
> Caution: EXT Email
> 
> On Mon, Oct 10, 2022 at 09:15:48AM -0600, Jason A. Donenfeld wrote:
> >
> > Do you mean to say that other drivers that use hardware-backed keys do
> > so by setting "cra_name" to something particular? Like instead of "aes"
> > it'd be "aes-but-special-for-this-driver"? If so, that would seem to
> > break the design of the crypto API. Which driver did you see that does
> > this? Or perhaps, more generally, what are the drivers that Herbert is
> > talking about when he mentions the "plenty of existing drivers" that
> > already do this?
> 
> Grep for paes for the existing drivers that support this.  I don't have anything
> against this feature per se, but the last thing we want is a proliferation of
> different ways of doing the same thing.

Our goal is to have a generic solution, which can be extended to any driver dealing with:
- Generating HBK and adding to trusted keyring.
- Using the trusted keyring's HBK for crypto operation.

With this framework in place, driver specific custom changes can be avoided, bridging the interface-gap of:
kernel-keyring <-> kernel-crypto-layer.

Thanks.
> 
> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au> Home Page:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgondo
> r.apana.org.au%2F~herbert%2F&amp;data=05%7C01%7Cpankaj.gupta%40nx
> p.com%7C4ef27fc922d04350ca9f08daab67a1a3%7C686ea1d3bc2b4c6fa92cd9
> 9c5c301635%7C0%7C0%7C638010758832054902%7CUnknown%7CTWFpbGZs
> b3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> 0%3D%7C3000%7C%7C%7C&amp;sdata=SOguJ9LGhSCDmspbjDIEzkQLk9Bz%
> 2FsS0B%2BLNc4gzRo8%3D&amp;reserved=0
> PGP Key:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgondo
> r.apana.org.au%2F~herbert%2Fpubkey.txt&amp;data=05%7C01%7Cpankaj.g
> upta%40nxp.com%7C4ef27fc922d04350ca9f08daab67a1a3%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C0%7C638010758832054902%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> CJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=hCzT2fPfJ%2BBNVqN6JR
> wMx9zNJkqvdRSLrR68ubhCvN4%3D&amp;reserved=0

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
  2022-10-11  9:03         ` [EXT] " Herbert Xu
  2022-10-11 11:32           ` Pankaj Gupta
@ 2022-10-11 20:01           ` Jason A. Donenfeld
  2022-10-12  9:06             ` Herbert Xu
  1 sibling, 1 reply; 31+ messages in thread
From: Jason A. Donenfeld @ 2022-10-11 20:01 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Pankaj Gupta, jarkko, a.fatoum, gilad, jejb, zohar, dhowells,
	sumit.garg, david, michael, john.ernberg, jmorris, serge, davem,
	j.luebbe, ebiggers, richard, keyrings, linux-crypto,
	linux-integrity, linux-kernel, linux-security-module,
	Sahil Malhotra, Kshitiz Varshney, Horia Geanta, Varun Sethi

On Tue, Oct 11, 2022 at 05:03:31PM +0800, Herbert Xu wrote:
> On Mon, Oct 10, 2022 at 09:15:48AM -0600, Jason A. Donenfeld wrote:
> >
> > Do you mean to say that other drivers that use hardware-backed keys do
> > so by setting "cra_name" to something particular? Like instead of "aes"
> > it'd be "aes-but-special-for-this-driver"? If so, that would seem to
> > break the design of the crypto API. Which driver did you see that does
> > this? Or perhaps, more generally, what are the drivers that Herbert is
> > talking about when he mentions the "plenty of existing drivers" that
> > already do this?
> 
> Grep for paes for the existing drivers that support this.  I don't
> have anything against this feature per se, but the last thing we
> want is a proliferation of different ways of doing the same thing.

I've got no stake in this, but isn't the whole idea that if you specify
"aes" you get AES, and if you specify "cbc(aes)" you get AES-CBC, and so
forth? And so leaking implementation details into the algorithm name
feels like it breaks the abstraction a bit.

Rather, drivers that do AES should be called "aes". For this hardware
key situation, I guess that means keys have a type (in-memory vs
hardware-resident). Then, a crypto operation takes an "algorithm" and a
"key", and the abstraction then picks the best implementation that's
compatible with both the "algorithm" and the "key".

I haven't looked carefully, but I assume that's more or less what this
patchset does.

If you don't want a proliferation of different ways of doing the same
thing, maybe the requirement should be that the author of this series
also converts the existing "paes" kludge to use the new thing he's
proposing?

Or maybe the "paes" kludge is better for other reasons? I don't know
really. Just my 2¢, but feel free to disregard, as I really have nothing
to do with this change.

Jason

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v0 1/8] hw-bound-key: introducing the generic structure
  2022-10-06 13:08 ` [PATCH v0 1/8] hw-bound-key: introducing the generic structure Pankaj Gupta
@ 2022-10-12  8:52   ` Jarkko Sakkinen
  2022-10-12  8:53   ` Jarkko Sakkinen
  1 sibling, 0 replies; 31+ messages in thread
From: Jarkko Sakkinen @ 2022-10-12  8:52 UTC (permalink / raw)
  To: Pankaj Gupta
  Cc: a.fatoum, gilad, Jason, jejb, zohar, dhowells, sumit.garg, david,
	michael, john.ernberg, jmorris, serge, herbert, davem, j.luebbe,
	ebiggers, richard, keyrings, linux-crypto, linux-integrity,
	linux-kernel, linux-security-module, sahil.malhotra,
	kshitiz.varshney, horia.geanta, V.Sethi

On Thu, Oct 06, 2022 at 06:38:30PM +0530, Pankaj Gupta wrote:
> Hardware bound keys buffer has additional information,
> that will be accessed using this new structure.

I don't really understand what I should get from this.

It lacks motivation and function of this structure, even
the name of the structure.

Hardware bound key does not mean anything at all without
a context. I don't know what it is.

> 
> structure members are:
> - flags, flags for hardware specific information.
> - key_sz, size of the plain key.

Who cares listing member names?

> 
> Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
> ---
>  include/linux/hw_bound_key.h | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 include/linux/hw_bound_key.h
> 
> diff --git a/include/linux/hw_bound_key.h b/include/linux/hw_bound_key.h
> new file mode 100644
> index 000000000000..e7f152410438
> --- /dev/null
> +++ b/include/linux/hw_bound_key.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-only
> + *
> + * Copyright 2022 NXP
> + * Author: Pankaj Gupta <pankaj.gupta@nxp.com>

Formatting here is incorrect and there is no such license in
existence as "GPL-2.0-only".

Should probably be:

/* SPDX-License-Identifier: GPL-2.0+ */
/*
 * Copyright (C) 2022 NXP Semiconductors N.V.
 */

Author-field is redundant as it is part of the git metadata.
Also it is inaccurate description of authorship, as a file
can have multiple contributors over time.

This all is documented in 

https://www.kernel.org/doc/html/latest/process/license-rules.html

> + */
> +
> +#ifndef _HW_BOUND_KEY_H
> +#define _HW_BOUND_KEY_H
> +
> +#include "types.h"
> +
> +struct hw_bound_key_info {
> +	/* Key types specific to the hw. [Implementation Defined]
> +	 */
> +	uint8_t flags;
> +	uint8_t reserved;
> +	/* Plain key size.
> +	 */
> +	uint16_t key_sz;
> +};
> +
> +#define set_hbk_info(hbk_info, hw_flags, key_len) do {\
> +	hbk_info->flags = hw_flags;\
> +	hbk_info->key_sz = key_len;\
> +} while (0)
> +
> +#endif /* _HW_BOUND_KEY_H */
> -- 
> 2.17.1
> 

BR, Jarkko

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v0 1/8] hw-bound-key: introducing the generic structure
  2022-10-06 13:08 ` [PATCH v0 1/8] hw-bound-key: introducing the generic structure Pankaj Gupta
  2022-10-12  8:52   ` Jarkko Sakkinen
@ 2022-10-12  8:53   ` Jarkko Sakkinen
  1 sibling, 0 replies; 31+ messages in thread
From: Jarkko Sakkinen @ 2022-10-12  8:53 UTC (permalink / raw)
  To: Pankaj Gupta
  Cc: a.fatoum, gilad, Jason, jejb, zohar, dhowells, sumit.garg, david,
	michael, john.ernberg, jmorris, serge, herbert, davem, j.luebbe,
	ebiggers, richard, keyrings, linux-crypto, linux-integrity,
	linux-kernel, linux-security-module, sahil.malhotra,
	kshitiz.varshney, horia.geanta, V.Sethi

On Thu, Oct 06, 2022 at 06:38:30PM +0530, Pankaj Gupta wrote:
> +#define set_hbk_info(hbk_info, hw_flags, key_len) do {\
> +	hbk_info->flags = hw_flags;\
> +	hbk_info->key_sz = key_len;\
> +} while (0)

Also this:

1. Undocumented.
2. No idea why you want to use a macro instead of inline function.

BR, Jarkko

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
  2022-10-06 13:08 ` [PATCH v0 3/8] crypto: hbk flags & info added to the tfm Pankaj Gupta
  2022-10-07  6:58   ` Herbert Xu
@ 2022-10-12  8:57   ` Jarkko Sakkinen
  1 sibling, 0 replies; 31+ messages in thread
From: Jarkko Sakkinen @ 2022-10-12  8:57 UTC (permalink / raw)
  To: Pankaj Gupta
  Cc: a.fatoum, gilad, Jason, jejb, zohar, dhowells, sumit.garg, david,
	michael, john.ernberg, jmorris, serge, herbert, davem, j.luebbe,
	ebiggers, richard, keyrings, linux-crypto, linux-integrity,
	linux-kernel, linux-security-module, sahil.malhotra,
	kshitiz.varshney, horia.geanta, V.Sethi

What are "hbk flags & info" and "the tfm"?

There can be multiple instances of struct crypto_tfm in
the kernel.

Maybe "crypto: Add hbk_info and is_hbk to struct crypto_tfm" ?

On Thu, Oct 06, 2022 at 06:38:32PM +0530, Pankaj Gupta wrote:
> Consumer of the kernel crypto api, after allocating
> the transformation (tfm), sets the:
> - flag 'is_hbk'
> - structure 'struct hw_bound_key_info hbk_info'
> based on the type of key, the consumer is using.
> 
> This helps:
> 
> - This helps to influence the core processing logic
>   for the encapsulated algorithm.
> - This flag is set by the consumer after allocating
>   the tfm and before calling the function crypto_xxx_setkey().

I don't really get "this helps part".



> 
> Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
> ---
>  include/linux/crypto.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/crypto.h b/include/linux/crypto.h
> index 2324ab6f1846..cd476f8a1cb4 100644
> --- a/include/linux/crypto.h
> +++ b/include/linux/crypto.h
> @@ -19,6 +19,7 @@
>  #include <linux/refcount.h>
>  #include <linux/slab.h>
>  #include <linux/completion.h>
> +#include <linux/hw_bound_key.h>
>  
>  /*
>   * Autoloaded crypto modules should only use a prefixed name to avoid allowing
> @@ -639,6 +640,10 @@ struct crypto_tfm {
>  
>  	u32 crt_flags;
>  
> +	unsigned int is_hbk;

Not sure why not just use bool as type here.

> +
> +	struct hw_bound_key_info hbk_info;
> +
>  	int node;
>  	
>  	void (*exit)(struct crypto_tfm *tfm);
> -- 
> 2.17.1
> 

BR, Jarkko

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v0 4/8] sk_cipher: checking for hw bound operation
  2022-10-06 13:08 ` [PATCH v0 4/8] sk_cipher: checking for hw bound operation Pankaj Gupta
@ 2022-10-12  8:59   ` Jarkko Sakkinen
  0 siblings, 0 replies; 31+ messages in thread
From: Jarkko Sakkinen @ 2022-10-12  8:59 UTC (permalink / raw)
  To: Pankaj Gupta
  Cc: a.fatoum, gilad, Jason, jejb, zohar, dhowells, sumit.garg, david,
	michael, john.ernberg, jmorris, serge, herbert, davem, j.luebbe,
	ebiggers, richard, keyrings, linux-crypto, linux-integrity,
	linux-kernel, linux-security-module, sahil.malhotra,
	kshitiz.varshney, horia.geanta, V.Sethi

In the short summary, please try to use imperative form and
accurate names. "Checking" means nothing.

On Thu, Oct 06, 2022 at 06:38:33PM +0530, Pankaj Gupta wrote:
> Checking for hw bound key. If yes,
>  - skipping the key-length validation to fall in min-max range.

What does "-" mean here? I seriously cannot interpret what I'm
reading.

> 
> Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>
> ---
>  crypto/skcipher.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/crypto/skcipher.c b/crypto/skcipher.c
> index 418211180cee..0f2d0228d73e 100644
> --- a/crypto/skcipher.c
> +++ b/crypto/skcipher.c
> @@ -598,7 +598,8 @@ int crypto_skcipher_setkey(struct crypto_skcipher *tfm, const u8 *key,
>  	unsigned long alignmask = crypto_skcipher_alignmask(tfm);
>  	int err;
>  
> -	if (keylen < cipher->min_keysize || keylen > cipher->max_keysize)
> +	if ((!tfm->base.is_hbk)
> +	    && (keylen < cipher->min_keysize || keylen > cipher->max_keysize))
>  		return -EINVAL;
>  
>  	if ((unsigned long)key & alignmask)
> -- 
> 2.17.1
>

BR, Jarkko

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [PATCH v0 7/8] caam alg: symmetric key ciphers are updated
  2022-10-06 13:08 ` [PATCH v0 7/8] caam alg: symmetric key ciphers are updated Pankaj Gupta
@ 2022-10-12  9:01   ` Jarkko Sakkinen
  0 siblings, 0 replies; 31+ messages in thread
From: Jarkko Sakkinen @ 2022-10-12  9:01 UTC (permalink / raw)
  To: Pankaj Gupta
  Cc: a.fatoum, gilad, Jason, jejb, zohar, dhowells, sumit.garg, david,
	michael, john.ernberg, jmorris, serge, herbert, davem, j.luebbe,
	ebiggers, richard, keyrings, linux-crypto, linux-integrity,
	linux-kernel, linux-security-module, sahil.malhotra,
	kshitiz.varshney, horia.geanta, V.Sethi

On Thu, Oct 06, 2022 at 06:38:36PM +0530, Pankaj Gupta wrote:
> Changes to enable:
> - To work both with black key and plain key.
> - It is supported in context of trusted key only.
>   - as meta-data is added as part of trusted key generation.
>   - otherwise, work as previously.
> 
> Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>

These commit messages look more like a personal backlog
copy-pasted than a reasonable summary of what is the
movation with the change and how the change achieves
the goals.

BR, Jarkko

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
  2022-10-11 20:01           ` Jason A. Donenfeld
@ 2022-10-12  9:06             ` Herbert Xu
  2022-10-14 19:19               ` Jason Gunthorpe
  0 siblings, 1 reply; 31+ messages in thread
From: Herbert Xu @ 2022-10-12  9:06 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Pankaj Gupta, jarkko, a.fatoum, gilad, jejb, zohar, dhowells,
	sumit.garg, david, michael, john.ernberg, jmorris, serge, davem,
	j.luebbe, ebiggers, richard, keyrings, linux-crypto,
	linux-integrity, linux-kernel, linux-security-module,
	Sahil Malhotra, Kshitiz Varshney, Horia Geanta, Varun Sethi

On Tue, Oct 11, 2022 at 02:01:45PM -0600, Jason A. Donenfeld wrote:
>
> I've got no stake in this, but isn't the whole idea that if you specify
> "aes" you get AES, and if you specify "cbc(aes)" you get AES-CBC, and so
> forth? And so leaking implementation details into the algorithm name
> feels like it breaks the abstraction a bit.

Well, keys stored in hardware are fundamentally incompatible with
the algorithm/implementation model.  The whole point of having
algorithms with multiple implementations (e.g., drivers) is that
they all provide exactly the same functionality and could be
substituted at will.

This completely breaks down with hardware keys because by definition
the key is stored in a specific piece of hardware so it will only
work with a particular driver.  IOW it almost never makes sense
to allocate "aes" if you have a hardware key, you almost always
want to allocate "aes-mydriver" instead.

> Rather, drivers that do AES should be called "aes". For this hardware
> key situation, I guess that means keys have a type (in-memory vs
> hardware-resident). Then, a crypto operation takes an "algorithm" and a
> "key", and the abstraction then picks the best implementation that's
> compatible with both the "algorithm" and the "key".

No the key is already in a specific hardware bound to some driver.
The user already knows where the key is and therefore they know
which driver it is.

> If you don't want a proliferation of different ways of doing the same
> thing, maybe the requirement should be that the author of this series
> also converts the existing "paes" kludge to use the new thing he's
> proposing?

Yes that would definitely be a good idea.  We should also talk to the
people who added paes in the first place, i.e., s390.

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

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
  2022-10-12  9:06             ` Herbert Xu
@ 2022-10-14 19:19               ` Jason Gunthorpe
  2022-10-20  4:26                 ` Eric Biggers
  0 siblings, 1 reply; 31+ messages in thread
From: Jason Gunthorpe @ 2022-10-14 19:19 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Jason A. Donenfeld, Pankaj Gupta, jarkko, a.fatoum, gilad, jejb,
	zohar, dhowells, sumit.garg, david, michael, john.ernberg,
	jmorris, serge, davem, j.luebbe, ebiggers, richard, keyrings,
	linux-crypto, linux-integrity, linux-kernel,
	linux-security-module, Sahil Malhotra, Kshitiz Varshney,
	Horia Geanta, Varun Sethi

On Wed, Oct 12, 2022 at 05:06:16PM +0800, Herbert Xu wrote:

> > Rather, drivers that do AES should be called "aes". For this hardware
> > key situation, I guess that means keys have a type (in-memory vs
> > hardware-resident). Then, a crypto operation takes an "algorithm" and a
> > "key", and the abstraction then picks the best implementation that's
> > compatible with both the "algorithm" and the "key".
> 
> No the key is already in a specific hardware bound to some driver.
> The user already knows where the key is and therefore they know
> which driver it is.

Do they?

We have HW that can do HW resident keys as as well, in our case it is
plugged into the storage system with fscrypt and all the crypto
operations are being done "inline" as the data is DMA'd into/out of
the storage. So, no crypto API here.

I would say the user knows about the key and its binding in the sense
they loaded a key into the storage device and mounted a fscrypt
filesystem from that storage device - but the kernel may not know this
explicitly.

> > If you don't want a proliferation of different ways of doing the same
> > thing, maybe the requirement should be that the author of this series
> > also converts the existing "paes" kludge to use the new thing he's
> > proposing?
> 
> Yes that would definitely be a good idea.  We should also talk to the
> people who added paes in the first place, i.e., s390.

Yes, it would be nice to see a comprehensive understand on how HW
resident keys can be modeled in the keyring. Almost every computer now
has a TPM that is also quite capable of doing operations with these
kinds of keys. Seeing the whole picture, including how we generate and
load/save/provision these things seems important.

Jason

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
  2022-10-14 19:19               ` Jason Gunthorpe
@ 2022-10-20  4:26                 ` Eric Biggers
  2022-10-20 19:23                   ` Jason Gunthorpe
  0 siblings, 1 reply; 31+ messages in thread
From: Eric Biggers @ 2022-10-20  4:26 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Herbert Xu, Jason A. Donenfeld, Pankaj Gupta, jarkko, a.fatoum,
	gilad, jejb, zohar, dhowells, sumit.garg, david, michael,
	john.ernberg, jmorris, serge, davem, j.luebbe, richard, keyrings,
	linux-crypto, linux-integrity, linux-kernel,
	linux-security-module, Sahil Malhotra, Kshitiz Varshney,
	Horia Geanta, Varun Sethi

Hi Jason,

On Fri, Oct 14, 2022 at 04:19:41PM -0300, Jason Gunthorpe wrote:
> On Wed, Oct 12, 2022 at 05:06:16PM +0800, Herbert Xu wrote:
> 
> > > Rather, drivers that do AES should be called "aes". For this hardware
> > > key situation, I guess that means keys have a type (in-memory vs
> > > hardware-resident). Then, a crypto operation takes an "algorithm" and a
> > > "key", and the abstraction then picks the best implementation that's
> > > compatible with both the "algorithm" and the "key".
> > 
> > No the key is already in a specific hardware bound to some driver.
> > The user already knows where the key is and therefore they know
> > which driver it is.
> 
> Do they?
> 
> We have HW that can do HW resident keys as as well, in our case it is
> plugged into the storage system with fscrypt and all the crypto
> operations are being done "inline" as the data is DMA'd into/out of
> the storage. So, no crypto API here.
> 
> I would say the user knows about the key and its binding in the sense
> they loaded a key into the storage device and mounted a fscrypt
> filesystem from that storage device - but the kernel may not know this
> explicitly.

Are you referring to the support for hardware-wrapped inline crypto keys?  It
isn't upstream yet, but my latest patchset is at
https://lore.kernel.org/linux-fscrypt/20220927014718.125308-2-ebiggers@kernel.org/T/#u.
There's also a version of it used by some Android devices already.  Out of
curiosity, are you using it in an Android device, or have you adopted it in some
other downstream?

Anyway, that feature does indeed use a boolean flag to indicate whether the key
is hardware-wrapped or not.  And yes, it doesn't use the crypto API.  Nor does
it use the keyrings subsystem, for that matter.

However, the design of hardware-wrapped inline crypto keys is that keys are
scoped to a particular block device (or a set of block devices), which are
assumed to have only one version of wrapped keys.  That makes the boolean flag
work, as it's always unambiguous what the keys mean.

I don't think that would work as well for the crypto API, which is a bit more
general.  In the crypto API, there can be an arbitrary number of crypto drivers,
each of which has its own version of hardware-wrapped (bound) keys.  So maybe
the existing design that is based on algorithm names is fine.

> > > If you don't want a proliferation of different ways of doing the same
> > > thing, maybe the requirement should be that the author of this series
> > > also converts the existing "paes" kludge to use the new thing he's
> > > proposing?
> > 
> > Yes that would definitely be a good idea.  We should also talk to the
> > people who added paes in the first place, i.e., s390.
> 
> Yes, it would be nice to see a comprehensive understand on how HW
> resident keys can be modeled in the keyring.

Note that the keyrings subsystem is not as useful as it might seem.  It sounds
like something you want (you have keys, and there is a subsystem called
"keyrings", so it should be used, right?), but often it isn't.  fscrypt has
mostly moved away from using it, as it caused lots of problems.  I would caution
against assuming that it needs to be part of any solution.

- Eric

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
  2022-10-20  4:26                 ` Eric Biggers
@ 2022-10-20 19:23                   ` Jason Gunthorpe
  2022-10-20 21:28                     ` Eric Biggers
  0 siblings, 1 reply; 31+ messages in thread
From: Jason Gunthorpe @ 2022-10-20 19:23 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Herbert Xu, Jason A. Donenfeld, Pankaj Gupta, jarkko, a.fatoum,
	gilad, jejb, zohar, dhowells, sumit.garg, david, michael,
	john.ernberg, jmorris, serge, davem, j.luebbe, richard, keyrings,
	linux-crypto, linux-integrity, linux-kernel,
	linux-security-module, Sahil Malhotra, Kshitiz Varshney,
	Horia Geanta, Varun Sethi

On Wed, Oct 19, 2022 at 09:26:05PM -0700, Eric Biggers wrote:

> Are you referring to the support for hardware-wrapped inline crypto keys?  It
> isn't upstream yet, but my latest patchset is at
> https://lore.kernel.org/linux-fscrypt/20220927014718.125308-2-ebiggers@kernel.org/T/#u.
> There's also a version of it used by some Android devices already.  Out of
> curiosity, are you using it in an Android device, or have you adopted it in some
> other downstream?

Unrelated to Android, similar functionality, but slightly different
ultimate purpose. We are going to be sending a fscrypt patch series
for mlx5 and nvme soonish.

> > Yes, it would be nice to see a comprehensive understand on how HW
> > resident keys can be modeled in the keyring.
> 
> Note that the keyrings subsystem is not as useful as it might seem.  It sounds
> like something you want (you have keys, and there is a subsystem called
> "keyrings", so it should be used, right?), but often it isn't.  fscrypt has
> mostly moved away from using it, as it caused lots of problems.  I would caution
> against assuming that it needs to be part of any solution.

That sounds disappointing that we are now having parallel ways for the
admin to manipulate kernel owned keys.

Jason

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
  2022-10-20 19:23                   ` Jason Gunthorpe
@ 2022-10-20 21:28                     ` Eric Biggers
  2022-10-20 23:42                       ` Jason Gunthorpe
  0 siblings, 1 reply; 31+ messages in thread
From: Eric Biggers @ 2022-10-20 21:28 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Herbert Xu, Jason A. Donenfeld, Pankaj Gupta, jarkko, a.fatoum,
	gilad, jejb, zohar, dhowells, sumit.garg, david, michael,
	john.ernberg, jmorris, serge, davem, j.luebbe, richard, keyrings,
	linux-crypto, linux-integrity, linux-kernel,
	linux-security-module, Sahil Malhotra, Kshitiz Varshney,
	Horia Geanta, Varun Sethi

On Thu, Oct 20, 2022 at 04:23:53PM -0300, Jason Gunthorpe wrote:
> On Wed, Oct 19, 2022 at 09:26:05PM -0700, Eric Biggers wrote:
> 
> > Are you referring to the support for hardware-wrapped inline crypto keys?  It
> > isn't upstream yet, but my latest patchset is at
> > https://lore.kernel.org/linux-fscrypt/20220927014718.125308-2-ebiggers@kernel.org/T/#u.
> > There's also a version of it used by some Android devices already.  Out of
> > curiosity, are you using it in an Android device, or have you adopted it in some
> > other downstream?
> 
> Unrelated to Android, similar functionality, but slightly different
> ultimate purpose. We are going to be sending a fscrypt patch series
> for mlx5 and nvme soonish.

That's interesting, though also slightly scary in that it sounds like you've
already shipped some major fscrypt changes without review!

> > > Yes, it would be nice to see a comprehensive understand on how HW
> > > resident keys can be modeled in the keyring.
> > 
> > Note that the keyrings subsystem is not as useful as it might seem.  It sounds
> > like something you want (you have keys, and there is a subsystem called
> > "keyrings", so it should be used, right?), but often it isn't.  fscrypt has
> > mostly moved away from using it, as it caused lots of problems.  I would caution
> > against assuming that it needs to be part of any solution.
> 
> That sounds disappointing that we are now having parallel ways for the
> admin to manipulate kernel owned keys.

Well, the keyrings subsystem never worked properly for fscrypt anyway.  At most,
it's only useful for providing the key to the filesystem initially (by passing a
key ID to FS_IOC_ADD_ENCRYPTION_KEY, instead of the key bytes), similar to what
dm-crypt allows.  After that, the keyrings subsystem plays no role.

I'm open to making FS_IOC_ADD_ENCRYPTION_KEY accept other 'struct key' types,
like "trusted" which has been discussed before and which dm-crypt supports.

Just don't assume that just because you have a key, that you automatically
*need* the keyrings subsystem.  Normally just passing the key bytes in the ioctl
works just as well and is much simpler.  Same for dm-crypt, which normally takes
the key bytes in the device-mapper table parameters...

- Eric

^ permalink raw reply	[flat|nested] 31+ messages in thread

* Re: [EXT] Re: [PATCH v0 3/8] crypto: hbk flags & info added to the tfm
  2022-10-20 21:28                     ` Eric Biggers
@ 2022-10-20 23:42                       ` Jason Gunthorpe
  0 siblings, 0 replies; 31+ messages in thread
From: Jason Gunthorpe @ 2022-10-20 23:42 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Herbert Xu, Jason A. Donenfeld, Pankaj Gupta, jarkko, a.fatoum,
	gilad, jejb, zohar, dhowells, sumit.garg, david, michael,
	john.ernberg, jmorris, serge, davem, j.luebbe, richard, keyrings,
	linux-crypto, linux-integrity, linux-kernel,
	linux-security-module, Sahil Malhotra, Kshitiz Varshney,
	Horia Geanta, Varun Sethi

On Thu, Oct 20, 2022 at 02:28:36PM -0700, Eric Biggers wrote:
> On Thu, Oct 20, 2022 at 04:23:53PM -0300, Jason Gunthorpe wrote:
> > On Wed, Oct 19, 2022 at 09:26:05PM -0700, Eric Biggers wrote:
> > 
> > > Are you referring to the support for hardware-wrapped inline crypto keys?  It
> > > isn't upstream yet, but my latest patchset is at
> > > https://lore.kernel.org/linux-fscrypt/20220927014718.125308-2-ebiggers@kernel.org/T/#u.
> > > There's also a version of it used by some Android devices already.  Out of
> > > curiosity, are you using it in an Android device, or have you adopted it in some
> > > other downstream?
> > 
> > Unrelated to Android, similar functionality, but slightly different
> > ultimate purpose. We are going to be sending a fscrypt patch series
> > for mlx5 and nvme soonish.
> 
> That's interesting, though also slightly scary in that it sounds like you've
> already shipped some major fscrypt changes without review!

Heh, says the Android guy :)

Fortunately nothing major, we are enterprise focused, we need stuff in
real distros - we know know how to do it.

> > That sounds disappointing that we are now having parallel ways for the
> > admin to manipulate kernel owned keys.
> 
> Well, the keyrings subsystem never worked properly for fscrypt anyway.  At most,
> it's only useful for providing the key to the filesystem initially (by passing a
> key ID to FS_IOC_ADD_ENCRYPTION_KEY, instead of the key bytes), similar to what
> dm-crypt allows.  After that, the keyrings subsystem plays no role.

Sure, but loading the key into the keyring should allow many different
options, including things like TPM PCR secured keys (eg like
bitlocker) - we shouldn't allow user space the ability to see the key
data at all.

Duplicating this in every subsystem makes no sense, there is a
reasonable role for the keyring to play in solving these kinds of
problems for everything.

Jason

^ permalink raw reply	[flat|nested] 31+ messages in thread

end of thread, other threads:[~2022-10-20 23:43 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-06 13:08 [PATCH v0 0/8] Hardware Bound key added to Trusted Key-Ring Pankaj Gupta
2022-10-06 13:08 ` [PATCH v0 1/8] hw-bound-key: introducing the generic structure Pankaj Gupta
2022-10-12  8:52   ` Jarkko Sakkinen
2022-10-12  8:53   ` Jarkko Sakkinen
2022-10-06 13:08 ` [PATCH v0 2/8] keys-trusted: new cmd line option added Pankaj Gupta
2022-10-06 12:37   ` Ben Boeckel
2022-10-06 13:08 ` [PATCH v0 3/8] crypto: hbk flags & info added to the tfm Pankaj Gupta
2022-10-07  6:58   ` Herbert Xu
2022-10-10 11:15     ` [EXT] " Pankaj Gupta
2022-10-10 15:15       ` Jason A. Donenfeld
2022-10-10 21:35         ` [EXT] " David Gstir
2022-10-11  9:03         ` [EXT] " Herbert Xu
2022-10-11 11:32           ` Pankaj Gupta
2022-10-11 20:01           ` Jason A. Donenfeld
2022-10-12  9:06             ` Herbert Xu
2022-10-14 19:19               ` Jason Gunthorpe
2022-10-20  4:26                 ` Eric Biggers
2022-10-20 19:23                   ` Jason Gunthorpe
2022-10-20 21:28                     ` Eric Biggers
2022-10-20 23:42                       ` Jason Gunthorpe
2022-10-11 11:05         ` Pankaj Gupta
2022-10-12  8:57   ` Jarkko Sakkinen
2022-10-06 13:08 ` [PATCH v0 4/8] sk_cipher: checking for hw bound operation Pankaj Gupta
2022-10-12  8:59   ` Jarkko Sakkinen
2022-10-06 13:08 ` [PATCH v0 5/8] keys-trusted: re-factored caam based trusted key Pankaj Gupta
2022-10-06 13:08 ` [PATCH v0 6/8] KEYS: trusted: caam based black key Pankaj Gupta
2022-10-06 12:42   ` Ben Boeckel
2022-10-06 12:52     ` James Bottomley
2022-10-06 13:08 ` [PATCH v0 7/8] caam alg: symmetric key ciphers are updated Pankaj Gupta
2022-10-12  9:01   ` Jarkko Sakkinen
2022-10-06 13:08 ` [PATCH v0 8/8] dm-crypt: consumer-app setting the flag-is_hbk Pankaj Gupta

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).