linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Sumit Garg <sumit.garg@linaro.org>
To: James Bottomley <jejb@linux.ibm.com>
Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	Mimi Zohar <zohar@linux.ibm.com>,
	 David Howells <dhowells@redhat.com>,
	Jens Wiklander <jens.wiklander@linaro.org>,
	Jonathan Corbet <corbet@lwn.net>,
	James Morris <jmorris@namei.org>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	 Casey Schaufler <casey@schaufler-ca.com>,
	Janne Karhunen <janne.karhunen@gmail.com>,
	 Daniel Thompson <daniel.thompson@linaro.org>,
	Markus Wamser <Markus.Wamser@mixed-mode.de>,
	 Luke Hinds <lhinds@redhat.com>,
	Elaine Palmer <erpalmer@us.ibm.com>,
	 Ahmad Fatoum <a.fatoum@pengutronix.de>,
	 "open list:ASYMMETRIC KEYS" <keyrings@vger.kernel.org>,
	 linux-integrity <linux-integrity@vger.kernel.org>,
	 "open list:SECURITY SUBSYSTEM"
	<linux-security-module@vger.kernel.org>,
	 Linux Doc Mailing List <linux-doc@vger.kernel.org>,
	 Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	 linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	op-tee@lists.trustedfirmware.org
Subject: Re: [PATCH v9 1/4] KEYS: trusted: Add generic trusted keys framework
Date: Wed, 21 Apr 2021 16:38:13 +0530	[thread overview]
Message-ID: <CAFA6WYOzD-qhHrcnzvd9P7iFvEqWwf0NCKXrgrEgvnB5i_-SxQ@mail.gmail.com> (raw)
In-Reply-To: <65dcc9fa28833e6beb1eadf98b0ed3402404d693.camel@linux.ibm.com>

Hi James,

On Wed, 21 Apr 2021 at 04:47, James Bottomley <jejb@linux.ibm.com> wrote:
>
> On Mon, 2021-03-01 at 18:41 +0530, Sumit Garg wrote:
> > Current trusted keys framework is tightly coupled to use TPM device
> > as an underlying implementation which makes it difficult for
> > implementations like Trusted Execution Environment (TEE) etc. to
> > provide trusted keys support in case platform doesn't posses a TPM
> > device.
> >
> > Add a generic trusted keys framework where underlying implementations
> > can be easily plugged in. Create struct trusted_key_ops to achieve
> > this, which contains necessary functions of a backend.
> >
> > Also, define a module parameter in order to select a particular trust
> > source in case a platform support multiple trust sources. In case its
> > not specified then implementation itetrates through trust sources
> > list starting with TPM and assign the first trust source as a backend
> > which has initiazed successfully during iteration.
> >
> > Note that current implementation only supports a single trust source
> > at runtime which is either selectable at compile time or during boot
> > via aforementioned module parameter.
>
> You never actually tested this, did you?  I'm now getting EINVAL from
> all the trusted TPM key operations because of this patch.
>

Unfortunately, I don't possess a development machine with a TPM
device. So mine testing was entirely based on TEE as a backend which
doesn't support any optional parameters. And that being the reason I
didn't catch this issue at first instance.

Is there any TPM emulation environment available that I can use for testing?

> The reason is quite simple:  this function:
>
> > index 000000000000..0db86b44605d
> > --- /dev/null
> > +++ b/security/keys/trusted-keys/trusted_core.c
> [...]
> > +static int datablob_parse(char *datablob, struct trusted_key_payload
> > *p)
> > +{
> > +     substring_t args[MAX_OPT_ARGS];
> > +     long keylen;
> > +     int ret = -EINVAL;
> > +     int key_cmd;
> > +     char *c;
> > +
> > +     /* main command */
> > +     c = strsep(&datablob, " \t");
>
> Modifies its argument to consume tokens and separates them with NULL.
>
> so the arguments for
>
> keyctl add trusted kmk "new 34 keyhandle=0x81000001"
>
> Go into this function as
>
> datablob="new 34 keyhandle=0x81000001"
>
> After we leave it, it looks like
>
> datablob="new\034\0keyhandle=0x81000001"
>
> However here:
>
> > +static int trusted_instantiate(struct key *key,
> > +                            struct key_preparsed_payload *prep)
> > +{
> > +     struct trusted_key_payload *payload = NULL;
> > +     size_t datalen = prep->datalen;
> > +     char *datablob;
> > +     int ret = 0;
> > +     int key_cmd;
> > +     size_t key_len;
> > +
> > +     if (datalen <= 0 || datalen > 32767 || !prep->data)
> > +             return -EINVAL;
> > +
> > +     datablob = kmalloc(datalen + 1, GFP_KERNEL);
> > +     if (!datablob)
> > +             return -ENOMEM;
> > +     memcpy(datablob, prep->data, datalen);
> > +     datablob[datalen] = '\0';
> > +
> > +     payload = trusted_payload_alloc(key);
> > +     if (!payload) {
> > +             ret = -ENOMEM;
> > +             goto out;
> > +     }
> > +
> > +     key_cmd = datablob_parse(datablob, payload);
> > +     if (key_cmd < 0) {
> > +             ret = key_cmd;
> > +             goto out;
> > +     }
> > +
> > +     dump_payload(payload);
> > +
> > +     switch (key_cmd) {
> > +     case Opt_load:
> > +             ret = static_call(trusted_key_unseal)(payload,
> > datablob);
>
> We're passing the unmodified
>
> datablob="new\034\0keyhandle=0x81000001"
>
> Into the tpm trusted_key_unseal function.  However, it only sees "new"
> and promply gives EINVAL because you've removed the ability to process
> the new option from it.  What should have happened is you should have
> moved data blob up to passed the consumed tokens, so it actually reads
>
> datablob="keyhandle=0x81000001"
>
> However, to do that you'd have to have the updated pointer passed out
> of your datablob_parse() above.

Thanks for the detailed explanation.

>
> There's also a lost !tpm2 in the check for options->keyhandle, but I
> suspect Jarkko lost that merging the two patches.  I think what's below
> fixes all of this, so if you can test it for trusted_tee, I'll package
> it up as two separate patches fixing all of this.
>

Below fixes look good to me and I have tested them using TEE as a
backend too. So feel free to add:

Tested-by: Sumit Garg <sumit.garg@linaro.org>

-Sumit

> James
>
> ---
>
> diff --git a/security/keys/trusted-keys/trusted_core.c b/security/keys/trusted-keys/trusted_core.c
> index ec3a066a4b42..7c636212429b 100644
> --- a/security/keys/trusted-keys/trusted_core.c
> +++ b/security/keys/trusted-keys/trusted_core.c
> @@ -62,7 +62,7 @@ static const match_table_t key_tokens = {
>   *
>   * On success returns 0, otherwise -EINVAL.
>   */
> -static int datablob_parse(char *datablob, struct trusted_key_payload *p)
> +static int datablob_parse(char **datablob, struct trusted_key_payload *p)
>  {
>         substring_t args[MAX_OPT_ARGS];
>         long keylen;
> @@ -71,14 +71,14 @@ static int datablob_parse(char *datablob, struct trusted_key_payload *p)
>         char *c;
>
>         /* main command */
> -       c = strsep(&datablob, " \t");
> +       c = strsep(datablob, " \t");
>         if (!c)
>                 return -EINVAL;
>         key_cmd = match_token(c, key_tokens, args);
>         switch (key_cmd) {
>         case Opt_new:
>                 /* first argument is key size */
> -               c = strsep(&datablob, " \t");
> +               c = strsep(datablob, " \t");
>                 if (!c)
>                         return -EINVAL;
>                 ret = kstrtol(c, 10, &keylen);
> @@ -89,7 +89,7 @@ static int datablob_parse(char *datablob, struct trusted_key_payload *p)
>                 break;
>         case Opt_load:
>                 /* first argument is sealed blob */
> -               c = strsep(&datablob, " \t");
> +               c = strsep(datablob, " \t");
>                 if (!c)
>                         return -EINVAL;
>                 p->blob_len = strlen(c) / 2;
> @@ -138,7 +138,7 @@ static int trusted_instantiate(struct key *key,
>  {
>         struct trusted_key_payload *payload = NULL;
>         size_t datalen = prep->datalen;
> -       char *datablob;
> +       char *datablob, *orig_datablob;
>         int ret = 0;
>         int key_cmd;
>         size_t key_len;
> @@ -146,7 +146,7 @@ static int trusted_instantiate(struct key *key,
>         if (datalen <= 0 || datalen > 32767 || !prep->data)
>                 return -EINVAL;
>
> -       datablob = kmalloc(datalen + 1, GFP_KERNEL);
> +       orig_datablob = datablob = kmalloc(datalen + 1, GFP_KERNEL);
>         if (!datablob)
>                 return -ENOMEM;
>         memcpy(datablob, prep->data, datalen);
> @@ -158,7 +158,7 @@ static int trusted_instantiate(struct key *key,
>                 goto out;
>         }
>
> -       key_cmd = datablob_parse(datablob, payload);
> +       key_cmd = datablob_parse(&datablob, payload);
>         if (key_cmd < 0) {
>                 ret = key_cmd;
>                 goto out;
> @@ -194,7 +194,7 @@ static int trusted_instantiate(struct key *key,
>                 ret = -EINVAL;
>         }
>  out:
> -       kfree_sensitive(datablob);
> +       kfree_sensitive(orig_datablob);
>         if (!ret)
>                 rcu_assign_keypointer(key, payload);
>         else
> @@ -218,7 +218,7 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
>         struct trusted_key_payload *p;
>         struct trusted_key_payload *new_p;
>         size_t datalen = prep->datalen;
> -       char *datablob;
> +       char *datablob, *orig_datablob;
>         int ret = 0;
>
>         if (key_is_negative(key))
> @@ -229,7 +229,7 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
>         if (datalen <= 0 || datalen > 32767 || !prep->data)
>                 return -EINVAL;
>
> -       datablob = kmalloc(datalen + 1, GFP_KERNEL);
> +       orig_datablob = datablob = kmalloc(datalen + 1, GFP_KERNEL);
>         if (!datablob)
>                 return -ENOMEM;
>
> @@ -241,7 +241,7 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
>
>         memcpy(datablob, prep->data, datalen);
>         datablob[datalen] = '\0';
> -       ret = datablob_parse(datablob, new_p);
> +       ret = datablob_parse(&datablob, new_p);
>         if (ret != Opt_update) {
>                 ret = -EINVAL;
>                 kfree_sensitive(new_p);
> @@ -265,7 +265,7 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
>         rcu_assign_keypointer(key, new_p);
>         call_rcu(&p->rcu, trusted_rcu_free);
>  out:
> -       kfree_sensitive(datablob);
> +       kfree_sensitive(orig_datablob);
>         return ret;
>  }
>
> diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c
> index 4e5c50138f92..bc702ba0a596 100644
> --- a/security/keys/trusted-keys/trusted_tpm1.c
> +++ b/security/keys/trusted-keys/trusted_tpm1.c
> @@ -747,6 +747,9 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
>
>         opt->hash = tpm2 ? HASH_ALGO_SHA256 : HASH_ALGO_SHA1;
>
> +       if (!c)
> +               return 0;
> +
>         while ((p = strsep(&c, " \t"))) {
>                 if (*p == '\0' || *p == ' ' || *p == '\t')
>                         continue;
> @@ -944,7 +947,7 @@ static int trusted_tpm_unseal(struct trusted_key_payload *p, char *datablob)
>                 goto out;
>         dump_options(options);
>
> -       if (!options->keyhandle) {
> +       if (!options->keyhandle && !tpm2) {
>                 ret = -EINVAL;
>                 goto out;
>         }
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-04-21 11:10 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-01 13:11 [PATCH v9 0/4] Introduce TEE based Trusted Keys support Sumit Garg
2021-03-01 13:11 ` [PATCH v9 1/4] KEYS: trusted: Add generic trusted keys framework Sumit Garg
2021-03-01 21:29   ` Jarkko Sakkinen
2021-04-20 23:16   ` James Bottomley
2021-04-21 11:08     ` Sumit Garg [this message]
2021-04-21 17:20       ` James Bottomley
2021-04-22  4:47         ` Sumit Garg
2021-03-01 13:11 ` [PATCH v9 2/4] KEYS: trusted: Introduce TEE based Trusted Keys Sumit Garg
2021-03-01 21:29   ` Jarkko Sakkinen
2021-03-01 13:11 ` [PATCH v9 3/4] doc: trusted-encrypted: updates with TEE as a new trust source Sumit Garg
2021-03-01 13:11 ` [PATCH v9 4/4] MAINTAINERS: Add entry for TEE based Trusted Keys Sumit Garg
2021-03-04 10:00 ` [PATCH v9 0/4] Introduce TEE based Trusted Keys support Sumit Garg
2021-03-04 15:43   ` Jarkko Sakkinen
2021-03-09  9:10     ` Sumit Garg
2021-03-10 19:56       ` Jarkko Sakkinen
2021-03-10 22:26         ` James Bottomley
2021-03-10 23:35           ` Jarkko Sakkinen
2021-03-10 23:41             ` Jarkko Sakkinen
2021-03-12 16:26           ` Jarkko Sakkinen
2021-03-12 16:30             ` James Bottomley
2021-03-13 10:44               ` Jarkko Sakkinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAFA6WYOzD-qhHrcnzvd9P7iFvEqWwf0NCKXrgrEgvnB5i_-SxQ@mail.gmail.com \
    --to=sumit.garg@linaro.org \
    --cc=Markus.Wamser@mixed-mode.de \
    --cc=a.fatoum@pengutronix.de \
    --cc=casey@schaufler-ca.com \
    --cc=corbet@lwn.net \
    --cc=daniel.thompson@linaro.org \
    --cc=dhowells@redhat.com \
    --cc=erpalmer@us.ibm.com \
    --cc=janne.karhunen@gmail.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=jejb@linux.ibm.com \
    --cc=jens.wiklander@linaro.org \
    --cc=jmorris@namei.org \
    --cc=keyrings@vger.kernel.org \
    --cc=lhinds@redhat.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=op-tee@lists.trustedfirmware.org \
    --cc=serge@hallyn.com \
    --cc=zohar@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).