[tegrarcm PATCH v1 0/4] Add flashing support for T124 rsa fused board

[tegrarcm PATCH v1 0/4] Add flashing support for T124 rsa fused board

[Jimmy Zhang]

V1:
1. Use option "--pkc" to sign and download bootloader. This option is
   designed for developer. Patch 0001-Add-option-pkc.patch was originally
   submitted by Alban Bedel.
2. Use option "--ml_rcm" and "--pkc" to sign rcm messages and bootloader.
   This signing only feature is intended for production where signging
   is done at secured server and flashing can be done at different stage and
   site without requiring keyfile being present. 
3. Use option "--signed" to specify and download signed rcm messages.
   This option is used for flashing on fused board. ie, a system with
   security mode enabled.


Jimmy Zhang (4):
  Add option "--pkc"
  Add option --ml_rcm <rcm_ml_blob>
  Add option --signed
  Increate USB timeout value

 src/Makefile.am |   2 +
 src/main.c      | 412 ++++++++++++++++++++++++++++++++++++++++++++++++--------
 src/rcm.c       |  16 ++-
 src/rcm.h       |   3 +-
 src/rsa-pss.cpp | 147 ++++++++++++++++++++
 src/rsa-pss.h   |  46 +++++++
 src/usb.c       |   2 +-
 7 files changed, 567 insertions(+), 61 deletions(-)
 create mode 100644 src/rsa-pss.cpp
 create mode 100644 src/rsa-pss.h

-- 
1.9.1

[tegrarcm PATCH v1 1/4] Add option "--pkc"

[Jimmy Zhang]

Add the support code needed to sign the RCM messages with RSA-PSS as
needed to communicate with secured production devices. This mode is
enabled by passing a key via the --pkc command line argument. If such
a key is set the RCM messages will be signed with it as well as the
bootloader.

Signed-off-by: Alban Bedel <alban.bedel@...>
---
 src/Makefile.am |   2 +
 src/main.c      |  51 ++++++++++++++++----
 src/rcm.c       |  20 ++++++--
 src/rcm.h       |   3 +-
 src/rsa-pss.cpp | 147 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/rsa-pss.h   |  46 ++++++++++++++++++
 6 files changed, 256 insertions(+), 13 deletions(-)
 create mode 100644 src/rsa-pss.cpp
 create mode 100644 src/rsa-pss.h

diff --git a/src/Makefile.am b/src/Makefile.am
index 4b548859e075..3dad0e6d5e72 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -8,6 +8,8 @@ tegrarcm_SOURCES = \
 	nv3p.c \
 	debug.c \
 	rcm.c \
+	rsa-pss.cpp \
+	rsa-pss.h \
 	aes-cmac.cpp \
 	aes-cmac.h \
 	debug.h \
diff --git a/src/main.c b/src/main.c
index 3db0ed8be506..fedeab2e1402 100644
--- a/src/main.c
+++ b/src/main.c
@@ -44,6 +44,7 @@
 #include "nv3p.h"
 #include "nv3p_status.h"
 #include "aes-cmac.h"
+#include "rsa-pss.h"
 #include "rcm.h"
 #include "debug.h"
 #include "config.h"
@@ -60,7 +61,7 @@
 // tegra124 miniloader
 #include "miniloader/tegra124-miniloader.h"
 
-static int initialize_rcm(uint16_t devid, usb_device_t *usb);
+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);
 static int wait_status(nv3p_handle_t h3p);
 static int send_file(nv3p_handle_t h3p, const char *filename);
@@ -69,7 +70,8 @@ static int download_miniloader(usb_device_t *usb, uint8_t *miniloader,
 static void dump_platform_info(nv3p_platform_info_t *info);
 static int download_bct(nv3p_handle_t h3p, char *filename);
 static int download_bootloader(nv3p_handle_t h3p, char *filename,
-			       uint32_t entry, uint32_t loadaddr);
+			       uint32_t entry, uint32_t loadaddr,
+			       const char *pkc_keyfile);
 static int read_bct(nv3p_handle_t h3p, char *filename);
 
 enum cmdline_opts {
@@ -81,6 +83,7 @@ enum cmdline_opts {
 	OPT_VERSION,
 	OPT_MINILOADER,
 	OPT_MINIENTRY,
+	OPT_PKC,
 #ifdef HAVE_USB_PORT_MATCH
 	OPT_USBPORTPATH,
 #endif
@@ -123,6 +126,10 @@ static void usage(char *progname)
 	fprintf(stderr, "\t\tminiloader\n");
 	fprintf(stderr, "\t--miniloader_entry=<mlentry>\n");
 	fprintf(stderr, "\t\tSpecify the entry point for the miniloader\n");
+	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, "\n");
 }
 
@@ -175,6 +182,7 @@ int main(int argc, char **argv)
 	int do_read = 0;
 	char *mlfile = NULL;
 	uint32_t mlentry = 0;
+	char *pkc = NULL;
 #ifdef HAVE_USB_PORT_MATCH
 	bool match_port = false;
 	uint8_t match_bus;
@@ -191,6 +199,7 @@ int main(int argc, char **argv)
 		[OPT_VERSION]    = {"version", 0, 0, 0},
 		[OPT_MINILOADER] = {"miniloader", 1, 0, 0},
 		[OPT_MINIENTRY]  = {"miniloader_entry", 1, 0, 0},
+		[OPT_PKC]        = {"pkc", 1, 0, 0},
 #ifdef HAVE_USB_PORT_MATCH
 		[OPT_USBPORTPATH]  = {"usb-port-path", 1, 0, 0},
 #endif
@@ -229,6 +238,9 @@ int main(int argc, char **argv)
 			case OPT_MINIENTRY:
 				mlentry = strtoul(optarg, NULL, 0);
 				break;
+			case OPT_PKC:
+				pkc = optarg;
+				break;
 #ifdef HAVE_USB_PORT_MATCH
 			case OPT_USBPORTPATH:
 				parse_usb_port_path(argv[0], optarg,
@@ -308,7 +320,7 @@ int main(int argc, char **argv)
 			error(1, errno, "USB read truncated");
 
 		// initialize rcm
-		ret2 = initialize_rcm(devid, usb);
+		ret2 = initialize_rcm(devid, usb, pkc);
 		if (ret2)
 			error(1, errno, "error initializing RCM protocol");
 
@@ -355,6 +367,7 @@ int main(int argc, char **argv)
 	if (info.op_mode != RCM_OP_MODE_DEVEL &&
 	    info.op_mode != RCM_OP_MODE_ODM_OPEN &&
 	    info.op_mode != RCM_OP_MODE_ODM_SECURE &&
+	    info.op_mode != RCM_OP_MODE_ODM_SECURE_PKC &&
 	    info.op_mode != RCM_OP_MODE_PRE_PRODUCTION)
 		error(1, ENODEV, "device is not in developer, open, secure, "
 		      "or pre-production mode, cannot flash");
@@ -366,7 +379,7 @@ int main(int argc, char **argv)
 	}
 
 	// download the bootloader
-	ret = download_bootloader(h3p, blfile, entryaddr, loadaddr);
+	ret = download_bootloader(h3p, blfile, entryaddr, loadaddr, pkc);
 	if (ret)
 		error(1, ret, "error downloading bootloader: %s", blfile);
 
@@ -376,7 +389,7 @@ int main(int argc, char **argv)
 	return 0;
 }
 
-static int initialize_rcm(uint16_t devid, usb_device_t *usb)
+static int initialize_rcm(uint16_t devid, usb_device_t *usb, const char *keyfile)
 {
 	int ret;
 	uint8_t *msg_buff;
@@ -388,13 +401,13 @@ static int initialize_rcm(uint16_t devid, usb_device_t *usb)
 	if ((devid & 0xff) == USB_DEVID_NVIDIA_TEGRA20 ||
 	    (devid & 0xff) == USB_DEVID_NVIDIA_TEGRA30) {
 		dprintf("initializing RCM version 1\n");
-		ret = rcm_init(RCM_VERSION_1);
+		ret = rcm_init(RCM_VERSION_1, keyfile);
 	} else if ((devid & 0xff) == USB_DEVID_NVIDIA_TEGRA114) {
 		dprintf("initializing RCM version 35\n");
-		ret = rcm_init(RCM_VERSION_35);
+		ret = rcm_init(RCM_VERSION_35, keyfile);
 	} else if ((devid & 0xff) == USB_DEVID_NVIDIA_TEGRA124) {
 		dprintf("initializing RCM version 40\n");
-		ret = rcm_init(RCM_VERSION_40);
+		ret = rcm_init(RCM_VERSION_40, keyfile);
 	} else {
 		fprintf(stderr, "unknown tegra device: 0x%x\n", devid);
 		return errno;
@@ -720,6 +733,7 @@ static void dump_platform_info(nv3p_platform_info_t *info)
 	case RCM_OP_MODE_DEVEL:             op_mode = "developer mode"; break;
 	case RCM_OP_MODE_ODM_OPEN:          op_mode = "odm open mode"; break;
 	case RCM_OP_MODE_ODM_SECURE:        op_mode = "odm secure mode"; break;
+	case RCM_OP_MODE_ODM_SECURE_PKC:    op_mode = "odm secure mode with PKC"; break;
 	default:                            op_mode = "unknown"; break;
 	}
 	printf(" (%s)\n", op_mode);
@@ -813,7 +827,8 @@ out:
 }
 
 static int download_bootloader(nv3p_handle_t h3p, char *filename,
-			       uint32_t entry, uint32_t loadaddr)
+			       uint32_t entry, uint32_t loadaddr,
+			       const char *pkc_keyfile)
 {
 	int ret;
 	nv3p_cmd_dl_bl_t arg;
@@ -849,6 +864,24 @@ static int download_bootloader(nv3p_handle_t h3p, char *filename,
 		return ret;
 	}
 
+	// When using PKC the bootloader hash must be sent first
+	if (pkc_keyfile) {
+		uint8_t rsa_pss_sig[2048 / 8];
+
+		ret = rsa_pss_sign_file(pkc_keyfile, filename, rsa_pss_sig);
+		if (ret) {
+			dprintf("error signing %s with %s\n",
+				filename, pkc_keyfile);
+			return ret;
+		}
+
+		ret = nv3p_data_send(h3p, rsa_pss_sig, sizeof(rsa_pss_sig));
+		if (ret) {
+			dprintf("error sending bootloader signature\n");
+			return ret;
+		}
+	}
+
 	// send the bootloader file
 	ret = send_file(h3p, filename);
 	if (ret) {
diff --git a/src/rcm.c b/src/rcm.c
index cb53d8f4f56d..c7f0f8dddecc 100644
--- a/src/rcm.c
+++ b/src/rcm.c
@@ -32,6 +32,7 @@
 #include <errno.h>
 #include "rcm.h"
 #include "aes-cmac.h"
+#include "rsa-pss.h"
 
 static int rcm_sign_msg(uint8_t *buf);
 static int rcm1_sign_msg(uint8_t *buf);
@@ -72,8 +73,9 @@ static uint32_t rcm_get_msg_buf_len(uint32_t payload_len);
 
 static uint32_t rcm_version = 0;
 static uint32_t message_size = 0;
+static const char *rcm_keyfile = NULL;
 
-int rcm_init(uint32_t version)
+int rcm_init(uint32_t version, const char *keyfile)
 {
 	int ret = -EINVAL;
 
@@ -92,6 +94,9 @@ int rcm_init(uint32_t version)
 		message_size = sizeof(rcm40_msg_t);
 		ret = 0;
 	}
+
+	rcm_keyfile = keyfile;
+
 	return ret;
 }
 
@@ -197,7 +202,11 @@ 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;
 }
 
@@ -217,7 +226,12 @@ 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;
 }
 
diff --git a/src/rcm.h b/src/rcm.h
index ab4bea2d8752..fb371b3c9f7a 100644
--- a/src/rcm.h
+++ b/src/rcm.h
@@ -109,8 +109,9 @@ typedef struct {
 #define RCM_OP_MODE_DEVEL           0x3
 #define RCM_OP_MODE_ODM_SECURE      0x4
 #define RCM_OP_MODE_ODM_OPEN        0x5
+#define RCM_OP_MODE_ODM_SECURE_PKC  0x6
 
-int rcm_init(uint32_t version);
+int rcm_init(uint32_t version, const char *keyfile);
 uint32_t rcm_get_msg_len(uint8_t *msg);
 int rcm_create_msg(
 	uint32_t opcode,
diff --git a/src/rsa-pss.cpp b/src/rsa-pss.cpp
new file mode 100644
index 000000000000..d9aa8c604197
--- /dev/null
+++ b/src/rsa-pss.cpp
@@ -0,0 +1,147 @@
+/*
+ * Copyright (c) 2015-1016, Avionic Design GmbH
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *  * Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ *  * Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *  * Neither the name of Avionic Design GmbH nor the names of its
+ *    contributors may be used to endorse or promote products derived
+ *    from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+#include <iostream>
+using std::cout;
+using std::cerr;
+using std::endl;
+
+#include <iomanip>
+using std::hex;
+
+#include <string>
+using std::string;
+
+#include <cstdlib>
+using std::exit;
+
+#include "cryptlib.h"
+using CryptoPP::Exception;
+
+#include "integer.h"
+using CryptoPP::Integer;
+
+#include "files.h"
+using CryptoPP::FileSource;
+
+#include "filters.h"
+using CryptoPP::StringSink;
+using CryptoPP::SignerFilter;
+
+#include "queue.h"
+using CryptoPP::ByteQueue;
+
+#include "rsa.h"
+using CryptoPP::RSA;
+using CryptoPP::RSASS;
+
+#include "pssr.h"
+using CryptoPP::PSS;
+
+#include "sha.h"
+using CryptoPP::SHA256;
+
+#include "secblock.h"
+using CryptoPP::SecByteBlock;
+
+#include "osrng.h"
+using CryptoPP::AutoSeededRandomPool;
+
+#include "rsa-pss.h"
+
+extern "C" int rsa_pss_sign(const char *key_file, const unsigned char *msg,
+			int len, unsigned char *sig_buf, unsigned char *modulus_buf)
+{
+	try {
+		AutoSeededRandomPool rng;
+		FileSource file(key_file, true);
+		RSA::PrivateKey key;
+		ByteQueue bq;
+
+		// Load the key
+		file.TransferTo(bq);
+		bq.MessageEnd();
+		key.BERDecodePrivateKey(bq, false, bq.MaxRetrievable());
+
+		// Write the modulus
+		Integer mod = key.GetModulus();
+		for (int i = 0; i < mod.ByteCount(); i++)
+			modulus_buf[i] = mod.GetByte(i);
+
+		// Sign the message
+		RSASS<PSS, SHA256>::Signer signer(key);
+		size_t length = signer.MaxSignatureLength();
+		SecByteBlock signature(length);
+
+		length = signer.SignMessage(rng, msg, len, signature);
+
+		// Copy in reverse order
+		for (int i = 0; i < length; i++)
+			sig_buf[length - i - 1] = signature[i];
+	}
+	catch(const CryptoPP::Exception& e) {
+		cerr << e.what() << endl;
+		return 1;
+	}
+
+	return 0;
+}
+
+extern "C" int rsa_pss_sign_file(const char *key_file, const char *msg_file,
+			unsigned char *sig_buf)
+{
+	try {
+		AutoSeededRandomPool rng;
+		FileSource file(key_file, true);
+		RSA::PrivateKey key;
+		ByteQueue bq;
+
+		// Load the key
+		file.TransferTo(bq);
+		bq.MessageEnd();
+		key.BERDecodePrivateKey(bq, false, bq.MaxRetrievable());
+
+		// Sign the message
+		RSASS<PSS, SHA256>::Signer signer(key);
+		string signature;
+		FileSource src(msg_file, true,
+			new SignerFilter(rng, signer,
+					new StringSink(signature)));
+		int length = signature.length();
+
+		// Copy in reverse order
+		for (int i = 0; i < length; i++)
+			sig_buf[length - i - 1] = signature[i];
+	}
+	catch(const CryptoPP::Exception& e) {
+		cerr << e.what() << endl;
+		return 1;
+	}
+
+	return 0;
+}
diff --git a/src/rsa-pss.h b/src/rsa-pss.h
new file mode 100644
index 000000000000..39e88c0f12a8
--- /dev/null
+++ b/src/rsa-pss.h
@@ -0,0 +1,46 @@
+/*
+ * Copyright (c) 2015-1016, Avionic Design GmbH
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *  * Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ *  * Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *  * Neither the name of Avionic Design GmbH nor the names of its
+ *    contributors may be used to endorse or promote products derived
+ *    from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+#ifndef _RSA_PSS_H
+#define _RSA_PSS_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+int rsa_pss_sign(const char *key_file, const unsigned char *msg,
+		int len, unsigned char *sig_buf, unsigned char *modulus_buf);
+
+int rsa_pss_sign_file(const char *key_file, const char *msg_file,
+		unsigned char *sig_buf);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif // _RSA_PSS_H
-- 
1.9.1

Re: [tegrarcm PATCH v1 1/4] Add option "--pkc"

[Allen Martin]

On Fri, Mar 04, 2016 at 03:44:44PM -0800, Jimmy Zhang wrote:
> Add the support code needed to sign the RCM messages with RSA-PSS as
> needed to communicate with secured production devices. This mode is
> enabled by passing a key via the --pkc command line argument. If such
> a key is set the RCM messages will be signed with it as well as the
> bootloader.
> 
> Signed-off-by: Alban Bedel <alban.bedel@...>
> ---


> diff --git a/src/main.c b/src/main.c
> index 3db0ed8be506..fedeab2e1402 100644
> --- a/src/main.c
> +++ b/src/main.c

> @@ -123,6 +126,10 @@ static void usage(char *progname)
>  	fprintf(stderr, "\t\tminiloader\n");
>  	fprintf(stderr, "\t--miniloader_entry=<mlentry>\n");
>  	fprintf(stderr, "\t\tSpecify the entry point for the miniloader\n");
> +	fprintf(stderr, "\t--pkc=<key.ber>\n");

.der?
Also "--pkcs" might be more accurate, or even better "--rsa-pss"
Please update man page as well

> +extern "C" int rsa_pss_sign_file(const char *key_file, const char *msg_file,
> +			unsigned char *sig_buf)

Make this function a wrapper around rsa_pss_sign() to avoid the code
duplication. 

-Allen

Re: [tegrarcm PATCH v1 1/4] Add option "--pkc"

[Stephen Warren]

On 03/04/2016 04:44 PM, Jimmy Zhang wrote:
> Add the support code needed to sign the RCM messages with RSA-PSS as
> needed to communicate with secured production devices. This mode is
> enabled by passing a key via the --pkc command line argument. If such
> a key is set the RCM messages will be signed with it as well as the
> bootloader.
>
> Signed-off-by: Alban Bedel <alban.bedel@...>

Part of that s-o-b line has been corrupted.

If Alban wrote this, there should be a "From:" line for Alban at the top 
of the email. Check that "git log" locally shows Alban as the git author 
of the patch, and "git format-patch" will do the right thing automatically.

Your s-o-b line is missing. It needs to be present even for patches you 
didn't author, but are simply passing on.


IIUC, this patch allows the user to interact with a chip with PKC 
enabled, yet without creating a variety of pre-signed binaries/messages. 
How does that relate to other review comments that complained about 
having to create pre-signed binaries/messages?

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

> +	fprintf(stderr, "\t\tSpecify the key file for secured devices. The key should be\n");

s/key/private key/ ?

> @@ -175,6 +182,7 @@ int main(int argc, char **argv)
>   	int do_read = 0;
>   	char *mlfile = NULL;
>   	uint32_t mlentry = 0;
> +	char *pkc = NULL;

s/pkc/pkc_filename/?

> -static int initialize_rcm(uint16_t devid, usb_device_t *usb)
> +static int initialize_rcm(uint16_t devid, usb_device_t *usb, const char *keyfile)

s/keyfile/pkc_filename/? (there could be an SBK file instead/too 
perhaps, and it'd be good to differentiate the two)

A general comment: It'd be good to call this pkc_filename /everywhere/, 
rather than sometimes pkc, sometimes keyfile, sometimes pkc_keyfile, 
etc. (One exception might be rsa-pss.c, since that's generic crypto 
code, not necessarily exclusively used for chip PKC functionality).

> diff --git a/src/rsa-pss.cpp b/src/rsa-pss.cpp

> + * Copyright (c) 2015-1016, Avionic Design GmbH

s/1016/2016/. Same comment in the header file.

> +extern "C" int rsa_pss_sign(const char *key_file, const unsigned char *msg,
> +			int len, unsigned char *sig_buf, unsigned char *modulus_buf)
> +{

Here and in rsa_pss_sign_file(), it would be good to validate that the 
length of the modulus and signature don't exceed the expected size, so 
that this code doesn't write too much data into sig_buf or modulus_buf.

RE: [tegrarcm PATCH v1 1/4] Add option "--pkc"

[Jimmy Zhang]

> -----Original Message-----
> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> Sent: Monday, March 07, 2016 11:56 AM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; alban.bedel-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org; linux-
> tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [tegrarcm PATCH v1 1/4] Add option "--pkc"
> 
> On 03/04/2016 04:44 PM, Jimmy Zhang wrote:
> > Add the support code needed to sign the RCM messages with RSA-PSS as
> > needed to communicate with secured production devices. This mode is
> > enabled by passing a key via the --pkc command line argument. If such
> > a key is set the RCM messages will be signed with it as well as the
> > bootloader.
> >
> > Signed-off-by: Alban Bedel <alban.bedel@...>
> 
> Part of that s-o-b line has been corrupted.
> 
> If Alban wrote this, there should be a "From:" line for Alban at the top of the
> email. Check that "git log" locally shows Alban as the git author of the patch,
> and "git format-patch" will do the right thing automatically.
> 

I tried not making any changes on Alban's patch. Seems you are suggesting me to make minor changes.

> Your s-o-b line is missing. It needs to be present even for patches you didn't
> author, but are simply passing on.
> 

Sure.

> 
> IIUC, this patch allows the user to interact with a chip with PKC
> enabled, yet without creating a variety of pre-signed binaries/messages.
> How does that relate to other review comments that complained about
> having to create pre-signed binaries/messages?
> 

This patch does the work as designed, ie, signing and down loading in one step. Other modes are added in patch 2/4 and 3/4.

> > diff --git a/src/main.c b/src/main.c
> 
> > +	fprintf(stderr, "\t\tSpecify the key file for secured devices. The key
> should be\n");
> 
> s/key/private key/ ?

OK.

> 
> > @@ -175,6 +182,7 @@ int main(int argc, char **argv)
> >   	int do_read = 0;
> >   	char *mlfile = NULL;
> >   	uint32_t mlentry = 0;
> > +	char *pkc = NULL;
> 
> s/pkc/pkc_filename/?

OK. In fact, I made similar changes in patch 2/4.

> 
> > -static int initialize_rcm(uint16_t devid, usb_device_t *usb)
> > +static int initialize_rcm(uint16_t devid, usb_device_t *usb, const char
> *keyfile)
> 
> s/keyfile/pkc_filename/? (there could be an SBK file instead/too
> perhaps, and it'd be good to differentiate the two)
> 

OK

> A general comment: It'd be good to call this pkc_filename /everywhere/,
> rather than sometimes pkc, sometimes keyfile, sometimes pkc_keyfile,
> etc. (One exception might be rsa-pss.c, since that's generic crypto
> code, not necessarily exclusively used for chip PKC functionality).
> 
> > diff --git a/src/rsa-pss.cpp b/src/rsa-pss.cpp
> 
> > + * Copyright (c) 2015-1016, Avionic Design GmbH
> 
> s/1016/2016/. Same comment in the header file.
> 

OK

> > +extern "C" int rsa_pss_sign(const char *key_file, const unsigned char
> *msg,
> > +			int len, unsigned char *sig_buf, unsigned char
> *modulus_buf)
> > +{
> 
> Here and in rsa_pss_sign_file(), it would be good to validate that the
> length of the modulus and signature don't exceed the expected size, so
> that this code doesn't write too much data into sig_buf or modulus_buf.

Re: [tegrarcm PATCH v1 1/4] Add option "--pkc"

[Stephen Warren]

On 03/08/2016 05:50 PM, Jimmy Zhang wrote:
>> Stephen Warren wrote at Monday, March 07, 2016 11:56 AM:
>> On 03/04/2016 04:44 PM, Jimmy Zhang wrote:
>>> Add the support code needed to sign the RCM messages with RSA-PSS as
>>> needed to communicate with secured production devices. This mode is
>>> enabled by passing a key via the --pkc command line argument. If such
>>> a key is set the RCM messages will be signed with it as well as the
>>> bootloader.
>>>
>>> Signed-off-by: Alban Bedel <alban.bedel@...>
>>
>> Part of that s-o-b line has been corrupted.
>>
>> If Alban wrote this, there should be a "From:" line for Alban at the top of the
>> email. Check that "git log" locally shows Alban as the git author of the patch,
>> and "git format-patch" will do the right thing automatically.
>>
>
> I tried not making any changes on Alban's patch. Seems you are suggesting me to make minor changes.

You have changed Alban's patch, and I'm asking you to undo that. If you 
run "git log", you will see that the commit has your name as "Author:" 
whereas it should have Alban's name. Make sure you applied Alban's patch 
using "git am" not "patch", and everything will be correct 
automatically. You can fix this by doing a "git rebase -i" and 
re-applying Alban's patch.

>> Your s-o-b line is missing. It needs to be present even for patches you didn't
>> author, but are simply passing on.
>
> Sure.

There's no need to reply to points you agree with; just make the change 
an include it in V2 along with a changelog entry for it.

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

[Jimmy Zhang]

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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 src/main.c | 194 ++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 src/rcm.c  |   8 +--
 2 files changed, 172 insertions(+), 30 deletions(-)

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;
 }
-- 
1.9.1

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

[Allen Martin]

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

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

[Jimmy Zhang]

> -----Original Message-----
> From: Allen Martin
> Sent: Friday, March 04, 2016 5:25 PM
> To: Jimmy Zhang
> Cc: Stephen Warren; alban.bedel-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org; linux-
> tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.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

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2255 bytes --]

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-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org; linux-
> > tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Stephen Warren]

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()?

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

[Jimmy Zhang]

> -----Original Message-----
> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> Sent: Monday, March 07, 2016 12:16 PM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; alban.bedel-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org; linux-
> tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.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.

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

[Stephen Warren]

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.

[tegrarcm PATCH v1 3/4] Add option --signed

[Jimmy Zhang]

This option allows user to specify and download signed rcm messages and bootloader
to device. This option must come along with option "--miniloader".

Example:
$ sudo ./tegrarcm --miniloader t124_ml_rcm.bin --signed --bct test.bct --bootloader u-boo

Signed-of-by: Jimmy Zhang <jimmzhang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 src/main.c | 221 +++++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 171 insertions(+), 50 deletions(-)

diff --git a/src/main.c b/src/main.c
index 8a5a7680837e..a991ecdf9d8a 100644
--- a/src/main.c
+++ b/src/main.c
@@ -64,23 +64,30 @@
 #define FILENAME_MAX_SIZE 256
 
 static int initialize_rcm(uint16_t devid, usb_device_t *usb,
-		 const char *keyfile, const char *ml_rcm_file);
+		const char *keyfile, const char *ml_rcm_file,
+		const char *ml_file, bool pkc_signed);
 static int initialize_miniloader(uint16_t devid, usb_device_t *usb,
-		char *mlfile, uint32_t mlentry, const char *ml_rcm_file);
+		char *mlfile, uint32_t mlentry, const char *ml_rcm_file,
+		bool pkc_signed);
 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);
+			       uint32_t size, uint32_t entry, bool pkc_signed);
 static void dump_platform_info(nv3p_platform_info_t *info);
 static int download_bct(nv3p_handle_t h3p, char *filename);
 static int download_bootloader(nv3p_handle_t h3p, char *filename,
 			       uint32_t entry, uint32_t loadaddr,
-			       const char *pkc_keyfile);
+			       const char *pkc_keyfile, bool pkc_signed);
 static int read_bct(nv3p_handle_t h3p, char *filename);
 static int sign_blob(const char *blob_filename, const char *keyfile);
 
+static void set_platform_info(nv3p_platform_info_t *info);
+static uint32_t get_op_mode(void);
+
+static nv3p_platform_info_t *g_platform_info = NULL;
+
 enum cmdline_opts {
 	OPT_BCT,
 	OPT_BOOTLOADER,
@@ -95,6 +102,7 @@ enum cmdline_opts {
 	OPT_USBPORTPATH,
 #endif
 	OPT_CREATE_ML_RCM,
+	OPT_SIGNED_ML,
 	OPT_END,
 };
 
@@ -137,6 +145,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--signed\n");
+	fprintf(stderr, "\t\tSpecify rsa signed miniloader\n");
 	fprintf(stderr, "\t--ml_rcm=ml_rcm_file\n");
 	fprintf(stderr, "\t\tSave rcm message prefixed miniloader\n");
 	fprintf(stderr, "\n");
@@ -191,7 +201,7 @@ int main(int argc, char **argv)
 	int do_read = 0;
 	char *mlfile = NULL;
 	uint32_t mlentry = 0;
-	char *pkc = NULL;
+	char *pkc_keyfile = NULL;
 #ifdef HAVE_USB_PORT_MATCH
 	bool match_port = false;
 	uint8_t match_bus;
@@ -199,6 +209,7 @@ int main(int argc, char **argv)
 	int match_ports_len;
 #endif
 	char *ml_rcm_file = NULL;
+	bool pkc_signed = false;
 
 	static struct option long_options[] = {
 		[OPT_BCT]        = {"bct", 1, 0, 0},
@@ -214,6 +225,7 @@ int main(int argc, char **argv)
 		[OPT_USBPORTPATH]  = {"usb-port-path", 1, 0, 0},
 #endif
 		[OPT_CREATE_ML_RCM] = {"ml_rcm", 1, 0, 0},
+		[OPT_SIGNED_ML]    = {"signed", 0, 0, 0},
 		[OPT_END]        = {0, 0, 0, 0}
 	};
 
@@ -250,7 +262,7 @@ int main(int argc, char **argv)
 				mlentry = strtoul(optarg, NULL, 0);
 				break;
 			case OPT_PKC:
-				pkc = optarg;
+				pkc_keyfile = optarg;
 				break;
 #ifdef HAVE_USB_PORT_MATCH
 			case OPT_USBPORTPATH:
@@ -263,6 +275,9 @@ int main(int argc, char **argv)
 			case OPT_CREATE_ML_RCM:
 				ml_rcm_file = optarg;
 				break;
+			case OPT_SIGNED_ML:
+				pkc_signed = true;
+				break;
 			case OPT_HELP:
 			default:
 				usage(argv[0]);
@@ -290,7 +305,7 @@ int main(int argc, char **argv)
 	/* error check */
 	if (ml_rcm_file) {
 		/* ml_rcm option must also come along with pkc option */
-		if (pkc == NULL) {
+		if (pkc_keyfile == NULL) {
 			fprintf(stderr, "PKC key file must be specified\n");
 			goto usage_exit;
 		}
@@ -302,6 +317,18 @@ int main(int argc, char **argv)
 		}
 	}
 
+	/* signed ml must be loaded in from external blob file */
+	if ((pkc_signed == true) && (mlfile == NULL)) {
+		fprintf(stderr, "Signed miniloader file must be specified\n");
+		goto usage_exit;
+	}
+
+	/* if signed already, no pkc keyfile is needed. */
+	if ((pkc_signed == true) && pkc_keyfile) {
+		fprintf(stderr, "No pkc keyfile is needed\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");
@@ -348,19 +375,20 @@ int main(int argc, char **argv)
 			error(1, errno, "USB read truncated");
 
 		// initialize rcm
-		ret2 = initialize_rcm(devid, usb, pkc, ml_rcm_file);
+		ret2 = initialize_rcm(devid, usb, pkc_keyfile,
+					ml_rcm_file, mlfile, pkc_signed);
 		if (ret2)
 			error(1, errno, "error initializing RCM protocol");
 
 		// download the miniloader to start nv3p or create ml rcm file
 		ret2 = initialize_miniloader(devid, usb, mlfile, mlentry,
-						ml_rcm_file);
+						ml_rcm_file, pkc_signed);
 		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);
+			sign_blob(blfile, pkc_keyfile);
 			goto done;
 		}
 
@@ -399,6 +427,7 @@ int main(int argc, char **argv)
 	if (ret)
 		error(1, errno, "wait status after platform info");
 	dump_platform_info(&info);
+	set_platform_info(&info);
 
 	if (info.op_mode != RCM_OP_MODE_DEVEL &&
 	    info.op_mode != RCM_OP_MODE_ODM_OPEN &&
@@ -415,7 +444,8 @@ int main(int argc, char **argv)
 	}
 
 	// download the bootloader
-	ret = download_bootloader(h3p, blfile, entryaddr, loadaddr, pkc);
+	ret = download_bootloader(h3p, blfile, entryaddr, loadaddr,
+				pkc_keyfile, pkc_signed);
 	if (ret)
 		error(1, ret, "error downloading bootloader: %s", blfile);
 
@@ -464,7 +494,8 @@ static int save_to_file(const char *filename, const uint8_t *msg_buff,
 }
 
 static int initialize_rcm(uint16_t devid, usb_device_t *usb,
-			const char *keyfile, const char *ml_rcm_file)
+			const char *keyfile, const char *ml_rcm_file,
+			const char *ml_file, bool pkc_signed)
 {
 	int ret = 0;
 	uint8_t *msg_buff;
@@ -509,6 +540,54 @@ static int initialize_rcm(uint16_t devid, usb_device_t *usb,
 		goto done;
 	}
 
+	/* use signed query_rcm for option --signed */
+	if (pkc_signed == true) {
+		int fd;
+		struct stat sb;
+
+		/* form query_rcm name by adding .qry ext to ml_file */
+		if (ml_file == NULL) {
+			fprintf(stderr, "Missing ml file\n");
+			ret = -1;
+			goto done;
+
+		}
+		ret = create_name_string(ml_file, query_filename,
+					query_rcm_ext);
+		if (ret)
+			goto done;
+
+		printf("Download signed query version rcm from file %s\n",
+			query_filename);
+		free(msg_buff);			// allocated by rcm_create_msg
+		msg_buff = NULL;
+
+		fd = open(query_filename, O_RDONLY, 0);
+		if (fd < 0) {
+			fprintf(stderr, "error opening %s for reading\n",
+				query_filename);
+			return errno;
+		}
+		ret = fstat(fd, &sb);
+		if (ret) {
+			fprintf(stderr, "error on fstat of %s\n",
+				query_filename);
+			return ret;
+		}
+		msg_buff = (uint8_t *)malloc(sb.st_size);
+		if (!msg_buff) {
+			fprintf(stderr, "error allocating %zd bytes for query"
+				" rcm\n", sb.st_size);
+			return errno;
+		}
+		if (read(fd, msg_buff, sb.st_size) != sb.st_size) {
+			fprintf(stderr,"error reading from %s\n",
+				query_filename);
+			free(msg_buff);
+			return errno;
+		}
+	}
+
 	// write query version message to device
 	msg_len = rcm_get_msg_len(msg_buff);
 	if (msg_len == 0) {
@@ -542,7 +621,8 @@ done:
 }
 
 static int initialize_miniloader(uint16_t devid, usb_device_t *usb,
-		 char *mlfile, uint32_t mlentry, const char *ml_rcm_file)
+		char *mlfile, uint32_t mlentry, const char *ml_rcm_file,
+		bool pkc_signed)
 {
 	int fd;
 	struct stat sb;
@@ -552,6 +632,28 @@ static int initialize_miniloader(uint16_t devid, usb_device_t *usb,
 	uint32_t miniloader_entry;
 
 	// use prebuilt miniloader if not loading from a file
+	if ((devid & 0xff) == USB_DEVID_NVIDIA_TEGRA20) {
+		miniloader = miniloader_tegra20;
+		miniloader_size = sizeof(miniloader_tegra20);
+		miniloader_entry = TEGRA20_MINILOADER_ENTRY;
+	} else if ((devid & 0xff) == USB_DEVID_NVIDIA_TEGRA30) {
+		miniloader = miniloader_tegra30;
+		miniloader_size = sizeof(miniloader_tegra30);
+		miniloader_entry = TEGRA30_MINILOADER_ENTRY;
+	} else if ((devid & 0xff) == USB_DEVID_NVIDIA_TEGRA114) {
+		miniloader = miniloader_tegra114;
+		miniloader_size = sizeof(miniloader_tegra114);
+		miniloader_entry = TEGRA114_MINILOADER_ENTRY;
+	} else if ((devid & 0xff) == USB_DEVID_NVIDIA_TEGRA124) {
+		miniloader = miniloader_tegra124;
+		miniloader_size = sizeof(miniloader_tegra124);
+		miniloader_entry = TEGRA124_MINILOADER_ENTRY;
+	} else {
+		fprintf(stderr, "unknown tegra device: 0x%x\n", devid);
+		return ENODEV;
+	}
+
+	// if loading from a file
 	if (mlfile) {
 		fd = open(mlfile, O_RDONLY, 0);
 		if (fd < 0) {
@@ -573,30 +675,13 @@ static int initialize_miniloader(uint16_t devid, usb_device_t *usb,
 			dprintf("error reading from miniloader file");
 			return errno;
 		}
-		miniloader_entry = mlentry;
-	} else {
-		if ((devid & 0xff) == USB_DEVID_NVIDIA_TEGRA20) {
-			miniloader = miniloader_tegra20;
-			miniloader_size = sizeof(miniloader_tegra20);
-			miniloader_entry = TEGRA20_MINILOADER_ENTRY;
-		} else if ((devid & 0xff) == USB_DEVID_NVIDIA_TEGRA30) {
-			miniloader = miniloader_tegra30;
-			miniloader_size = sizeof(miniloader_tegra30);
-			miniloader_entry = TEGRA30_MINILOADER_ENTRY;
-		} else if ((devid & 0xff) == USB_DEVID_NVIDIA_TEGRA114) {
-			miniloader = miniloader_tegra114;
-			miniloader_size = sizeof(miniloader_tegra114);
-			miniloader_entry = TEGRA114_MINILOADER_ENTRY;
-		} else if ((devid & 0xff) == USB_DEVID_NVIDIA_TEGRA124) {
-			miniloader = miniloader_tegra124;
-			miniloader_size = sizeof(miniloader_tegra124);
-			miniloader_entry = TEGRA124_MINILOADER_ENTRY;
-		} else {
-			fprintf(stderr, "unknown tegra device: 0x%x\n", devid);
-			return ENODEV;
-		}
 	}
 
+	// if entry is specified
+	if (mlentry)
+		miniloader_entry = mlentry;
+
+	// if creating rcm file
 	if (ml_rcm_file) {
 		return create_miniloader_rcm(miniloader, miniloader_size,
 				miniloader_entry, ml_rcm_file);
@@ -605,7 +690,7 @@ static int initialize_miniloader(uint16_t devid, usb_device_t *usb,
 	printf("downloading miniloader to target at address 0x%x (%d bytes)...\n",
 		miniloader_entry, miniloader_size);
 	ret = download_miniloader(usb, miniloader, miniloader_size,
-				  miniloader_entry);
+				  miniloader_entry, pkc_signed);
 	if (ret) {
 		fprintf(stderr, "Error downloading miniloader\n");
 		return ret;
@@ -747,7 +832,7 @@ static int create_miniloader_rcm(uint8_t *miniloader, uint32_t size,
 }
 
 static int download_miniloader(usb_device_t *usb, uint8_t *miniloader,
-			       uint32_t size, uint32_t entry)
+			       uint32_t size, uint32_t entry, bool pkc_signed)
 {
 	uint8_t *msg_buff;
 	int ret;
@@ -755,9 +840,13 @@ static int download_miniloader(usb_device_t *usb, uint8_t *miniloader,
 	int actual_len;
 
 	// download the miniloader to the bootrom
-	rcm_create_msg(RCM_CMD_DL_MINILOADER,
-		       (uint8_t *)&entry, sizeof(entry), miniloader, size,
-		       &msg_buff);
+	if (pkc_signed == true)		/* signed ml contains rcm message */
+		msg_buff = miniloader;
+	else
+		rcm_create_msg(RCM_CMD_DL_MINILOADER,
+			       (uint8_t *)&entry, sizeof(entry), miniloader, size,
+			       &msg_buff);
+
 	ret = usb_write(usb, msg_buff, rcm_get_msg_len(msg_buff));
 	if (ret)
 		goto fail;
@@ -944,7 +1033,7 @@ out:
 
 static int download_bootloader(nv3p_handle_t h3p, char *filename,
 			       uint32_t entry, uint32_t loadaddr,
-			       const char *pkc_keyfile)
+			       const char *pkc_keyfile, bool pkc_signed)
 {
 	int ret;
 	nv3p_cmd_dl_bl_t arg;
@@ -980,18 +1069,36 @@ static int download_bootloader(nv3p_handle_t h3p, char *filename,
 		return ret;
 	}
 
-	// When using PKC the bootloader hash must be sent first
-	if (pkc_keyfile) {
-		uint8_t rsa_pss_sig[2048 / 8];
+	// For fused board, the bootloader hash must be sent first
+	if (get_op_mode() == RCM_OP_MODE_ODM_SECURE_PKC) {
+		/* sign and download */
+		if (pkc_keyfile)  {
+			uint8_t rsa_pss_sig[2048 / 8];
+
+			ret = rsa_pss_sign_file(pkc_keyfile, filename, rsa_pss_sig);
+			if (ret) {
+				dprintf("error signing %s with %s\n",
+					filename, pkc_keyfile);
+				return ret;
+			}
 
-		ret = rsa_pss_sign_file(pkc_keyfile, filename, rsa_pss_sig);
-		if (ret) {
-			dprintf("error signing %s with %s\n",
-				filename, pkc_keyfile);
-			return ret;
+			ret = nv3p_data_send(h3p, rsa_pss_sig, sizeof(rsa_pss_sig));
+		}
+
+		/* download pkc signature */
+		if (pkc_signed) {
+			#define sign_ext	".sig"
+			char signature_filename[FILENAME_MAX_SIZE];
+
+			ret = create_name_string(filename, signature_filename,
+						sign_ext);
+			if (ret)
+				return ret;
+
+			// send the bootloader file
+			ret = send_file(h3p, signature_filename);
 		}
 
-		ret = nv3p_data_send(h3p, rsa_pss_sig, sizeof(rsa_pss_sig));
 		if (ret) {
 			dprintf("error sending bootloader signature\n");
 			return ret;
@@ -1033,3 +1140,17 @@ static int sign_blob(const char *blob_filename, const char *keyfile)
 				sizeof(rsa_pss_sig));
 	return ret;
 }
+
+static void set_platform_info(nv3p_platform_info_t *info)
+{
+	g_platform_info = info;
+}
+
+static uint32_t get_op_mode(void)
+{
+	if (g_platform_info)
+		return g_platform_info->op_mode;
+
+	fprintf(stderr, "Error: No platform info has been retrieved\n");
+	return 0;
+}
-- 
1.9.1

Re: [tegrarcm PATCH v1 3/4] Add option --signed

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 643 bytes --]

On Fri, Mar 04, 2016 at 03:44:46PM -0800, Jimmy Zhang wrote:
> This option allows user to specify and download signed rcm messages and bootloader
> to device. This option must come along with option "--miniloader".
> 
> Example:
> $ sudo ./tegrarcm --miniloader t124_ml_rcm.bin --signed --bct test.bct --bootloader u-boo

The t124_ml_rcm.bin file is the one generated by passing --ml_rcm,
right? Can't we determine automatically that this file is signed? Also,
could we sanity check that the device we're trying to bootstrap is in
production mode and abort early (or warn) if we're trying to upload a
non-signed binary?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [tegrarcm PATCH v1 3/4] Add option --signed

[Stephen Warren]

On 03/04/2016 04:44 PM, Jimmy Zhang wrote:
> This option allows user to specify and download signed rcm messages and bootloader
> to device. This option must come along with option "--miniloader".
>
> Example:
> $ sudo ./tegrarcm --miniloader t124_ml_rcm.bin --signed --bct test.bct --bootloader u-boo

I won't review this patch in detail since I expect it will change quite 
a bit to implement 3 modes of operation:

a) Create signed files, don't interact with HW.
b) Read signed files, send them to HW.
c) Sign data on-the-fly, while sending it to HW.

RE: [tegrarcm PATCH v1 3/4] Add option --signed

[Jimmy Zhang]

> -----Original Message-----
> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> Sent: Monday, March 07, 2016 12:32 PM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; alban.bedel-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org; linux-
> tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [tegrarcm PATCH v1 3/4] Add option --signed
> 
> On 03/04/2016 04:44 PM, Jimmy Zhang wrote:
> > This option allows user to specify and download signed rcm messages
> > and bootloader to device. This option must come along with option "--
> miniloader".
> >
> > Example:
> > $ sudo ./tegrarcm --miniloader t124_ml_rcm.bin --signed --bct test.bct
> > --bootloader u-boo
> 
> I won't review this patch in detail since I expect it will change quite a bit to
> implement 3 modes of operation:
> 

All three modes are in place.

> a) Create signed files, don't interact with HW.

This is patch 2/4. Command syntax:
$ sudo ./tegrarcm --ml_rcm <ml> --pkc <keyfile> --bootloader <bootloader>

User still needs to put device in recovery mode so that tegrarcm can detect and figure out what soc. Otherwise, we need to add one more parameter for soc.

> b) Read signed files, send them to HW.

This is patch 3/4. Command syntax:
$ sudo ./tegrarcm --miniloader <signed_ml> --signed --bct <bct> --bootloader <bootloader> --loadaddr <addr> 

> c) Sign data on-the-fly, while sending it to HW.

This is patch 1/4. Command syntax:
$ sudo ./tegrarcm --pkc <keyfile> --bct <bct> --bootloader <bootloader> --loadaddr <addr>

Re: [tegrarcm PATCH v1 3/4] Add option --signed

[Stephen Warren]

On 03/08/2016 05:36 PM, Jimmy Zhang wrote:
>
>
>> -----Original Message-----
>> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
>> Sent: Monday, March 07, 2016 12:32 PM
>> To: Jimmy Zhang
>> Cc: Allen Martin; Stephen Warren; alban.bedel-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org; linux-
>> tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [tegrarcm PATCH v1 3/4] Add option --signed
>>
>> On 03/04/2016 04:44 PM, Jimmy Zhang wrote:
>>> This option allows user to specify and download signed rcm messages
>>> and bootloader to device. This option must come along with option "--
>> miniloader".
>>>
>>> Example:
>>> $ sudo ./tegrarcm --miniloader t124_ml_rcm.bin --signed --bct test.bct
>>> --bootloader u-boo
>>
>> I won't review this patch in detail since I expect it will change quite a bit to
>> implement 3 modes of operation:
>>
>
> All three modes are in place.
>
>> a) Create signed files, don't interact with HW.
>
> This is patch 2/4. Command syntax:
> $ sudo ./tegrarcm --ml_rcm <ml> --pkc <keyfile> --bootloader <bootloader>
>
> User still needs to put device in recovery mode so that tegrarcm can detect and figure out what soc. Otherwise, we need to add one more parameter for soc.
>
>> b) Read signed files, send them to HW.
>
> This is patch 3/4. Command syntax:
> $ sudo ./tegrarcm --miniloader <signed_ml> --signed --bct <bct> --bootloader <bootloader> --loadaddr <addr>
>
>> c) Sign data on-the-fly, while sending it to HW.
>
> This is patch 1/4. Command syntax:
> $ sudo ./tegrarcm --pkc <keyfile> --bct <bct> --bootloader <bootloader> --loadaddr <addr>

OK. Updating the documentation would be useful to make this clear.

I don't like describing the file that contains signed data as a 
miniloader. Doesn't the file contain much more than the miniloader 
(IIUC, all the RCM messages need to be signed, so presumably we need to 
pre-calculate and store all RCM messages to avoid tegrarcm needing 
access to the PKC which is the whole point of this mode of operation)? I 
would like to see the --miniloader option reserved for the case where we 
allow the user to supply an alternative (plain unsigned, no header) 
miniloader binary instead of the built-in binary.

As I probably mentioned before, the naming of --ml_rcm isn't great.

I don't like the fact that the operational mode is derived from the set 
of command-line arguments. I'd like the default to be to interact with 
HW, perform signatures if required, and download data to the HW. I'd 
prefer the other modes to be explicitly requested so it's clear what the 
tool will do; perhaps something like:

download unsigned:
tegrarcm --bootloader <bl> --loadaddr <addr>

download with auto-signing:
tegrarcm --bootloader <bl> --loadaddr <addr> --pkc <pkc>

generate signed messages:
tegrarcm --gen-signed-msgs --signed-msgs-file msgs.bin \
     --bootloader <bl> --loadaddr <addr> --pkc <pkc>

execute signed messages:
tegrarcm --send-signed-msgs --signed-msg-file msgs.bin

It also seems useful for the "generate signed messages" mode of 
operation to work without access to any HW. If we have a separate 
factory (that programs HW) and signature server (that generates the 
signed message file for the factory to use), I don't see why the 
signature server system should need a Tegra device attached just to 
calculate the messages to send to the HW. Presumably this simply means 
passing a --soc option when using the --gen-signed-msgs flag? Or is 
there more needed beyond just the SoC type?

RE: [tegrarcm PATCH v1 3/4] Add option --signed

[Jimmy Zhang]

> -----Original Message-----
> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> Sent: Wednesday, March 09, 2016 9:29 AM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; alban.bedel-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org; linux-
> tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [tegrarcm PATCH v1 3/4] Add option --signed
> 
> On 03/08/2016 05:36 PM, Jimmy Zhang wrote:
> >
> >
> >> -----Original Message-----
> >> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> >> Sent: Monday, March 07, 2016 12:32 PM
> >> To: Jimmy Zhang
> >> Cc: Allen Martin; Stephen Warren; alban.bedel-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org;
> >> linux- tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> Subject: Re: [tegrarcm PATCH v1 3/4] Add option --signed
> >>
> >> On 03/04/2016 04:44 PM, Jimmy Zhang wrote:
> >>> This option allows user to specify and download signed rcm messages
> >>> and bootloader to device. This option must come along with option
> >>> "--
> >> miniloader".
> >>>
> >>> Example:
> >>> $ sudo ./tegrarcm --miniloader t124_ml_rcm.bin --signed --bct
> >>> test.bct --bootloader u-boo
> >>
> >> I won't review this patch in detail since I expect it will change
> >> quite a bit to implement 3 modes of operation:
> >>
> >
> > All three modes are in place.
> >
> >> a) Create signed files, don't interact with HW.
> >
> > This is patch 2/4. Command syntax:
> > $ sudo ./tegrarcm --ml_rcm <ml> --pkc <keyfile> --bootloader
> > <bootloader>
> >
> > User still needs to put device in recovery mode so that tegrarcm can detect
> and figure out what soc. Otherwise, we need to add one more parameter for
> soc.
> >
> >> b) Read signed files, send them to HW.
> >
> > This is patch 3/4. Command syntax:
> > $ sudo ./tegrarcm --miniloader <signed_ml> --signed --bct <bct>
> > --bootloader <bootloader> --loadaddr <addr>
> >
> >> c) Sign data on-the-fly, while sending it to HW.
> >
> > This is patch 1/4. Command syntax:
> > $ sudo ./tegrarcm --pkc <keyfile> --bct <bct> --bootloader
> > <bootloader> --loadaddr <addr>
> 
> OK. Updating the documentation would be useful to make this clear.
> 
> I don't like describing the file that contains signed data as a miniloader.
> Doesn't the file contain much more than the miniloader (IIUC, all the RCM
> messages need to be signed, so presumably we need to pre-calculate and
> store all RCM messages to avoid tegrarcm needing access to the PKC which is
> the whole point of this mode of operation)? I would like to see the --
> miniloader option reserved for the case where we allow the user to supply
> an alternative (plain unsigned, no header) miniloader binary instead of the
> built-in binary.
> 
> As I probably mentioned before, the naming of --ml_rcm isn't great.
> 
> I don't like the fact that the operational mode is derived from the set of
> command-line arguments. I'd like the default to be to interact with HW,
> perform signatures if required, and download data to the HW. I'd prefer the
> other modes to be explicitly requested so it's clear what the tool will do;
> perhaps something like:
> 
> download unsigned:
> tegrarcm --bootloader <bl> --loadaddr <addr>
> 
> download with auto-signing:
> tegrarcm --bootloader <bl> --loadaddr <addr> --pkc <pkc>
> 
> generate signed messages:
> tegrarcm --gen-signed-msgs --signed-msgs-file msgs.bin \
>      --bootloader <bl> --loadaddr <addr> --pkc <pkc>
> 
The signed messages include
a) query version rcm
b) download miniloader rcm
c) bl signature

During flashing, tegrarcm needs to down load these three blobs as independent binary to target at predefined flashing phase. Currently I use option "--ml_rcm" and "--bootloader" to derive filenames for these three blobs. If using one file for all, we have to come up a mechanism to pack them together during signing and unpack them when flashing. I agree with your command line parameter. But, I still prefer to create separate message files. For example, if I have a command as below:

 tegrarcm --gen-signed-msgs  --signed-msgs-file rel_1001.bin \
      --bootloader <bl> --loadaddr <addr> --pkc <pkc>

I prefer to actually create files 
a) rel_1001.bin.qry for signed query version rcm
b) rel_1001.bin.ml for signed download miniloader rcm
c) rel_1001.bin.bl for bootloader's 256 bytes rsa_pss signature

User should have doc to trace what key_file, bootloader (flasher) are used for rel_1001

> execute signed messages:
> tegrarcm --send-signed-msgs --signed-msg-file msgs.bin
> 
> It also seems useful for the "generate signed messages" mode of operation
> to work without access to any HW. If we have a separate factory (that
> programs HW) and signature server (that generates the signed message file
> for the factory to use), I don't see why the signature server system should
> need a Tegra device attached just to calculate the messages to send to the
> HW. Presumably this simply means passing a --soc option when using the --
> gen-signed-msgs flag? Or is there more needed beyond just the SoC type?

Yes, SoC type is the only thing needed for generating correct version of rcm messages.

Re: [tegrarcm PATCH v1 3/4] Add option --signed

[Stephen Warren]

On 03/09/2016 02:01 PM, Jimmy Zhang wrote:
>
>
>> -----Original Message-----
>> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
>> Sent: Wednesday, March 09, 2016 9:29 AM
>> To: Jimmy Zhang
>> Cc: Allen Martin; Stephen Warren; alban.bedel-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org; linux-
>> tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [tegrarcm PATCH v1 3/4] Add option --signed
>>
>> On 03/08/2016 05:36 PM, Jimmy Zhang wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
>>>> Sent: Monday, March 07, 2016 12:32 PM
>>>> To: Jimmy Zhang
>>>> Cc: Allen Martin; Stephen Warren; alban.bedel-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org;
>>>> linux- tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>> Subject: Re: [tegrarcm PATCH v1 3/4] Add option --signed
>>>>
>>>> On 03/04/2016 04:44 PM, Jimmy Zhang wrote:
>>>>> This option allows user to specify and download signed rcm messages
>>>>> and bootloader to device. This option must come along with option
>>>>> "--
>>>> miniloader".
>>>>>
>>>>> Example:
>>>>> $ sudo ./tegrarcm --miniloader t124_ml_rcm.bin --signed --bct
>>>>> test.bct --bootloader u-boo
>>>>
>>>> I won't review this patch in detail since I expect it will change
>>>> quite a bit to implement 3 modes of operation:
>>>>
>>>
>>> All three modes are in place.
>>>
>>>> a) Create signed files, don't interact with HW.
>>>
>>> This is patch 2/4. Command syntax:
>>> $ sudo ./tegrarcm --ml_rcm <ml> --pkc <keyfile> --bootloader
>>> <bootloader>
>>>
>>> User still needs to put device in recovery mode so that tegrarcm can detect
>> and figure out what soc. Otherwise, we need to add one more parameter for
>> soc.
>>>
>>>> b) Read signed files, send them to HW.
>>>
>>> This is patch 3/4. Command syntax:
>>> $ sudo ./tegrarcm --miniloader <signed_ml> --signed --bct <bct>
>>> --bootloader <bootloader> --loadaddr <addr>
>>>
>>>> c) Sign data on-the-fly, while sending it to HW.
>>>
>>> This is patch 1/4. Command syntax:
>>> $ sudo ./tegrarcm --pkc <keyfile> --bct <bct> --bootloader
>>> <bootloader> --loadaddr <addr>
>>
>> OK. Updating the documentation would be useful to make this clear.
>>
>> I don't like describing the file that contains signed data as a miniloader.
>> Doesn't the file contain much more than the miniloader (IIUC, all the RCM
>> messages need to be signed, so presumably we need to pre-calculate and
>> store all RCM messages to avoid tegrarcm needing access to the PKC which is
>> the whole point of this mode of operation)? I would like to see the --
>> miniloader option reserved for the case where we allow the user to supply
>> an alternative (plain unsigned, no header) miniloader binary instead of the
>> built-in binary.
>>
>> As I probably mentioned before, the naming of --ml_rcm isn't great.
>>
>> I don't like the fact that the operational mode is derived from the set of
>> command-line arguments. I'd like the default to be to interact with HW,
>> perform signatures if required, and download data to the HW. I'd prefer the
>> other modes to be explicitly requested so it's clear what the tool will do;
>> perhaps something like:
>>
>> download unsigned:
>> tegrarcm --bootloader <bl> --loadaddr <addr>
>>
>> download with auto-signing:
>> tegrarcm --bootloader <bl> --loadaddr <addr> --pkc <pkc>
>>
>> generate signed messages:
>> tegrarcm --gen-signed-msgs --signed-msgs-file msgs.bin \
>>       --bootloader <bl> --loadaddr <addr> --pkc <pkc>
>>
> The signed messages include
> a) query version rcm
> b) download miniloader rcm
> c) bl signature
>
> During flashing, tegrarcm needs to down load these three blobs as independent binary to target at predefined flashing phase. Currently I use option "--ml_rcm" and "--bootloader" to derive filenames for these three blobs. If using one file for all, we have to come up a mechanism to pack them together during signing and unpack them when flashing. I agree with your command line parameter. But, I still prefer to create separate message files. For example, if I have a command as below:
>
>   tegrarcm --gen-signed-msgs  --signed-msgs-file rel_1001.bin \
>        --bootloader <bl> --loadaddr <addr> --pkc <pkc>
>
> I prefer to actually create files
> a) rel_1001.bin.qry for signed query version rcm
> b) rel_1001.bin.ml for signed download miniloader rcm
> c) rel_1001.bin.bl for bootloader's 256 bytes rsa_pss signature
>
> User should have doc to trace what key_file, bootloader (flasher) are used for rel_1001

That seems fine.

[tegrarcm PATCH v1 4/4] Increate USB timeout value

[Jimmy Zhang]

It has been observed that some times USB time out could occure when a long (3ft)
usb cable is connected to recovery mode usb port

Signed-off-by: Jimmy Zhang <jimmzhang>
---
 src/usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/usb.c b/src/usb.c
index 8a367426b29c..4f7d1cae9d3a 100644
--- a/src/usb.c
+++ b/src/usb.c
@@ -35,7 +35,7 @@
 #include "debug.h"
 
 // USB xfer timeout in ms
-#define USB_TIMEOUT 1000
+#define USB_TIMEOUT 5000
 
 #define USB_XFER_MAX 4096
 
-- 
1.9.1

Re: [tegrarcm PATCH v1 4/4] Increate USB timeout value

[Allen Martin]

On Fri, Mar 04, 2016 at 03:44:47PM -0800, Jimmy Zhang wrote:
> It has been observed that some times USB time out could occure when a long (3ft)
> usb cable is connected to recovery mode usb port
> 

How about making it a command line argument instead, so next time
someone tries a 6ft cable we don't need another patch? :^)

-Allen

RE: [tegrarcm PATCH v1 4/4] Increate USB timeout value

[Jimmy Zhang]

> -----Original Message-----
> From: Allen Martin
> Sent: Friday, March 04, 2016 5:47 PM
> To: Jimmy Zhang
> Cc: Stephen Warren; alban.bedel-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org; linux-
> tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [tegrarcm PATCH v1 4/4] Increate USB timeout value
> 
> On Fri, Mar 04, 2016 at 03:44:47PM -0800, Jimmy Zhang wrote:
> > It has been observed that some times USB time out could occure when a
> > long (3ft) usb cable is connected to recovery mode usb port
> >
> 
> How about making it a command line argument instead, so next time
> someone tries a 6ft cable we don't need another patch? :^)
> 
Agree. Will make a change.

> -Allen

Re: [tegrarcm PATCH v1 4/4] Increate USB timeout value

[Stephen Warren]

On 03/04/2016 04:44 PM, Jimmy Zhang wrote:
> It has been observed that some times USB time out could occure when a long (3ft)
> usb cable is connected to recovery mode usb port

This explanation makes no sense. 3ft is practically the shortest USB 
cable anyone would use. Equally, assuming a compliant correctly 
functioning cable, cable length doesn't affect the time it takes to 
execute transactions.

Is the issue more that if a low quality cable is used, then there are 
lots of transfer errors, and the time taken for retries is large? If so, 
there's not much guarantee that a larger timeout would help in general, 
since there's no guarantee of any upper bound on the time it takes for a 
packet not to get corrupted in this case. Still, I suppose it's fine to 
increase the timeout to account for when it accidentally works.

In summary: a commit description that more accurately represents the 
problem being solved is required.

RE: [tegrarcm PATCH v1 4/4] Increate USB timeout value

[Jimmy Zhang]

> -----Original Message-----
> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> Sent: Monday, March 07, 2016 11:40 AM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; alban.bedel-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org; linux-
> tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [tegrarcm PATCH v1 4/4] Increate USB timeout value
> 
> On 03/04/2016 04:44 PM, Jimmy Zhang wrote:
> > It has been observed that some times USB time out could occure when a
> > long (3ft) usb cable is connected to recovery mode usb port
> 
> This explanation makes no sense. 3ft is practically the shortest USB cable
> anyone would use. Equally, assuming a compliant correctly functioning cable,
> cable length doesn't affect the time it takes to execute transactions.
> 
> Is the issue more that if a low quality cable is used, then there are lots of
> transfer errors, and the time taken for retries is large? If so, there's not much
> guarantee that a larger timeout would help in general, since there's no
> guarantee of any upper bound on the time it takes for a packet not to get
> corrupted in this case. Still, I suppose it's fine to increase the timeout to
> account for when it accidentally works.
> 
> In summary: a commit description that more accurately represents the
> problem being solved is required.

We have seen this problem since T30/T114. Initially we just retried by downloading again. It may work after a few tries. Later, we found a simple solution is replacing with a shorter cable. Then one day, for testing purpose, when the timeout value was replaced with 0, ie, unlimited timeout, all cables work fine. So, it may worth to figure out the root cause.
   

Re: [tegrarcm PATCH v1 4/4] Increate USB timeout value

[Stephen Warren]

On 03/08/2016 06:41 PM, Jimmy Zhang wrote:
>
>
>> -----Original Message-----
>> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
>> Sent: Monday, March 07, 2016 11:40 AM
>> To: Jimmy Zhang
>> Cc: Allen Martin; Stephen Warren; alban.bedel-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org; linux-
>> tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [tegrarcm PATCH v1 4/4] Increate USB timeout value
>>
>> On 03/04/2016 04:44 PM, Jimmy Zhang wrote:
>>> It has been observed that some times USB time out could occure when a
>>> long (3ft) usb cable is connected to recovery mode usb port
>>
>> This explanation makes no sense. 3ft is practically the shortest USB cable
>> anyone would use. Equally, assuming a compliant correctly functioning cable,
>> cable length doesn't affect the time it takes to execute transactions.
>>
>> Is the issue more that if a low quality cable is used, then there are lots of
>> transfer errors, and the time taken for retries is large? If so, there's not much
>> guarantee that a larger timeout would help in general, since there's no
>> guarantee of any upper bound on the time it takes for a packet not to get
>> corrupted in this case. Still, I suppose it's fine to increase the timeout to
>> account for when it accidentally works.
>>
>> In summary: a commit description that more accurately represents the
>> problem being solved is required.
>
> We have seen this problem since T30/T114. Initially we just retried by downloading again. It may work after a few tries. Later, we found a simple solution is replacing with a shorter cable. Then one day, for testing purpose, when the timeout value was replaced with 0, ie, unlimited timeout, all cables work fine. So, it may worth to figure out the root cause.

There's obviously some variability here, since you mention that the 
communication works sometimes and not others.

When you tested the varying cable lengths, did you test at least 10x 
more times than the percentage of times that pass or fail with the 
original cable you used? Without testing a large number of times, you 
don't know if you just got lucky the one time you tried a different 
cable, or whether it really made a statistical difference.

Much more likely is that Tegra itself sometimes takes more time to 
respond, or sometimes receives/sends packets that get corrupted 
somewhere and have to be retried.

BTW, have you tried using Wireshark and usbmon to capture a trace of the 
failures to try and diagnose the issue?

Finally, I'm not objecting to this patch if it does work around the 
problem. All I'm objecting to is blaming the issue on cable length when 
I'm not convinced that's been conclusively proven. A simple message such 
as the following would work:

RCM communication sometimes fails if the USB timeout is short. Increase 
the timeout value to avoid this issue. The exact cause is not yet diagnosed.

RE: [tegrarcm PATCH v1 4/4] Increate USB timeout value

[Jimmy Zhang]

> -----Original Message-----
> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> Sent: Wednesday, March 09, 2016 9:41 AM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; alban.bedel-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org; linux-
> tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [tegrarcm PATCH v1 4/4] Increate USB timeout value
> 
> On 03/08/2016 06:41 PM, Jimmy Zhang wrote:
> >
> >
> >> -----Original Message-----
> >> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> >> Sent: Monday, March 07, 2016 11:40 AM
> >> To: Jimmy Zhang
> >> Cc: Allen Martin; Stephen Warren; alban.bedel-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org;
> >> linux- tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> Subject: Re: [tegrarcm PATCH v1 4/4] Increate USB timeout value
> >>
> >> On 03/04/2016 04:44 PM, Jimmy Zhang wrote:
> >>> It has been observed that some times USB time out could occure when
> >>> a long (3ft) usb cable is connected to recovery mode usb port
> >>
> >> This explanation makes no sense. 3ft is practically the shortest USB
> >> cable anyone would use. Equally, assuming a compliant correctly
> >> functioning cable, cable length doesn't affect the time it takes to execute
> transactions.
> >>
> >> Is the issue more that if a low quality cable is used, then there are
> >> lots of transfer errors, and the time taken for retries is large? If
> >> so, there's not much guarantee that a larger timeout would help in
> >> general, since there's no guarantee of any upper bound on the time it
> >> takes for a packet not to get corrupted in this case. Still, I
> >> suppose it's fine to increase the timeout to account for when it
> accidentally works.
> >>
> >> In summary: a commit description that more accurately represents the
> >> problem being solved is required.
> >
> > We have seen this problem since T30/T114. Initially we just retried by
> downloading again. It may work after a few tries. Later, we found a simple
> solution is replacing with a shorter cable. Then one day, for testing purpose,
> when the timeout value was replaced with 0, ie, unlimited timeout, all cables
> work fine. So, it may worth to figure out the root cause.
> 
> There's obviously some variability here, since you mention that the
> communication works sometimes and not others.
> 
> When you tested the varying cable lengths, did you test at least 10x more
> times than the percentage of times that pass or fail with the original cable
> you used? Without testing a large number of times, you don't know if you
> just got lucky the one time you tried a different cable, or whether it really
> made a statistical difference.
> 
> Much more likely is that Tegra itself sometimes takes more time to respond,
> or sometimes receives/sends packets that get corrupted somewhere and
> have to be retried.
> 
> BTW, have you tried using Wireshark and usbmon to capture a trace of the
> failures to try and diagnose the issue?
> 
> Finally, I'm not objecting to this patch if it does work around the problem. All
> I'm objecting to is blaming the issue on cable length when I'm not convinced
> that's been conclusively proven. A simple message such as the following
> would work:
> 
Agree. We actually never pay much attention to the tool issue because the focus is always on the task performed at that moment.

As Allen suggested, I will create a new usb_timeout command line parameter to allow user to set timeout value.
  
> RCM communication sometimes fails if the USB timeout is short. Increase the
> timeout value to avoid this issue. The exact cause is not yet diagnosed.