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

[Ben Boeckel <me@benboeckel.net>]

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.

> @@ -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'

> @@ -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