diff mbox

[tegrarcm,v1,2/4] Add option --ml_rcm <rcm_ml_blob>

Message ID 1457135087-967-3-git-send-email-jimmzhang@nvidia.com
State Superseded, archived
Headers show

Commit Message

jimmzhang March 4, 2016, 11:44 p.m. UTC
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.

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

Signed-off-by: Jimmy Zhang <jimmzhang@nvidia.com>
---
 src/main.c | 194 ++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 src/rcm.c  |   8 +--
 2 files changed, 172 insertions(+), 30 deletions(-)

Comments

Allen Martin March 5, 2016, 1:25 a.m. UTC | #1
On Fri, Mar 04, 2016 at 03:44:45PM -0800, 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.
> 
> 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
> 

Why this extra step to write the signed miniloader to a separate file?
Why not just sign the miniloader in memory when using the --signed
option?  It looks like this is also generating a file for the signed
RCM messages, which should just be done in memory as well like we do
when using CMAC signing.


> +static int initialize_rcm(uint16_t devid, usb_device_t *usb,
> +			const char *keyfile, const char *ml_rcm_file)
> +{
> +	int ret = 0;
>  	uint8_t *msg_buff;
>  	int msg_len;
>  	uint32_t status;
>  	int actual_len;
> +	#define query_rcm_ext	".qry"

Don't need this #define, just use ".qry" directly below


> +static int sign_blob(const char *blob_filename, const char *keyfile)
> +{
> +	int ret;
> +	uint8_t rsa_pss_sig[2048 / 8];
> +
> +	#define sign_ext	".sig"

Here too


> diff --git a/src/rcm.c b/src/rcm.c
> index c7f0f8dddecc..cdf81309ae96 100644
> --- 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);

I don't understand this part, this looks like it undoes what you put
in the previous patch.


> @@ -226,11 +227,10 @@ static int rcm40_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);

Same here

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
jimmzhang March 5, 2016, 2:35 a.m. UTC | #2
> -----Original Message-----
> From: Allen Martin
> Sent: Friday, March 04, 2016 5:25 PM
> To: Jimmy Zhang
> Cc: Stephen Warren; alban.bedel@avionic-design.de; linux-
> tegra@vger.kernel.org
> Subject: Re: [tegrarcm PATCH v1 2/4] Add option --ml_rcm <rcm_ml_blob>
> 
> On Fri, Mar 04, 2016 at 03:44:45PM -0800, 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.
> >
> > 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
> >
> 
> Why this extra step to write the signed miniloader to a separate file?
> Why not just sign the miniloader in memory when using the --signed
> option?  It looks like this is also generating a file for the signed
> RCM messages, which should just be done in memory as well like we do
> when using CMAC signing.
> 
This is for production purpose for fused board. User can run this step to generate all signed blobs
from a secured server. On production server, assuming non secured, user uses previous signed
blobs to download flasher on fused board. By doing so, we can avoid to send rsa keyfile to
production server.

> 
> > +static int initialize_rcm(uint16_t devid, usb_device_t *usb,
> > +			const char *keyfile, const char *ml_rcm_file)
> > +{
> > +	int ret = 0;
> >  	uint8_t *msg_buff;
> >  	int msg_len;
> >  	uint32_t status;
> >  	int actual_len;
> > +	#define query_rcm_ext	".qry"
> 
> Don't need this #define, just use ".qry" directly below
>
OK. Will fix it.
> 
> > +static int sign_blob(const char *blob_filename, const char *keyfile)
> > +{
> > +	int ret;
> > +	uint8_t rsa_pss_sig[2048 / 8];
> > +
> > +	#define sign_ext	".sig"
> 
> Here too
> 
> 
> > diff --git a/src/rcm.c b/src/rcm.c
> > index c7f0f8dddecc..cdf81309ae96 100644
> > --- 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);
> 
> I don't understand this part, this looks like it undoes what you put
> in the previous patch.
> 
User may run this process on unfuse board. In that case, BR still verifies  cmac_hash.
cmac_hash and rsa_pss_sig are in different fields and can coexist.
 
> 
> > @@ -226,11 +227,10 @@ static int rcm40_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);
> 
> Same here

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding March 7, 2016, 8:54 a.m. UTC | #3
On Sat, Mar 05, 2016 at 02:35:51AM +0000, Jimmy Zhang wrote:
> 
> 
> > -----Original Message-----
> > From: Allen Martin
> > Sent: Friday, March 04, 2016 5:25 PM
> > To: Jimmy Zhang
> > Cc: Stephen Warren; alban.bedel@avionic-design.de; linux-
> > tegra@vger.kernel.org
> > Subject: Re: [tegrarcm PATCH v1 2/4] Add option --ml_rcm <rcm_ml_blob>
> > 
> > On Fri, Mar 04, 2016 at 03:44:45PM -0800, 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.
> > >
> > > 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
> > >
> > 
> > Why this extra step to write the signed miniloader to a separate file?
> > Why not just sign the miniloader in memory when using the --signed
> > option?  It looks like this is also generating a file for the signed
> > RCM messages, which should just be done in memory as well like we do
> > when using CMAC signing.
> > 
> This is for production purpose for fused board. User can run this step to generate all signed blobs
> from a secured server. On production server, assuming non secured, user uses previous signed
> blobs to download flasher on fused board. By doing so, we can avoid to send rsa keyfile to
> production server.

I don't like how this makes people jump through hoops to use this
feature. Couldn't we instead implement infrastructure for both
workflows?

For example, the standard behaviour could be to sign everything in
memory, which would allow developers to use this in the most
straightforward way. A command-line option could be added to switch
into "production" mode, where the necessary files are generated and
later used, which would allow the kind of setup that you describe
where the signing and flashing machines are separate.

Thierry
Stephen Warren March 7, 2016, 8:15 p.m. UTC | #4
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()?
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
jimmzhang March 9, 2016, 1:21 a.m. UTC | #5
> -----Original Message-----
> From: Stephen Warren [mailto:swarren@wwwdotorg.org]
> Sent: Monday, March 07, 2016 12:16 PM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; alban.bedel@avionic-design.de; linux-
> tegra@vger.kernel.org
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren March 9, 2016, 5:35 p.m. UTC | #6
On 03/08/2016 06:21 PM, Jimmy Zhang wrote:
> Stephen Warren wrote at Monday, March 07, 2016 12:16 PM:
>> 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.

I'd suggest just the following; that uses "signed blob" without actually 
mentioning that any such thing exists.

This feature allows generation of a signed blob that can later be used 
to communicate with a PKC-enabled Tegra device without access to the 
PKC. The --pkc option is required when generating the blob.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/src/main.c b/src/main.c
index fedeab2e1402..8a5a7680837e 100644
--- a/src/main.c
+++ b/src/main.c
@@ -61,10 +61,16 @@ 
 // tegra124 miniloader
 #include "miniloader/tegra124-miniloader.h"
 
-static int initialize_rcm(uint16_t devid, usb_device_t *usb, const char *keyfile);
-static int initialize_miniloader(uint16_t devid, usb_device_t *usb, char *mlfile, uint32_t mlentry);
+#define FILENAME_MAX_SIZE 256
+
+static int initialize_rcm(uint16_t devid, usb_device_t *usb,
+		 const char *keyfile, const char *ml_rcm_file);
+static int initialize_miniloader(uint16_t devid, usb_device_t *usb,
+		char *mlfile, uint32_t mlentry, const char *ml_rcm_file);
 static int wait_status(nv3p_handle_t h3p);
 static int send_file(nv3p_handle_t h3p, const char *filename);
+static int create_miniloader_rcm(uint8_t *miniloader, uint32_t size,
+			uint32_t entry, const char *ml_rcm_file);
 static int download_miniloader(usb_device_t *usb, uint8_t *miniloader,
 			       uint32_t size, uint32_t entry);
 static void dump_platform_info(nv3p_platform_info_t *info);
@@ -73,6 +79,7 @@  static int download_bootloader(nv3p_handle_t h3p, char *filename,
 			       uint32_t entry, uint32_t loadaddr,
 			       const char *pkc_keyfile);
 static int read_bct(nv3p_handle_t h3p, char *filename);
+static int sign_blob(const char *blob_filename, const char *keyfile);
 
 enum cmdline_opts {
 	OPT_BCT,
@@ -87,6 +94,7 @@  enum cmdline_opts {
 #ifdef HAVE_USB_PORT_MATCH
 	OPT_USBPORTPATH,
 #endif
+	OPT_CREATE_ML_RCM,
 	OPT_END,
 };
 
@@ -129,7 +137,8 @@  static void usage(char *progname)
 	fprintf(stderr, "\t--pkc=<key.ber>\n");
 	fprintf(stderr, "\t\tSpecify the key file for secured devices. The key should be\n");
 	fprintf(stderr, "\t\tin DER format\n");
-
+	fprintf(stderr, "\t--ml_rcm=ml_rcm_file\n");
+	fprintf(stderr, "\t\tSave rcm message prefixed miniloader\n");
 	fprintf(stderr, "\n");
 }
 
@@ -189,6 +198,7 @@  int main(int argc, char **argv)
 	uint8_t match_ports[PORT_MATCH_MAX_PORTS];
 	int match_ports_len;
 #endif
+	char *ml_rcm_file = NULL;
 
 	static struct option long_options[] = {
 		[OPT_BCT]        = {"bct", 1, 0, 0},
@@ -203,6 +213,7 @@  int main(int argc, char **argv)
 #ifdef HAVE_USB_PORT_MATCH
 		[OPT_USBPORTPATH]  = {"usb-port-path", 1, 0, 0},
 #endif
+		[OPT_CREATE_ML_RCM] = {"ml_rcm", 1, 0, 0},
 		[OPT_END]        = {0, 0, 0, 0}
 	};
 
@@ -249,6 +260,9 @@  int main(int argc, char **argv)
 				match_port = true;
 				break;
 #endif
+			case OPT_CREATE_ML_RCM:
+				ml_rcm_file = optarg;
+				break;
 			case OPT_HELP:
 			default:
 				usage(argv[0]);
@@ -268,29 +282,43 @@  int main(int argc, char **argv)
 		else {
 			fprintf(stderr, "%s: Unknown command line argument: %s\n",
 				argv[0], argv[optind]);
-			usage(argv[0]);
-			exit(EXIT_FAILURE);
+			goto usage_exit;
 		}
 		optind++;
 	}
 
-	if (bctfile == NULL) {
+	/* 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");
+			goto usage_exit;
+		}
+
+		/* ml_rcm option must also come along with bootloader option */
+		if (blfile == NULL) {
+			fprintf(stderr, "bootloader file must be specified\n");
+			goto usage_exit;
+		}
+	}
+
+	/* specify bct file if no ml_rcm option */
+	if ((bctfile == NULL) && (ml_rcm_file == NULL)) {
 		fprintf(stderr, "BCT file must be specified\n");
-		usage(argv[0]);
-		exit(EXIT_FAILURE);
+		goto usage_exit;
 	}
-	printf("bct file: %s\n", bctfile);
 
-	if (!do_read) {
+	if (bctfile)
+		printf("bct file: %s\n", bctfile);
+
+	if ((ml_rcm_file == NULL) && !do_read) {
 		if (blfile == NULL) {
 			fprintf(stderr, "bootloader file must be specified\n");
-			usage(argv[0]);
-			exit(EXIT_FAILURE);
+			goto usage_exit;
 		}
 		if (loadaddr == 0) {
 			fprintf(stderr, "loadaddr must be specified\n");
-			usage(argv[0]);
-			exit(EXIT_FAILURE);
+			goto usage_exit;
 		}
 		if (entryaddr == 0) {
 			entryaddr = loadaddr;
@@ -320,17 +348,25 @@  int main(int argc, char **argv)
 			error(1, errno, "USB read truncated");
 
 		// initialize rcm
-		ret2 = initialize_rcm(devid, usb, pkc);
+		ret2 = initialize_rcm(devid, usb, pkc, ml_rcm_file);
 		if (ret2)
 			error(1, errno, "error initializing RCM protocol");
 
-		// download the miniloader to start nv3p
-		ret2 = initialize_miniloader(devid, usb, mlfile, mlentry);
+		// download the miniloader to start nv3p or create ml rcm file
+		ret2 = initialize_miniloader(devid, usb, mlfile, mlentry,
+						ml_rcm_file);
 		if (ret2)
 			error(1, errno, "error initializing miniloader");
 
+		/* if creating ml_rcm, sign bl as well, then exit */
+		if (ml_rcm_file) {
+			sign_blob(blfile, pkc);
+			goto done;
+		}
+
 		// device may have re-enumerated, so reopen USB
 		usb_close(usb);
+
 		usb = usb_open(USB_VENID_NVIDIA, &devid
 #ifdef HAVE_USB_PORT_MATCH
 		, &match_port, &match_bus, match_ports, &match_ports_len
@@ -384,18 +420,59 @@  int main(int argc, char **argv)
 		error(1, ret, "error downloading bootloader: %s", blfile);
 
 	nv3p_close(h3p);
+done:
 	usb_close(usb);
+	return 0;
+
+usage_exit:
+	usage(argv[0]);
+	exit(EXIT_FAILURE);
+}
 
+static int create_name_string(const char *in, char *out, char *ext)
+{
+	int len = strlen(in);
+
+	if ((len + strlen(ext) + 1) >  FILENAME_MAX_SIZE) {
+		fprintf(stderr, "error: name length %zu bytes exceed "
+				"limits for file %s\n",
+			len + strlen(ext) + 1 - FILENAME_MAX_SIZE, in);
+		return -1;
+	}
+	sprintf(out, "%s%s\n", in, ext);
+	*(out + len + strlen(ext)) = 0x0;
 	return 0;
 }
 
-static int initialize_rcm(uint16_t devid, usb_device_t *usb, const char *keyfile)
+static int save_to_file(const char *filename, const uint8_t *msg_buff,
+			const uint32_t length)
 {
-	int ret;
+	FILE *raw_file;
+
+	printf("Create file %s...\n", filename);
+
+	raw_file = fopen(filename, "wb");
+	if (raw_file == NULL) {
+		fprintf(stderr, "Error opening raw file %s.\n", filename);
+		return -1;
+	}
+
+	fwrite(msg_buff, 1, length, raw_file);
+	fclose(raw_file);
+
+	return 0;
+}
+
+static int initialize_rcm(uint16_t devid, usb_device_t *usb,
+			const char *keyfile, const char *ml_rcm_file)
+{
+	int ret = 0;
 	uint8_t *msg_buff;
 	int msg_len;
 	uint32_t status;
 	int actual_len;
+	#define query_rcm_ext	".qry"
+	char query_filename[FILENAME_MAX_SIZE];
 
 	// initialize RCM
 	if ((devid & 0xff) == USB_DEVID_NVIDIA_TEGRA20 ||
@@ -420,6 +497,18 @@  static int initialize_rcm(uint16_t devid, usb_device_t *usb, const char *keyfile
 	// create query version message
 	rcm_create_msg(RCM_CMD_QUERY_RCM_VERSION, NULL, 0, NULL, 0, &msg_buff);
 
+	/* create query version rcm file */
+	if (ml_rcm_file) {
+		ret = create_name_string(ml_rcm_file, query_filename,
+					query_rcm_ext);
+		if (ret)
+			goto done;
+
+		ret = save_to_file(query_filename, msg_buff,
+					rcm_get_msg_len(msg_buff));
+		goto done;
+	}
+
 	// write query version message to device
 	msg_len = rcm_get_msg_len(msg_buff);
 	if (msg_len == 0) {
@@ -429,28 +518,31 @@  static int initialize_rcm(uint16_t devid, usb_device_t *usb, const char *keyfile
 	ret = usb_write(usb, msg_buff, msg_len);
 	if (ret) {
 		fprintf(stderr, "write RCM query version: USB transfer failure\n");
-		return ret;
+		goto done;
 	}
-	free(msg_buff);
-	msg_buff = NULL;
 
 	// read response
 	ret = usb_read(usb, (uint8_t *)&status, sizeof(status), &actual_len);
 	if (ret) {
 		fprintf(stderr, "read RCM query version: USB transfer failure\n");
-		return ret;
+		goto done;
 	}
 	if (actual_len < sizeof(status)) {
 		fprintf(stderr, "read RCM query version: USB read truncated\n");
-		return EIO;
+		ret = EIO;
+		goto done;
 	}
 	printf("RCM version: %d.%d\n", RCM_VERSION_MAJOR(status),
 	       RCM_VERSION_MINOR(status));
 
-	return 0;
+done:
+	if (msg_buff)
+		free(msg_buff);
+	return ret;
 }
 
-static int initialize_miniloader(uint16_t devid, usb_device_t *usb, char *mlfile, uint32_t mlentry)
+static int initialize_miniloader(uint16_t devid, usb_device_t *usb,
+		 char *mlfile, uint32_t mlentry, const char *ml_rcm_file)
 {
 	int fd;
 	struct stat sb;
@@ -504,6 +596,12 @@  static int initialize_miniloader(uint16_t devid, usb_device_t *usb, char *mlfile
 			return ENODEV;
 		}
 	}
+
+	if (ml_rcm_file) {
+		return create_miniloader_rcm(miniloader, miniloader_size,
+				miniloader_entry, ml_rcm_file);
+	}
+
 	printf("downloading miniloader to target at address 0x%x (%d bytes)...\n",
 		miniloader_entry, miniloader_size);
 	ret = download_miniloader(usb, miniloader, miniloader_size,
@@ -629,6 +727,24 @@  fail:
 	return ret;
 }
 
+static int create_miniloader_rcm(uint8_t *miniloader, uint32_t size,
+			uint32_t entry, const char *ml_rcm_file)
+{
+	uint8_t *msg_buff;
+	int ret = 0;
+
+	// create RCM_CMD_DL_MINILOADER blob
+	rcm_create_msg(RCM_CMD_DL_MINILOADER,
+		       (uint8_t *)&entry, sizeof(entry), miniloader, size,
+		       &msg_buff);
+
+	// write to binary file
+	dprintf("Write miniloader rcm to %s\n", ml_rcm_file);
+
+	ret = save_to_file(ml_rcm_file, msg_buff, rcm_get_msg_len(msg_buff));
+	free(msg_buff);
+	return ret;
+}
 
 static int download_miniloader(usb_device_t *usb, uint8_t *miniloader,
 			       uint32_t size, uint32_t entry)
@@ -891,3 +1007,29 @@  static int download_bootloader(nv3p_handle_t h3p, char *filename,
 
 	return 0;
 }
+
+static int sign_blob(const char *blob_filename, const char *keyfile)
+{
+	int ret;
+	uint8_t rsa_pss_sig[2048 / 8];
+
+	#define sign_ext	".sig"
+	char signature_filename[FILENAME_MAX_SIZE];
+
+	ret = rsa_pss_sign_file(keyfile, blob_filename, rsa_pss_sig);
+	if (ret) {
+		fprintf(stderr, "error signing %s with %s\n",
+			blob_filename, keyfile);
+		return ret;
+	}
+
+	/* save signature to blob_filename.sig */
+	ret = create_name_string(blob_filename, signature_filename,
+				sign_ext);
+	if (ret)
+		return ret;
+
+	ret = save_to_file(signature_filename, rsa_pss_sig,
+				sizeof(rsa_pss_sig));
+	return ret;
+}
diff --git a/src/rcm.c b/src/rcm.c
index c7f0f8dddecc..cdf81309ae96 100644
--- 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);
+
 	return 0;
 }
 
@@ -226,11 +227,10 @@  static int rcm40_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);
 
 	return 0;
 }