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

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

In V3 (which actually should have been labelled v4 I think) none of the 
review comments below were addressed.

On 10/07/2015 10:33 AM, Stephen Warren wrote:
> 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).