[cbootimage,v1,2/8] Add bct_dump support for t210

Added support to dump additional fields such as rsa pubkey, rsa pss
signatures for bct and bootloader.

Signed-off-by: Jimmy Zhang <jimmzhang@nvidia.com>
---
 src/bct_dump.c           | 39 ++++++++++++++++++++++++++++++++++++++-
 src/t210/nvbctlib_t210.c | 23 +++++++++++++++++++----
 2 files changed, 57 insertions(+), 5 deletions(-)

On 09/02/2015 03:19 PM, Jimmy Zhang wrote:
> Added support to dump additional fields such as rsa pubkey, rsa pss
> signatures for bct and bootloader.

> diff --git a/src/bct_dump.c b/src/bct_dump.c

> @@ -54,11 +57,13 @@ static value_data const values[] = {
>   	{ token_odm_data,            "OdmData       = ", format_u32_hex8 },
>   	{ token_secure_jtag_control, "JtagCtrl      = ", format_u32_hex8 },
>   	{ token_secure_debug_control, "DebugCtrl     = ", format_u32_hex8 },
> +	{ token_crypto_hash, 	     "BCT AES Hash  = ", format_hex_16_bytes },
> +	{ token_pub_key_modulus,     "PubKeyModulus = ", format_rsa_param },
> +	{ token_rsa_pss_sig_bct,     "RsaPssSig_Bct = ", format_rsa_param },

The previous patch didn't have an underscore in the keyword name. We 
should be able to feed the output of bctdump straight back into 
cbootimage without having to manually fix up the syntax.

That said, I wonder if either (a) shouldn't this data be dumped to a 
file, since cbootimage would read it from a separate file (b) shouldn't 
cbootimage read the data from an inline value not a separate file. The 
use can always use the C pre-processor or similar "code generation" 
techniques to create the file from a template in order to use different 
keys for different devices.

> -	{ token_hash_size,           "# Hash size             = ", format_u32 },

Is there a reason for removing that? If so, it should be mentioned in 
the patch description.

> diff --git a/src/t210/nvbctlib_t210.c b/src/t210/nvbctlib_t210.c

> +	case token_rsa_pss_sig_bl:
> +		/* ONLY support one bl */
> +		memcpy(data, &bct_ptr->bootloader[set].signature.rsa_pss_sig,
> +			sizeof(nvboot_rsa_pss_sig));
> +		break;

Same objection about that comment here as in the previous patch.

> @@ -2121,15 +2128,23 @@ t210_bct_get_value(parse_token id, void *data, u_int8_t *bct)

>   	case token_crypto_hash:
> -		memcpy(data,
> -		&(bct_ptr->signature.crypto_hash),
> -		sizeof(nvboot_hash));
> +		memcpy(data, &(bct_ptr->signature.crypto_hash),
> +			sizeof(nvboot_hash));

This feels like an unrelated cleanup patch?
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> -----Original Message-----
> From: Stephen Warren [mailto:swarren@wwwdotorg.org]
> Sent: Monday, September 21, 2015 1:06 PM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; linux-tegra@vger.kernel.org
> Subject: Re: [cbootimage PATCH v1 2/8] Add bct_dump support for t210
> 
> On 09/02/2015 03:19 PM, Jimmy Zhang wrote:
> > Added support to dump additional fields such as rsa pubkey, rsa pss
> > signatures for bct and bootloader.
> 
> > diff --git a/src/bct_dump.c b/src/bct_dump.c
> 
> > @@ -54,11 +57,13 @@ static value_data const values[] = {
> >   	{ token_odm_data,            "OdmData       = ", format_u32_hex8 },
> >   	{ token_secure_jtag_control, "JtagCtrl      = ", format_u32_hex8 },
> >   	{ token_secure_debug_control, "DebugCtrl     = ", format_u32_hex8
> },
> > +	{ token_crypto_hash, 	     "BCT AES Hash  = ", format_hex_16_bytes
> },
> > +	{ token_pub_key_modulus,     "PubKeyModulus = ",
> format_rsa_param },
> > +	{ token_rsa_pss_sig_bct,     "RsaPssSig_Bct = ", format_rsa_param },
> 
> The previous patch didn't have an underscore in the keyword name. We
> should be able to feed the output of bctdump straight back into cbootimage
> without having to manually fix up the syntax.
> 
> That said, I wonder if either (a) shouldn't this data be dumped to a file, since
> cbootimage would read it from a separate file (b) shouldn't cbootimage read
> the data from an inline value not a separate file. The use can always use the C
> pre-processor or similar "code generation"
> techniques to create the file from a template in order to use different keys
> for different devices.
> 

Good inspiration. We don't limit how this tool is being used. I mainly used it for debug purpose.

I will remove the under score as you suggested.
  
> > -	{ token_hash_size,           "# Hash size             = ", format_u32 },
> 
> Is there a reason for removing that? If so, it should be mentioned in the
> patch description.
> 

This piece of code is not needed for dump_bct.  I can add descriptions in commit message. Otherwise, I can revert it.

> > diff --git a/src/t210/nvbctlib_t210.c b/src/t210/nvbctlib_t210.c
> 
> > +	case token_rsa_pss_sig_bl:
> > +		/* ONLY support one bl */
> > +		memcpy(data, &bct_ptr-
> >bootloader[set].signature.rsa_pss_sig,
> > +			sizeof(nvboot_rsa_pss_sig));
> > +		break;
> 
> Same objection about that comment here as in the previous patch.

Explained earlier.

> 
> > @@ -2121,15 +2128,23 @@ t210_bct_get_value(parse_token id, void
> *data,
> > u_int8_t *bct)
> 
> >   	case token_crypto_hash:
> > -		memcpy(data,
> > -		&(bct_ptr->signature.crypto_hash),
> > -		sizeof(nvboot_hash));
> > +		memcpy(data, &(bct_ptr->signature.crypto_hash),
> > +			sizeof(nvboot_hash));
> 
> This feels like an unrelated cleanup patch?
OK. I will revert it.

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html