> -----Original Message----- > From: Stephen Warren [mailto:swarren@xxxxxxxxxxxxx] > Sent: Monday, March 07, 2016 12:16 PM > To: Jimmy Zhang > Cc: Allen Martin; Stephen Warren; alban.bedel@xxxxxxxxxxxxxxxxx; linux- > tegra@xxxxxxxxxxxxxxx > Subject: Re: [tegrarcm PATCH v1 2/4] Add option --ml_rcm <rcm_ml_blob> > > 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 ... > Andrew helped me updating commit messages as below: This option along with "--pkc <keyfile>" allows user to generate signed query version rcm, miniloader rcm and signed bootloader (flasher). With the signed blob, user will then be able to later run tegrarcm on a fused system without needing the actual keyfile. > > 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. > Originally, this option is used to extract and save miniloader rcm to a file. Now, with patch 1/4, the saved rcm contains pkc sig as well. So, I should probably select a different word for this function. How about "--sign"? Then, the option "--signed" in 3/4 becomes too similar. Any suggestion for a better option key word there? > > 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. > OK > > +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()? > OK. > Why use a temporary variable to store strlen(in) but not another to store > strlen(ext); they're both used twice. > OK > > +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. > Ok > > 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()? OK. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
