Re: [tegrarcm PATCH v2 2/4] Add support for update pubkey and rsa-pss signatures

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

On 10/02/2015 02:56 PM, Jimmy Zhang wrote:
> Create new configuration keywords:
>     RsaKeyModulusFile: pubkey modulus
>     RsaPssSigBlFile:   bootloader rsa pss signature
>     RsaPssSigBctFile:  bct rsa pss signature
>
> Sample Configuration file update_bl_sig.cfg
>     RsaKeyModulusFile = pubkey.mod;
>     RsaPssSigBlFile = bl.sig;
>
> where pubkey.mod and bl.sig are files that contain the public key
> modulus and bootloader's rsa-pss signature respectively.
>
> public key modulus and signature are created through utilities
> outside cbootimage.
>
> Command line example:
>   $ cbootimage -s tegra210 -u update_bl_sig.cfg image.bin image.bin-bl-signed
>
> Above three new keywords added in this CL are only implemented support
> for T210.

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

> +#define ARSE_RSA_MAX_MODULUS_SIZE	2048
> +#define ARSE_RSA_PARAM_MAX_BYTES	(ARSE_RSA_MAX_MODULUS_SIZE / 8)

These values are currently defined in the chip-specific location 
src/tNNN/nvboot_bct_tNNN.h, and this patch only removes a single on of 
those 3 copies when creating this new centralized value. At best that 
makes the code inconsistent, and at worst could cause compile errors due 
to multiple definitions.

Instead, can you leave this value in the T210-specific location. More 
related to this below.

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

> +static int parse_rsa_param(build_image_context *context,
> +			parse_token token,
> +			char *rest)
> +{
> +	char filename[MAX_BUFFER];
> +
> +	assert(context != NULL);
> +	assert(rest != NULL);
> +
> +	if (context->generate_bct != 0)
> +		return 0;
> +
> +	/* Parse the file name. */
> +	rest = parse_filename(rest, filename, MAX_BUFFER);
> +	if (rest == NULL)
> +		return 1;

That doesn't look like the correct error check. Rather, other function 
that parse filenames (and other fields) appear to do something like the 
following after parsing all the fields:

         /* Parse the end state. */
         rest = parse_end_state(rest, e_state, MAX_STR_LEN);
         if (rest == NULL)
                 return 1;

I suspect that code should be present in this function too?

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

> +int
> +set_rsa_param(build_image_context *context, parse_token token,
> +		char *filename)
> +{
> +	int	result;
> +	u_int8_t *rsa_storage;	/* Holds the rsa param after reading */
> +	u_int32_t actual_size;	/* In bytes */
> +	file_type rsa_filetype = file_type_bin;
> +
> +        /* Read the image into memory. */
> +	result = read_from_image(filename,
> +				0,
> +				ARSE_RSA_PARAM_MAX_BYTES,
> +				&rsa_storage,
> +				&actual_size,
> +				rsa_filetype);
> +
> +	if (result) {
> +		printf("Error reading file %s.\n", filename);
> +		exit(1);
> +	}
> +
> +	if (actual_size != ARSE_RSA_PARAM_MAX_BYTES) {
> +		printf("Error: invalid size, file %s.\n", filename);
> +		exit(1);
> +        }

I would suggest removing anything from this function that restricts the 
file size to any particular value, since this is generic code, and in 
theory at least each SoC defines its own RSA size parameters.

Looks like an indentation error there too.

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

> @@ -2198,6 +2201,24 @@ 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_rsa_key_modulus:
> +		memcpy(&bct_ptr->key, data, sizeof(nvboot_rsa_key_modulus));
> +		break;
> +
> +	case token_rsa_pss_sig_bl:
> +		/*
> +		 * Update bootloader 0 since there is only one copy
> +		 * of bootloader being built in.
> +		 */
> +		memcpy(&bct_ptr->bootloader[0].signature.rsa_pss_sig,
> +			data, sizeof(nvboot_rsa_pss_sig));
> +		break;
> +
> +	case token_rsa_pss_sig_bct:
> +		memcpy(&bct_ptr->signature.rsa_pss_sig,
> +			data, sizeof(nvboot_rsa_pss_sig));
> +		break;

Instead, I think the file size can be validated here?

Hmm, I don't see a size parameter to this function:-(

Given that, I think that either:

a) Add a size parameter to this function. This has the disadvantage of 
needing to update a bunch of call sites. I don't know how many; perhaps 
this isn't a big deal?

or:

b) Modify set_rsa_param() so that it doesn't check the file size against 
a single global define ARSE_RSA_PARAM_MAX_BYTES, but rather calls a 
function that's implemented in src/t210/*.c that tells it what size the 
token should be.

This would also solve my other objection that currently set_rsa_param() 
assumes that all the RSA-related parameters must be the same size. While 
that is true at present, I'm not sure that it my be true in all cases in 
the future.

This is probably the most flexible, and not too hard to implement.

or:

c) Remove the duplicate defines (e.g. ARSE_RSA_MAX_MODULUS_SIZE) from 
the T114/124/132 files too. This has the disadvantage that if some 
future chip changes the RSA sizes, we'll have to undo this change at 
that time, and switch back to (a) or (b).