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
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 <[email protected]>
---
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 <[email protected]>
+ */
+
+#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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Checking for hw bound key. If yes,
- skipping the key-length validation to fall in min-max range.
Signed-off-by: Pankaj Gupta <[email protected]>
---
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
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 <[email protected]>
---
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 <[email protected]>
* Copyright (C) 2021 Pengutronix, Ahmad Fatoum <[email protected]>
+ * Copyright 2022 NXP, Pankaj Gupta <[email protected]>
*/
#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 <[email protected]>
+ * Copyright 2022 NXP, Pankaj Gupta <[email protected]>
*/
#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
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 <[email protected]>
---
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
- 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 <[email protected]>
---
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 <[email protected]>
+ * Copyright 2022 NXP, Pankaj Gupta <[email protected]>
*/
#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
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 <[email protected]>
---
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
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
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
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
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 <[email protected]>
> ---
> 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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> -----Original Message-----
> From: Herbert Xu <[email protected]>
> Sent: Friday, October 7, 2022 12:28 PM
> To: Pankaj Gupta <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; Sahil Malhotra
> <[email protected]>; Kshitiz Varshney <[email protected]>;
> Horia Geanta <[email protected]>; Varun Sethi <[email protected]>
> 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 <[email protected]>
> > ---
> > 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 <[email protected]> Home Page:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgondor.ap
> ana.org.au%2F~herbert%2F&data=05%7C01%7Cpankaj.gupta%40nxp.com
> %7C33055110772a4d4bb97508daa8317e93%7C686ea1d3bc2b4c6fa92cd99c5c3
> 01635%7C0%7C0%7C638007227793511347%7CUnknown%7CTWFpbGZsb3d8ey
> JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> 3000%7C%7C%7C&sdata=H0fzzxQhsV%2FyPphBAHBDmzQfTFnjDE7jYstTM
> ok%2F09I%3D&reserved=0
> PGP Key:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgondor.ap
> ana.org.au%2F~herbert%2Fpubkey.txt&data=05%7C01%7Cpankaj.gupta%4
> 0nxp.com%7C33055110772a4d4bb97508daa8317e93%7C686ea1d3bc2b4c6fa9
> 2cd99c5c301635%7C0%7C0%7C638007227793667554%7CUnknown%7CTWFpb
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6M
> n0%3D%7C3000%7C%7C%7C&sdata=SclJ9G7jBWhOW%2Fm3Gt0jP1oicqVp
> 5ghH%2BDT8Vd5maag%3D&reserved=0
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
> On 10.10.2022, at 17:15, Jason A. Donenfeld <[email protected]> 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/[email protected]/
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> -----Original Message-----
> From: Jason A. Donenfeld <[email protected]>
> Sent: Monday, October 10, 2022 8:46 PM
> To: Pankaj Gupta <[email protected]>
> Cc: 'Herbert Xu' <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-security-
> [email protected]; Sahil Malhotra <[email protected]>; Kshitiz
> Varshney <[email protected]>; Horia Geanta
> <[email protected]>; Varun Sethi <[email protected]>
> 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
> -----Original Message-----
> From: Herbert Xu <[email protected]>
> Sent: Tuesday, October 11, 2022 2:34 PM
> To: Jason A. Donenfeld <[email protected]>
> Cc: Pankaj Gupta <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-security-
> [email protected]; Sahil Malhotra <[email protected]>; Kshitiz
> Varshney <[email protected]>; Horia Geanta
> <[email protected]>; Varun Sethi <[email protected]>
> 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 <[email protected]> Home Page:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgondo
> r.apana.org.au%2F~herbert%2F&data=05%7C01%7Cpankaj.gupta%40nx
> p.com%7C4ef27fc922d04350ca9f08daab67a1a3%7C686ea1d3bc2b4c6fa92cd9
> 9c5c301635%7C0%7C0%7C638010758832054902%7CUnknown%7CTWFpbGZs
> b3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> 0%3D%7C3000%7C%7C%7C&sdata=SOguJ9LGhSCDmspbjDIEzkQLk9Bz%
> 2FsS0B%2BLNc4gzRo8%3D&reserved=0
> PGP Key:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgondo
> r.apana.org.au%2F~herbert%2Fpubkey.txt&data=05%7C01%7Cpankaj.g
> upta%40nxp.com%7C4ef27fc922d04350ca9f08daab67a1a3%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C0%7C638010758832054902%7CUnknown%7C
> TWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiL
> CJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=hCzT2fPfJ%2BBNVqN6JR
> wMx9zNJkqvdRSLrR68ubhCvN4%3D&reserved=0
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
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 <[email protected]>
> ---
> 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 <[email protected]>
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
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
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
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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
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/[email protected]/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
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/[email protected]/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
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/[email protected]/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
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/[email protected]/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