RE: [EXT] Re: [RFC PATCH HBK: 1/8] keys-trusted: new cmd line option added

-----Original Message-----
From: Ben Boeckel <redacted>
Sent: Tuesday, September 6, 2022 6:32 PM
To: Pankaj Gupta <pankaj.gupta@nxp.com>
Cc: jarkko@kernel.org; a.fatoum@pengutronix.de; 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;
herbert@gondor.apana.org.au; 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 [off-list ref]; Kshitiz
Varshney [off-list ref]; Horia Geanta
[off-list ref]; Varun Sethi [off-list ref]
Subject: [EXT] Re: [RFC PATCH HBK: 1/8] keys-trusted: new cmd line option
added

Caution: EXT Email

On Tue, Sep 06, 2022 at 12:21:50 +0530, Pankaj Gupta wrote:

quoted

Two changes are 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

- For "new", generating the hw bounded trusted key, updating the input

key

quoted

  length as part of seal operation as well.

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

diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h
index 4eb64548a74f..064266b936c7 100644
--- a/include/keys/trusted-type.h
+++ b/include/keys/trusted-type.h

@@ -22,6 +22,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 +30,7 @@ struct trusted_key_payload {
      unsigned int blob_len;
      unsigned char migratable;
      unsigned char old_format;
+     unsigned char is_hw_bound;
      unsigned char key[MAX_KEY_SIZE + 1];
      unsigned char blob[MAX_BLOB_SIZE];  }; diff --git

a/security/keys/trusted-keys/trusted_caam.c
b/security/keys/trusted-keys/trusted_caam.c
index e3415c520c0a..fceb9a271c4d 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

[off-list ref]
+ * Copyright 2022 NXP, Pankaj Gupta [off-list ref]
  */

 #include <keys/trusted_caam.h>

@@ -23,6 +24,7 @@ static int trusted_caam_seal(struct

trusted_key_payload *p, char *datablob)

quoted

              .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,
      };

      ret = caam_encap_blob(blobifier, &info); @@ -30,6 +32,9 @@
static int trusted_caam_seal(struct trusted_key_payload *p, char

*datablob)

quoted

              return ret;

      p->blob_len = info.output_len;
+     if (p->is_hw_bound)
+             p->key_len = info.input_len;
+
      return 0;
 }

@@ -40,6 +45,7 @@ static int trusted_caam_unseal(struct

trusted_key_payload *p, char *datablob)

quoted

              .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,
      };

      ret = caam_decap_blob(blobifier, &info); diff --git
a/security/keys/trusted-keys/trusted_core.c
b/security/keys/trusted-keys/trusted_core.c
index c6fc50d67214..7f7cc2551b92 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)

quoted

      int key_cmd;
      char *c;

+     p->is_hw_bound = !HW_BOUND_KEY;

This seems…backwards to me.

Initialized it to be a plain key & not a HW bounded key.
 

quoted

@@ -94,6 +96,12 @@ static int datablob_parse(char **datablob, struct

trusted_key_payload *p)

quoted

              if (ret < 0 || keylen < MIN_KEY_SIZE || keylen > MAX_KEY_SIZE)
                      return -EINVAL;
              p->key_len = keylen;
+             /* second argument is to determine if tied to HW */
+             c = strsep(datablob, " \t");
+             if (c) {
+                     if (strcmp(c, "hw") == 0)
+                             p->is_hw_bound = HW_BOUND_KEY;
+             }

Userspace documentation is missing for this new field. Must it always be
second or is it "any following argument"? For example, let's say we have
another flag like this for "FIPS" (or whatever). It'd be nice if these all worked:

    'new 32 fips hw'
    'new 32 fips'
    'new 32 hw fips'
    'new 32 hw'

Will consider this, in the next version of this patch set.

quoted

@@ -107,6 +115,12 @@ static int datablob_parse(char **datablob, struct

trusted_key_payload *p)

quoted

              ret = hex2bin(p->blob, c, p->blob_len);
              if (ret < 0)
                      return -EINVAL;
+             /* second argument is to determine if tied to HW */
+             c = strsep(datablob, " \t");
+             if (c) {
+                     if (strcmp(c, "hw") == 0)
+                             p->is_hw_bound = HW_BOUND_KEY;
+             }

Same here.

--Ben