> -----Original Message-----
> From: Ben Boeckel <me@xxxxxxxxxxxxxx>
> Sent: Tuesday, September 6, 2022 6:32 PM
> To: Pankaj Gupta <pankaj.gupta@xxxxxxx>
> Cc: jarkko@xxxxxxxxxx; a.fatoum@xxxxxxxxxxxxxx; Jason@xxxxxxxxx;
> jejb@xxxxxxxxxxxxx; zohar@xxxxxxxxxxxxx; dhowells@xxxxxxxxxx;
> sumit.garg@xxxxxxxxxx; david@xxxxxxxxxxxxx; michael@xxxxxxxx;
> john.ernberg@xxxxxxxx; jmorris@xxxxxxxxx; serge@xxxxxxxxxx;
> herbert@xxxxxxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx;
> j.luebbe@xxxxxxxxxxxxxx; ebiggers@xxxxxxxxxx; richard@xxxxxx;
> keyrings@xxxxxxxxxxxxxxx; linux-crypto@xxxxxxxxxxxxxxx; linux-
> integrity@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-security-
> module@xxxxxxxxxxxxxxx; Sahil Malhotra <sahil.malhotra@xxxxxxx>; Kshitiz
> Varshney <kshitiz.varshney@xxxxxxx>; Horia Geanta
> <horia.geanta@xxxxxxx>; Varun Sethi <V.Sethi@xxxxxxx>
> 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:
> > 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
> > length as part of seal operation as well.
> >
> > Signed-off-by: Pankaj Gupta <pankaj.gupta@xxxxxxx>
> > ---
> > 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
> > <kernel@xxxxxxxxxxxxxx>
> > + * Copyright 2022 NXP, Pankaj Gupta <pankaj.gupta@xxxxxxx>
> > */
> >
> > #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,
> > };
> >
> > ret = caam_encap_blob(blobifier, &info); @@ -30,6 +32,9 @@
> > 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 +45,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,
> > };
> >
> > 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)
> > 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.
> > @@ -94,6 +96,12 @@ 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;
> > + /* 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.
> > @@ -107,6 +115,12 @@ static int datablob_parse(char **datablob, struct
> trusted_key_payload *p)
> > 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
