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

[Pankaj Gupta <pankaj.gupta@nxp.com>]

> -----Original Message-----
> From: Ben Boeckel <me@benboeckel.net>
> 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 <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: [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@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
> > <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,
> >       };
> >
> >       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