Re: [cbootimage PATCH v1 1/8] Enable --update | -u option support for t210

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 09/02/2015 03:19 PM, Jimmy Zhang wrote:
> Added routines to allow updating pkc pubkey and rsa pss signatures.

"pkc" and "pss" should be explained.

> Specifically, added following configuration keywords:
>
>     RsaKeyModulus: set pubkey
>     RsaPssSigBl:   set bootloader rsa pss signature
>     RsaPssSigBct:  set bct rsa pss signature
>
> Sample Configuration file update_bl_sig.cfg
>     RsaKeyModulus = pubkey.mod;
>     RsaPssSigBl = bl.sig;

Oh, RsaKeyModulus is a file not an inline value? Better put "File" or 
"Filename" into the keywords then?

How can the user generate the value for RsaPssSigBct if this tool is 
generating the BCT? Why doesn't this tool generate the signatures itself 
internally?

> Commandline example:
>     $ cbootimage -s tegra210 -u update_bl_sig.cfg image.bin image.bin-bl-signed

This commit appears to be two unrelated changes squashed into one. This 
commit description is entirely unrelated to the commit subject for example.

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

>   	printf("    -u|--update           Copy input image data and update bct\n");
>   	printf("                          configs into new image file.\n");
> -	printf("                          This feature is only for tegra114/124.\n");
> +	printf("                          This feature is only for tegra114/124/210.\n");

I assume this feature will be enabled by default going forward. I think 
it'd be better to rephrase that as "This feature is not supported on 
Tegra20/30".

BTW, is there a particular reason the feature won't work there? If not, 
can we simply enable it for all chips?

>   		if (context->boot_data_version != BOOTDATA_VERSION_T114 &&
> -			context->boot_data_version != BOOTDATA_VERSION_T124) {
> -			printf("Update image feature is only for Tegra114 and Tegra124.\n");
> +			context->boot_data_version != BOOTDATA_VERSION_T124 &&
> +			context->boot_data_version != BOOTDATA_VERSION_T210) {
> +			printf("Update image feature is only for Tegra114, Tegra124"
> +				" and Tegra210.\n");
>   			return -EINVAL;

To avoid that if expanding forever, can it not check for == T20/30 
rather than != all other chips?


> diff --git a/src/cbootimage.h b/src/cbootimage.h

> @@ -105,6 +109,7 @@ typedef struct build_image_context_rec
>   	u_int32_t mts_attr;
>
>   	char *bct_filename;
> +	char *rsa_filename;

RSA *what* filename? This patch introduces 3 different RSA-related 
keywords. A more complete variable name would be useful so it's obvious 
which keyword's value is being stored here. Comments would be useful too.

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

> @@ -116,6 +118,9 @@ static parse_item s_top_level_items[] = {
>   	{ "ChipUid=",       token_unique_chip_id,	parse_value_chipuid },
>   	{ "JtagCtrl=",	    token_secure_jtag_control,	parse_value_u32 },
>   	{ "DebugCtrl=",	    token_secure_debug_control,	parse_value_u32 },
> +	{ "RsaKeyModulus=", token_pub_key_modulus,	parse_rsa_param },

Why "RsaKey" in the keyword name but "pub_key" in the enumeration?

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

> +int
> +set_rsa_param(build_image_context *context, parse_token token,
> +		const char *filename)
...
> +	if (result || (actual_size != ARSE_RSA_PARAM_MAX_BYTES)) {
> +		if (result)
> +			printf("Error reading file %s.\n", filename);
> +		else
> +			printf("Error: invalid size, file %s.\n", filename);
> +		exit(1);
> +        }

Since those are two entirely separate error cases, writing two entirely 
separate if conditions would be better:

if (result) {
     printf
     exit
}
if (actual_size != ... {
     printf
     exit
}

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

> @@ -2198,6 +2201,21 @@ t210_bct_set_value(parse_token id, void *data, u_int8_t *bct)
>   		memcpy(&bct_ptr->unique_chip_id, data, sizeof(nvboot_ecid));
>   		break;
>
> +	case token_pub_key_modulus:
> +		memcpy(&bct_ptr->key, data, sizeof(nvboot_rsa_key_modulus));
> +		break;
> +
> +	case token_rsa_pss_sig_bl:
> +		/* ONLY support one bl */
> +		memcpy(&bct_ptr->bootloader[0].signature.rsa_pss_sig,
> +			data, sizeof(nvboot_rsa_pss_sig));

That comment is unfortunate. Can we not make this keyword an array, so 
that this limitation does not exist? If not, won't this require a 
non-backwards-compatible syntax change when we add support for more than 
one bootloader (which incidentally is a feature I expect will be 
implemented not too far down the road...)