Re: [tegrarcm PATCH v1 2/4] Add option --ml_rcm <rcm_ml_blob>

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

On 03/04/2016 04:44 PM, Jimmy Zhang wrote:
> This option along with "--pkc <keyfile>" allows user to generate signed
> query version rcm, miniloader rcm and signed bootloader (flasher). With
> these signed blob, user will then be able to run tegrarcm on a fused system
> without keyfile.

That says "without keyfile", yet ...

> Command syntax:
>   $ ./tegrarcm --ml_rcm <ml_rcm_blob> --pkc <keyfile>
>
> Example:
> 1. connect usb cable to recovery mode usb port
> 2. put target in recovery mode
> 3. run command as below:
> $ sudo ./tegrarcm --ml_rcm t124_ml_rcm.bin --pkc rsa_priv.der

... in both those examples, the PKC file is provided as an argument. 
That seems inconsistent.

Oh, having read more of the patch, I see this patch is all about 
*generating* the signed messages, and presumably a later patch is about 
executing them. That's not at all obvious from the patch subject or even 
particularly obvious from the patch descriptions.

Equally, I see that the parameter to --ml_rcm is the base part of a 
filename, to which various extensions will be concatenated to form 
various actual filenames that are written to. This needs to be more 
clearly spelled out in the commit description. The help text:

> +	fprintf(stderr, "\t--ml_rcm=ml_rcm_file\n");
> +	fprintf(stderr, "\t\tSave rcm message prefixed miniloader\n");

... is also not at all clear or illuminating.

I don't see any changes to src/tegrarcm.1 in this series. The man page 
needs to document all the new functionality.

> diff --git a/src/main.c b/src/main.c
> +	/* error check */
> +	if (ml_rcm_file) {
> +		/* ml_rcm option must also come along with pkc option */
> +		if (pkc == NULL) {
> +			fprintf(stderr, "PKC key file must be specified\n");

I would add "if --ml_rcm is" or "with --mc_rcm" or something like that.

> -		usage(argv[0]);
> -		exit(EXIT_FAILURE);
> +		goto usage_exit;

That looks like an unrelated change; a cleanup patch.

> +static int create_name_string(const char *in, char *out, char *ext)

I would suggest re-ordering the parameters so all input and output 
pointers are next to each-other. The existing order is a bit confusing. 
(i.e. in ext out, or out in ext to be more like str/memcpy).

> +	sprintf(out, "%s%s\n", in, ext);
> +	*(out + len + strlen(ext)) = 0x0;

Doesn't that happen automatically since you're using sprintf() not 
snprintf()?

Why use a temporary variable to store strlen(in) but not another to 
store strlen(ext); they're both used twice.

> +static int save_to_file(const char *filename, const uint8_t *msg_buff,
> +			const uint32_t length)
>   {
> +	FILE *raw_file;

f, fp, or file would be more typical variable names; there aren't 
multiple files (e.g. one raw, one not) to differentiate between, so 
"raw_" doesn't seem necessary.

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

> @@ -202,11 +202,12 @@ static int rcm35_sign_msg(uint8_t *buf)
>   		return -EMSGSIZE;
>   	}
>
> +	cmac_hash(msg->reserved, crypto_len, msg->object_sig.cmac_hash);
> +
>   	if (rcm_keyfile)
>   		rsa_pss_sign(rcm_keyfile, msg->reserved, crypto_len,
>   			msg->object_sig.rsa_pss_sig, msg->modulus);
> -	else
> -		cmac_hash(msg->reserved, crypto_len, msg->object_sig.cmac_hash);
> +

This looks like it should be part of patch 1/4? Same for rcm40_sign_msg()?