[cbootimage PATCH v5 0/5] Add RSA signing support

[cbootimage PATCH v5 0/5] Add RSA signing support

[Jimmy Zhang]

V5:
1. Remove Rehash.cfg from patch 4 and add update.cfg into patch 3 and
   place it under samples directory.

2. Move test key file rsa_priv.pem to samples directory

V4:
1. Replace constant definition with soc specific supported function
   get_value_size() to obtain a field size.
2. Add byte order swapping function to support tegra rsa related fields
   byte order requirements.
3. Use one simplified sample script to demonstrate how to do rsa signing
   for T210 boot image.

V3: 
1. Address issues found in v2. 
2. Use keyword "RehashBl" instead of "ReSignBl" to re-generate AES hash
   for bootloader (and bct).
3. Add sample scripts to do rsa signing for T210 bootimage.

V2:
1. Split CL1 into two patches.

2. Use openssl utility to generate signature and save to file. Then use
   --update option to load in signature files to update rsa-pss signature
   fields in bct. So, all rsa-pss signing functions and files are removed.

3. Use keyword "ReSignBl" to re-generate AES hash for bootloader (and bct).

V1:
For security fused tegra chip, BR requires to verify rsa_pss_sig before
jumping to next level of boot loader.

The patches here are adding rsa_pss_sig related functions, such as updating
signatures and pubkey, generating signatures on boot loader and bct, and
generating signature on any given binary file.


Jimmy Zhang (5):
  Add support for update pubkey and rsa-pss signatures
  Add support to dump rsa related fields for t210
  Add new configuration keyword "RehashBl"
  Add a sample script to do rsa signing for T210 bootimage
  Bump to version 1.6

 configure.ac             |  2 +-
 samples/rsa_priv.pem     | 27 ++++++++++++++++++
 samples/sign.sh          | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
 samples/update.cfg       |  1 +
 src/bct_dump.c           | 39 ++++++++++++++++++++++++++
 src/cbootimage.h         |  1 +
 src/crypto.c             | 54 +++++++++++++++++++++++++++++++++++
 src/crypto.h             | 12 ++++++++
 src/data_layout.c        | 51 +++++++++++++++++++++++++++++++++
 src/data_layout.h        |  2 ++
 src/parse.c              | 44 +++++++++++++++++++++++++++++
 src/parse.h              | 16 +++++++++++
 src/set.c                | 49 ++++++++++++++++++++++++++++++++
 src/set.h                |  5 ++++
 src/t210/nvbctlib_t210.c | 71 +++++++++++++++++++++++++++++++++++++++++++++-
 15 files changed, 445 insertions(+), 2 deletions(-)
 create mode 100644 samples/rsa_priv.pem
 create mode 100755 samples/sign.sh
 create mode 100644 samples/update.cfg

-- 
1.8.1.5

[cbootimage PATCH v5 1/5] Add support for update pubkey and rsa-pss signatures

[Jimmy Zhang]

Create new configuration keywords:
   RsaKeyModulusFile: pubkey modulus
   RsaPssSigBlFile:   bootloader rsa pss signature
   RsaPssSigBctFile:  bct rsa pss signature

Sample Configuration file update_bl_sig.cfg
   RsaKeyModulusFile = pubkey.mod;
   RsaPssSigBlFile = bl.sig;

where pubkey.mod and bl.sig are files that contain the public key
modulus and bootloader's rsa-pss signature respectively.

public key modulus and signature are created through utilities
outside cbootimage.

Command line example:
 $ cbootimage -s tegra210 -u update_bl_sig.cfg image.bin image.bin-bl-signed

Above three new keywords added in this CL are only implemented support
for T210.

Signed-off-by: Jimmy Zhang <jimmzhang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 src/cbootimage.h         |  1 +
 src/crypto.c             | 20 ++++++++++++++++++
 src/crypto.h             |  5 +++++
 src/parse.c              | 35 +++++++++++++++++++++++++++++++
 src/parse.h              | 15 ++++++++++++++
 src/set.c                | 49 +++++++++++++++++++++++++++++++++++++++++++
 src/set.h                |  5 +++++
 src/t210/nvbctlib_t210.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++-
 8 files changed, 183 insertions(+), 1 deletion(-)

diff --git a/src/cbootimage.h b/src/cbootimage.h
index 9706b2c1edb8..63f0ee97e12e 100644
--- a/src/cbootimage.h
+++ b/src/cbootimage.h
@@ -60,6 +60,7 @@ typedef enum
 	file_type_bl = 0,
 	file_type_bct,
 	file_type_mts,
+	file_type_bin,
 } file_type;
 
 /*
diff --git a/src/crypto.c b/src/crypto.c
index 99e9f085763c..f1710e521c77 100644
--- a/src/crypto.c
+++ b/src/crypto.c
@@ -297,3 +297,23 @@ sign_bct(build_image_context *context,
 	free(hash_buffer);
 	return e;
 }
+
+void
+swap_endianness(
+	u_int8_t *out,
+	u_int8_t *in,
+	const u_int32_t size)
+{
+	u_int32_t i;
+
+	for (i = 0; i < size / 2; i++) {
+		u_int8_t b1 = in[i];
+		u_int8_t b2 = in[size - 1 - i];
+		out[i] = b2;
+		out[size - 1 - i] = b1;
+	}
+
+	/* In case odd number of bytes */
+	if (size % 2)
+		out[size / 2] = in[size / 2];
+}
diff --git a/src/crypto.h b/src/crypto.h
index d7151e0cd191..e9d578e8fa7e 100644
--- a/src/crypto.h
+++ b/src/crypto.h
@@ -44,4 +44,9 @@ sign_data_block(u_int8_t *source,
 		u_int32_t length,
 		u_int8_t *signature);
 
+void
+swap_endianness(
+	u_int8_t *out,
+	u_int8_t *in,
+	const u_int32_t size);
 #endif /* #ifndef INCLUDED_CRYPTO_H */
diff --git a/src/parse.c b/src/parse.c
index 8c9824437393..d2f4016effd8 100644
--- a/src/parse.c
+++ b/src/parse.c
@@ -65,6 +65,8 @@ parse_bootloader(build_image_context *context, parse_token token, char *rest);
 static int
 parse_mts_image(build_image_context *context, parse_token token, char *rest);
 static int
+parse_rsa_param(build_image_context *context, parse_token token, char *rest);
+static int
 parse_value_u32(build_image_context *context, parse_token token, char *rest);
 static int
 parse_value_chipuid(build_image_context *context,
@@ -116,6 +118,9 @@ static parse_item s_top_level_items[] = {
 	{ "ChipUid=",       token_unique_chip_id,	parse_value_chipuid },
 	{ "JtagCtrl=",	    token_secure_jtag_control,	parse_value_u32 },
 	{ "DebugCtrl=",	    token_secure_debug_control,	parse_value_u32 },
+	{ "RsaKeyModulusFile=", token_rsa_key_modulus,	parse_rsa_param },
+	{ "RsaPssSigBlFile=",   token_rsa_pss_sig_bl,	parse_rsa_param },
+	{ "RsaPssSigBctFile=",  token_rsa_pss_sig_bct,	parse_rsa_param },
 	{ NULL, 0, NULL } /* Must be last */
 };
 
@@ -480,6 +485,36 @@ static int parse_mts_image(build_image_context *context,
 }
 
 /*
+ * Parse the given rsa modulus/key/signature file name
+ * then call set_rsa_settings to set proper rsa field.
+ *
+ * @param context	The main context pointer
+ * @param token  	The parse token value
+ * @param rest   	String to parse
+ * @return 0 and 1 for success and failure
+ */
+static int parse_rsa_param(build_image_context *context,
+			parse_token token,
+			char *rest)
+{
+	char filename[MAX_BUFFER];
+
+	assert(context != NULL);
+	assert(rest != NULL);
+
+	if (context->generate_bct != 0)
+		return 0;
+
+	/* Parse the file name. */
+	rest = parse_filename(rest, filename, MAX_BUFFER);
+	if (rest == NULL)
+		return 1;
+
+	/* Parsing has finished - set the bootloader */
+	return set_rsa_param(context, token, filename);
+}
+
+/*
  * Parse the given string and find the array items in config file.
  *
  * @param context	The main context pointer
diff --git a/src/parse.h b/src/parse.h
index ce3f21fb8a31..f6411648f883 100644
--- a/src/parse.h
+++ b/src/parse.h
@@ -114,6 +114,10 @@ typedef enum
 	token_secure_jtag_control,
 	token_secure_debug_control,
 
+	token_rsa_key_modulus,
+	token_rsa_pss_sig_bl,
+	token_rsa_pss_sig_bct,
+
 	token_nand_clock_divider,
 	token_nand_nand_timing,
 	token_nand_nand_timing2,
@@ -1109,6 +1113,17 @@ typedef struct cbootimage_soc_config_rec {
 			void *data,
 			u_int8_t *bct);
 	/*
+	 * Get the size of specified bct field
+	 *
+	 * @param id  	The parse token
+	 * @param data	Return size from bct field
+	 * @param bct 	Bct pointer
+	 * @return 0 and -ENODATA for success and failure
+	 */
+	int (*get_value_size)(parse_token id,
+			void *data,
+			u_int8_t *bct);
+	/*
 	 * Set the bct crypto hash data.
 	 *
 	 * @param id    	The parse token value
diff --git a/src/set.c b/src/set.c
index 73af52111360..b6d33189b875 100644
--- a/src/set.c
+++ b/src/set.c
@@ -147,6 +147,55 @@ set_mts_image(build_image_context	*context,
 	context->mts_entry_point = entry_point;
 	return update_mts_image(context);
 }
+
+int
+set_rsa_param(build_image_context *context, parse_token token,
+		char *filename)
+{
+	int	result;
+	u_int8_t *rsa_storage;	/* Holds the rsa param after reading */
+	u_int32_t size;		/* Bytes to read */
+	u_int32_t actual_size;	/* In bytes */
+
+	if (g_soc_config->get_value_size == NULL) {
+		printf("Error: get_value_size() is not supported.\n");
+		exit(1);
+	}
+
+	if (g_soc_config->get_value_size(token, &size, context->bct)) {
+		printf("Error: Unsupported token %d for value size.\n", token);
+		exit(1);
+	}
+
+	/* Read the image into memory. */
+	result = read_from_image(filename,
+				0,
+				size,
+				&rsa_storage,
+				&actual_size,
+				file_type_bin);
+
+	if (result) {
+		printf("Error reading file %s.\n", filename);
+		exit(1);
+	}
+
+	if (actual_size != size) {
+		printf("Error: invalid size, file %s.\n", filename);
+		exit(1);
+	}
+
+	if (enable_debug)
+		printf("Updating token %d with file %s\n", (int)token, filename);
+
+	/* set to appropriate bct field */
+	result = g_soc_config->set_value(token,
+			rsa_storage, context->bct);
+
+	free(rsa_storage);
+	return result;
+}
+
 #define DEFAULT()                                                     \
 	default:                                                      \
 		printf("Unexpected token %d at line %d\n",            \
diff --git a/src/set.h b/src/set.h
index 8b9a69b2a950..b38d4cefcb4f 100644
--- a/src/set.h
+++ b/src/set.h
@@ -42,6 +42,11 @@ set_mts_image(build_image_context	*context,
 		u_int32_t	entry_point);
 
 int
+set_rsa_param(build_image_context	*context,
+		parse_token	token,
+		char	*filename);
+
+int
 context_set_value(build_image_context	*context,
 		parse_token	token,
 		void		*value);
diff --git a/src/t210/nvbctlib_t210.c b/src/t210/nvbctlib_t210.c
index 9921bbbe0d2d..97985db9db7c 100644
--- a/src/t210/nvbctlib_t210.c
+++ b/src/t210/nvbctlib_t210.c
@@ -113,7 +113,10 @@ parse_token t210_root_token_list[] = {
 	token_crypto_length,
 	token_max_bct_search_blks,
 	token_unique_chip_id,
-	token_secure_debug_control
+	token_secure_debug_control,
+	token_rsa_key_modulus,
+	token_rsa_pss_sig_bl,
+	token_rsa_pss_sig_bct
 };
 
 int
@@ -2174,6 +2177,36 @@ t210_bct_get_value(parse_token id, void *data, u_int8_t *bct)
 }
 
 int
+t210_bct_get_value_size(parse_token id, void *data, u_int8_t *bct)
+{
+	nvboot_config_table *bct_ptr = (nvboot_config_table *)bct;
+
+	if (data == NULL)
+		return -ENODATA;
+
+	switch (id) {
+	case token_rsa_key_modulus:
+		*(u_int32_t *)data = sizeof(nvboot_rsa_key_modulus);
+		break;
+
+	case token_rsa_pss_sig_bl:
+		*(u_int32_t *)data = sizeof(nvboot_rsa_pss_sig);
+		break;
+
+	case token_rsa_pss_sig_bct:
+		*(u_int32_t *)data = sizeof(nvboot_rsa_pss_sig);
+		break;
+
+	/*
+	 * Other bct fields can be added in when needed
+	 */
+	default:
+		return -ENODATA;
+	}
+	return 0;
+}
+
+int
 t210_bct_set_value(parse_token id, void *data, u_int8_t *bct)
 {
 	nvboot_config_table *bct_ptr = (nvboot_config_table *)bct;
@@ -2198,6 +2231,24 @@ t210_bct_set_value(parse_token id, void *data, u_int8_t *bct)
 		memcpy(&bct_ptr->unique_chip_id, data, sizeof(nvboot_ecid));
 		break;
 
+	case token_rsa_key_modulus:
+		swap_endianness(&bct_ptr->key, data, sizeof(nvboot_rsa_key_modulus));
+		break;
+
+	case token_rsa_pss_sig_bl:
+		/*
+		 * Update bootloader 0 since there is only one copy
+		 * of bootloader being built in.
+		 */
+		swap_endianness(&bct_ptr->bootloader[0].signature.rsa_pss_sig,
+			data, sizeof(nvboot_rsa_pss_sig));
+		break;
+
+	case token_rsa_pss_sig_bct:
+		swap_endianness(&bct_ptr->signature.rsa_pss_sig,
+			data, sizeof(nvboot_rsa_pss_sig));
+		break;
+
 	default:
 		return -ENODATA;
 	}
@@ -2279,6 +2330,7 @@ cbootimage_soc_config tegra210_config = {
 	.getbl_param				= t210_getbl_param,
 	.set_value					= t210_bct_set_value,
 	.get_value					= t210_bct_get_value,
+	.get_value_size					= t210_bct_get_value_size,
 	.set_data					= t210_bct_set_data,
 	.get_bct_size				= t210_get_bct_size,
 	.token_supported			= t210_bct_token_supported,
-- 
1.8.1.5

[cbootimage PATCH v5 2/5] Add support to dump rsa related fields for t210

[Jimmy Zhang]

Add support to dump rsa pubkey, bct's rsa-pss signature and
bootloader's rsa-pss signature.

Signed-off-by: Jimmy Zhang <jimmzhang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 src/bct_dump.c           | 39 +++++++++++++++++++++++++++++++++++++++
 src/t210/nvbctlib_t210.c | 17 +++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/src/bct_dump.c b/src/bct_dump.c
index be7b85dc72d6..1db7d250dc4f 100644
--- a/src/bct_dump.c
+++ b/src/bct_dump.c
@@ -30,6 +30,8 @@ cbootimage_soc_config * g_soc_config;
 static void format_u32_hex8(char const * message, void * data);
 static void format_u32(char const * message, void * data);
 static void format_chipuid(char const * message, void * data);
+static void format_hex_16_bytes(char const * message, void * data);
+static void format_rsa_param(char const * message, void * data);
 
 typedef void (*format_function)(char const * message, void * data);
 
@@ -39,9 +41,11 @@ typedef struct {
 	format_function format;
 } value_data;
 
+#define ARSE_RSA_PARAM_MAX_BYTES	256
 typedef union {
 	u_int32_t val;
 	u_int8_t uid[16];
+	u_int8_t rsa_param[ARSE_RSA_PARAM_MAX_BYTES];
 } param_types;
 
 #define MAX_PARAM_SIZE sizeof(param_types)
@@ -54,6 +58,9 @@ static value_data const values[] = {
 	{ token_odm_data,            "OdmData       = ", format_u32_hex8 },
 	{ token_secure_jtag_control, "JtagCtrl      = ", format_u32_hex8 },
 	{ token_secure_debug_control, "DebugCtrl     = ", format_u32_hex8 },
+	{ token_crypto_hash, 	     "BCT AES Hash  = ", format_hex_16_bytes },
+	{ token_rsa_key_modulus,     "RsaKeyModulus = ", format_rsa_param },
+	{ token_rsa_pss_sig_bct,     "RsaPssSigBct = ", format_rsa_param },
 	{ token_unique_chip_id,      "ChipUid       = ", format_chipuid },
 	{ token_bootloader_used,     "# Bootloader used       = ", format_u32 },
 	{ token_bootloaders_max,     "# Bootloaders max       = ", format_u32 },
@@ -72,6 +79,8 @@ static value_data const bl_values[] = {
 	{ token_bl_load_addr,   "Load address = ", format_u32_hex8 },
 	{ token_bl_entry_point, "Entry point  = ", format_u32_hex8 },
 	{ token_bl_attribute,   "Attributes   = ", format_u32_hex8 },
+	{ token_bl_crypto_hash, "Bl AES Hash  = ", format_hex_16_bytes },
+	{ token_rsa_pss_sig_bl,	"RsaPssSigBl  = ", format_rsa_param },
 };
 
 static value_data const mts_values[] = {
@@ -108,6 +117,36 @@ static void format_chipuid(char const * message, void * data)
 	printf("%s%s;\n", message, uid_str);
 }
 
+static void format_hex_16_bytes(char const * message, void * data)
+{
+	u_int8_t *p_byte = (u_int8_t *)data;
+	int byte_index;
+
+	printf("%s", message);
+	for (byte_index = 0; byte_index < 16; ++byte_index)
+		printf("%02x", *p_byte++);
+
+	printf(";\n");
+}
+
+static void format_rsa_param(char const * message, void * data)
+{
+	u_int8_t *rsa = (u_int8_t *)data;
+	int byte_index;
+
+	printf("%s", message);
+	for (byte_index = 0; byte_index < ARSE_RSA_PARAM_MAX_BYTES;
+					++byte_index) {
+		printf("%02x", *rsa++);
+
+		if ((byte_index + 1) % 64 == 0)
+			printf(";\n");
+	}
+
+	if (byte_index % 64 != 0)
+		printf(";\n");
+}
+
 /*****************************************************************************/
 static void usage(void)
 {
diff --git a/src/t210/nvbctlib_t210.c b/src/t210/nvbctlib_t210.c
index 97985db9db7c..ba2235e1c653 100644
--- a/src/t210/nvbctlib_t210.c
+++ b/src/t210/nvbctlib_t210.c
@@ -109,6 +109,8 @@ parse_token t210_root_token_list[] = {
 	token_bootloaders_max,
 	token_bct_size,
 	token_hash_size,
+	token_crypto_hash,
+	token_bl_crypto_hash,
 	token_crypto_offset,
 	token_crypto_length,
 	token_max_bct_search_blks,
@@ -2034,6 +2036,12 @@ t210_getbl_param(u_int32_t set,
 		sizeof(nvboot_hash));
 		break;
 
+	case token_rsa_pss_sig_bl:
+		swap_endianness(data,
+			&bct_ptr->bootloader[set].signature.rsa_pss_sig,
+			sizeof(nvboot_rsa_pss_sig));
+		break;
+
 	default:
 		return -ENODATA;
 	}
@@ -2130,6 +2138,15 @@ t210_bct_get_value(parse_token id, void *data, u_int8_t *bct)
 		memcpy(data, &(bct_ptr->unique_chip_id), sizeof(nvboot_ecid));
 		break;
 
+	case token_rsa_key_modulus:
+		swap_endianness(data, &bct_ptr->key, sizeof(nvboot_rsa_key_modulus));
+		break;
+
+	case token_rsa_pss_sig_bct:
+		swap_endianness(data, &bct_ptr->signature.rsa_pss_sig,
+			sizeof(nvboot_rsa_pss_sig));
+		break;
+
 	case token_reserved_offset:
 		*((u_int32_t *)data) = (u_int8_t *)&(samplebct.reserved)
 				- (u_int8_t *)&samplebct;
-- 
1.8.1.5

[cbootimage PATCH v5 3/5] Add new configuration keyword "RehashBl"

[Jimmy Zhang]

This feature is needed in case an image is updated at later stage
after bootimage has been created.

How to use:
  Add keyword "RehashBl" to configuration file, for example, update.cfg:
    RehashBl;

  Invoke cbootimage to re-calculate bootloader aes hash, for example, for
  bootimage bootloader.bin:
    $ cbootimage -s tegra210 --update update.cfg bootloader.bin bootloader.bin-resigned

  Where bootloader.bin-resigned is the resigned bootimage bootloader.bin

Signed-off-by: Jimmy Zhang <jimmzhang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 samples/update.cfg |  1 +
 src/crypto.c       | 34 ++++++++++++++++++++++++++++++++++
 src/crypto.h       |  7 +++++++
 src/data_layout.c  | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/data_layout.h  |  2 ++
 src/parse.c        |  9 +++++++++
 src/parse.h        |  1 +
 7 files changed, 105 insertions(+)
 create mode 100644 samples/update.cfg

diff --git a/samples/update.cfg b/samples/update.cfg
new file mode 100644
index 000000000000..c5c741bad536
--- /dev/null
+++ b/samples/update.cfg
@@ -0,0 +1 @@
+RehashBl;
diff --git a/src/crypto.c b/src/crypto.c
index f1710e521c77..7bf79b529410 100644
--- a/src/crypto.c
+++ b/src/crypto.c
@@ -317,3 +317,37 @@ swap_endianness(
 	if (size % 2)
 		out[size / 2] = in[size / 2];
 }
+
+int
+sign_bl(build_image_context *context,
+	u_int8_t *bootloader,
+	u_int32_t length,
+	u_int32_t image_instance)
+{
+	int e = 0;
+	u_int8_t  *hash_buffer;
+	u_int32_t  hash_size;
+
+	g_soc_config->get_value(token_hash_size,
+			&hash_size, context->bct);
+
+	hash_buffer = calloc(1, hash_size);
+	if (hash_buffer == NULL)
+		return -ENOMEM;
+
+	/* Encrypt and compute hash */
+	if ((e = sign_data_block(bootloader,
+			length,
+			hash_buffer)) != 0)
+		goto fail;
+
+	if ((e = g_soc_config->setbl_param(image_instance,
+				token_bl_crypto_hash,
+				(u_int32_t*)hash_buffer,
+				context->bct)) != 0)
+		goto fail;
+
+ fail:
+	free(hash_buffer);
+	return e;
+}
diff --git a/src/crypto.h b/src/crypto.h
index e9d578e8fa7e..b992d399c724 100644
--- a/src/crypto.h
+++ b/src/crypto.h
@@ -49,4 +49,11 @@ swap_endianness(
 	u_int8_t *out,
 	u_int8_t *in,
 	const u_int32_t size);
+
+int
+sign_bl(build_image_context *context,
+	u_int8_t *bootloader,
+	u_int32_t length,
+	u_int32_t image_instance);
+
 #endif /* #ifndef INCLUDED_CRYPTO_H */
diff --git a/src/data_layout.c b/src/data_layout.c
index 082609236724..5d3fe10ceda4 100644
--- a/src/data_layout.c
+++ b/src/data_layout.c
@@ -1065,3 +1065,54 @@ int get_bct_size_from_image(build_image_context *context)
 	context->bct = 0;
 	return bct_size;
 }
+
+int resign_bl(build_image_context *context)
+{
+	int ret;
+	u_int8_t  *buffer, *image;
+	u_int32_t  image_instance = 0;	/* support only one instance */
+	u_int32_t  image_actual_size; /* In bytes */
+	u_int32_t  bl_length;
+	u_int32_t  pages_in_image;
+	u_int32_t  blk_size, page_size, current_blk, current_page;
+	u_int32_t  offset;
+
+	/* read in bl from image */
+	g_soc_config->get_value(token_block_size, &blk_size, context->bct);
+	g_soc_config->get_value(token_page_size, &page_size, context->bct);
+
+	GET_BL_FIELD(image_instance, start_blk, &current_blk);
+	GET_BL_FIELD(image_instance, start_page,  &current_page);
+	GET_BL_FIELD(image_instance, length,  &bl_length);
+
+	offset = current_blk * blk_size +
+			current_page * page_size;
+
+	if (read_from_image(context->input_image_filename,
+				offset, bl_length,
+				&image, &image_actual_size, file_type_bin)) {
+		printf("Error reading image file %s.\n",
+				context->input_image_filename);
+		return -ENOMEM;
+	}
+
+	pages_in_image = ICEIL(image_actual_size, page_size);
+
+	/* Create a local copy of the bl */
+	if ((buffer = malloc(pages_in_image * page_size)) == NULL) {
+		ret = -ENOMEM;
+		goto fail;
+	}
+
+	memset(buffer, 0, pages_in_image * page_size);
+	memcpy(buffer, image, image_actual_size);
+
+	insert_padding(buffer, image_actual_size);
+
+	/* sign bl */
+	ret = sign_bl(context, buffer, image_actual_size, image_instance);
+	free (buffer);
+ fail:
+	free (image);
+	return ret;
+}
diff --git a/src/data_layout.h b/src/data_layout.h
index c6e53e61be83..0e6e41fcb24c 100644
--- a/src/data_layout.h
+++ b/src/data_layout.h
@@ -64,4 +64,6 @@ get_bct_size_from_image(build_image_context *context);
 int
 begin_update(build_image_context *context);
 
+int
+resign_bl(build_image_context *context);
 #endif /* #ifndef INCLUDED_DATA_LAYOUT_H */
diff --git a/src/parse.c b/src/parse.c
index d2f4016effd8..a7cfb72fa77c 100644
--- a/src/parse.c
+++ b/src/parse.c
@@ -80,6 +80,8 @@ static int
 parse_dev_param(build_image_context *context, parse_token token, char *rest);
 static int
 parse_sdram_param(build_image_context *context, parse_token token, char *rest);
+static int
+parse_sign_bl(build_image_context *context, parse_token token, char *rest);
 
 static int process_statement(build_image_context *context,
 				char *str,
@@ -121,6 +123,7 @@ static parse_item s_top_level_items[] = {
 	{ "RsaKeyModulusFile=", token_rsa_key_modulus,	parse_rsa_param },
 	{ "RsaPssSigBlFile=",   token_rsa_pss_sig_bl,	parse_rsa_param },
 	{ "RsaPssSigBctFile=",  token_rsa_pss_sig_bct,	parse_rsa_param },
+	{ "RehashBl",       token_sign_bl,		parse_sign_bl },
 	{ NULL, 0, NULL } /* Must be last */
 };
 
@@ -689,6 +692,12 @@ parse_bct_file(build_image_context *context, parse_token token, char *rest)
 	return 0;
 }
 
+static int
+parse_sign_bl(build_image_context *context, parse_token token, char *rest)
+{
+	return resign_bl(context);
+}
+
 static char *
 parse_end_state(char *str, char *uname, int chars_remaining)
 {
diff --git a/src/parse.h b/src/parse.h
index f6411648f883..97459dc53f46 100644
--- a/src/parse.h
+++ b/src/parse.h
@@ -117,6 +117,7 @@ typedef enum
 	token_rsa_key_modulus,
 	token_rsa_pss_sig_bl,
 	token_rsa_pss_sig_bct,
+	token_sign_bl,
 
 	token_nand_clock_divider,
 	token_nand_nand_timing,
-- 
1.8.1.5

[cbootimage PATCH v5 4/5] Add a sample script to do rsa signing for T210 bootimage

[Jimmy Zhang]

sign.sh runs openssl and other linux utilities to generate rsa-pss
signatures for a prebuilt bootimage and then uses cbootimage option
--update to update bootimage's rsa signatures and rsa modulus.

Syntax: sign.sh <bootimage> <rsa_key.pem>

Signed-off-by: Jimmy Zhang <jimmzhang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 samples/rsa_priv.pem | 27 +++++++++++++++++++
 samples/sign.sh      | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 100 insertions(+)
 create mode 100644 samples/rsa_priv.pem
 create mode 100755 samples/sign.sh

diff --git a/samples/rsa_priv.pem b/samples/rsa_priv.pem
new file mode 100644
index 000000000000..a02d77fc438c
--- /dev/null
+++ b/samples/rsa_priv.pem
@@ -0,0 +1,27 @@
+-----BEGIN RSA PRIVATE KEY-----
+MIIEpQIBAAKCAQEA2L0jolLgp2pKAzn/JeZuxgGPY1Yz4ZNkttzvBlVhozEynj2x
+Lttz1gZ6fYUb/ObM8v2PoeOlrwkGoWMscuMS4MnLG2NcJlWmlsLTyfw3EwxblM3D
+DniscakhMexNK3J7uxmmRkQTfldec4JHjsAN6d9cZQ0POdsA7j5lKNG0KgCohKk6
+p+lMYXFqgxx3IQWcynhuKVtVFm/UJJBC+a4ibbXcpnio96ySVrPO/ZpEOhEpPTWX
+VLeiqBB3dsu//9X0vDfShyBlctaonx2Z7xWQWotubze0iIvyU6U+T69aDOXfHQfu
+kNMX3Dj4VCndW/FUrrg5k/y9dMA1We3Ng1A0NQIDAQABAoIBABcWRqZy15VdwBaJ
+5gDeg+w5nFGDjDE6Jx9Hd3qgO69LfU3X2njYTYV92SxnsmyFFU3I7rTa7/ouJvOo
+AcMXJxqkxCrdsaIvu3gRtsesQx2XUmYOaPmwpwXQc0XDGxFGt6FdgRW5CK6LlfcN
+6JtvH8xKy6fD9Vw/VOEL6nCnrd5PU3UNU/Ng7h/SZ+5NEALJE7+gaMvmK9o9lX3a
+/tze6bwKKF+a2luTs2aVGxjUYBud6YOE2KPG7zltuHUHUeEgJ/X/sgWYiHsqpK3l
+rIrjCVIQnrRCCtCHg5BbqtwStl5Gz+Y431DXU9Sv6fVqIFgveweePhhDux/YV+KY
+rvq5RiECgYEA7OHr8BYWkeZKuU/IkGdsdiPEEB7mNOJHwE4OXdwLIIygQGtQCuJG
+EHMQv9kE/1ibVRIxqnliFb/CupZ5wwyvjFgUq5XZl7s6XpNOBhJHV0U5AJSvS0rb
+YNU2PBfRmMMI/gRdF/onUpopY7ZWLv7u+VF7ZgtM5hQr2jwcjwBzbRkCgYEA6jsK
+tB6SGIO2c5E+CLAY5J4eJca6ORaVcKw1OfDL346UJYkvqOLBc8KwFs87gDbwhmjn
+GJUWlhk5iUoWZrFJpTj8+hVNxKumtZ5x8MQkNXL7WBNYcVxobuGVW8c6jZU3C/al
+Im9DRTPXhgvMy7mu4slVaAhhrmUJRdl6fwmCR30CgYEA5FoxwML6RPGUrSl9Nb+N
+riFyWvv+fZJ5Cqf0b4S08U6/GPqaMbPJSQgzaE3D5Ie9Tff5CtZyuHagOJDglie/
+fvJWEsak+QETFqK3/2BVh4qClc2/YjyqWKGQ48MuWS4CmCUKvRd4GsfkCGx4jltR
+ceSbqVZRbiaZ04pJGY2ct9kCgYEArWaaLO/4zgcsGfArUXk0ZIMd5G9zS3IJnckO
++l7mPxEpYYRm8Qs1lcJKZAh0jx2dAJRGiO9OMj5oVtevL8UNtTA0L9t3oCJHH2s2
+BLzf5WXC5tgjgICdm4CK9s/N7CTMBKJKa+yci22un0C7ExLagm/0NzkFP3ry22/9
+/HAIr20CgYEAnUGwciM7Z9aMpPkX3iaRG/zm1FWbsuJldNa5IZQ6CamDIZhb+u2u
+1yuCUJZ7zY51RO4n2Hi/1OU1XS7XlevoT22i7xJmIjPVoWzumUwMjmhYVqxK/X50
+Hcd+qL1Xs6KmsWrlg2sgFliX79RawE3jl/yZrFMuHvWiItXO92YFuOI=
+-----END RSA PRIVATE KEY-----
diff --git a/samples/sign.sh b/samples/sign.sh
new file mode 100755
index 000000000000..2edd12695f4b
--- /dev/null
+++ b/samples/sign.sh
@@ -0,0 +1,73 @@
+#!/bin/bash
+#
+# Copyright (c) 2015, NVIDIA CORPORATION.  All rights reserved.
+#
+# This program is free software; you can redistribute it and/or modify it
+# under the terms and conditions of the GNU General Public License,
+# version 2, as published by the Free Software Foundation.
+#
+# This program is distributed in the hope it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+# more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# See file CREDITS for list of people who contributed to this
+# project.
+#
+set -e
+IMAGE_FILE=$1
+KEY_FILE=$2
+TARGET_IMAGE=$IMAGE_FILE
+CONFIG_FILE=config.tmp
+
+CBOOTIMAGE=../src/cbootimage
+BCT_DUMP=../src/bct_dump
+OBJCOPY=objcopy
+OPENSSL=openssl
+DD=dd
+RM=rm
+MV=mv
+XXD=xxd
+CUT=cut
+
+echo "Get rid of all temporary files: *.sig, *.tosig, *.tmp *.mod"
+$RM -f *.sig *.tosig *.tmp *.mod
+
+echo "Get bl length "
+BL_LENGTH=`$BCT_DUMP $IMAGE_FILE | grep "Bootloader\[0\].Length"\
+ | awk -F ' ' '{print $4}' | awk -F ';' '{print $1}'`
+
+echo "Extract bootloader to $IMAGE_FILE.bl.tosig, length $BL_LENGTH"
+$DD bs=1 skip=32768 if=$IMAGE_FILE of=$IMAGE_FILE.bl.tosig count=$BL_LENGTH
+
+echo "Calculate rsa signature for bootloader and save to $IMAGE_FILE.bl.sig"
+$OPENSSL dgst -sha256 -sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:-1 \
+ -sign $KEY_FILE -out $IMAGE_FILE.bl.sig $IMAGE_FILE.bl.tosig
+
+echo "Update bootloader's rsa signature, aes hash and bct's aes hash"
+echo "RsaPssSigBlFile = $IMAGE_FILE.bl.sig;" > $CONFIG_FILE
+echo "RehashBl;" >> $CONFIG_FILE
+$CBOOTIMAGE -s tegra210 -u $CONFIG_FILE $IMAGE_FILE $IMAGE_FILE.tmp
+
+echo "Extract the part of bct which needs to be rsa signed"
+$DD bs=1 if=$IMAGE_FILE.tmp of=$IMAGE_FILE.bct.tosig count=8944 skip=1296
+
+echo "Calculate rsa signature for bct and save to $IMAGE_FILE.bct.sig"
+$OPENSSL dgst -sha256 -sigopt rsa_padding_mode:pss -sigopt rsa_pss_saltlen:-1 \
+ -sign $KEY_FILE -out $IMAGE_FILE.bct.sig $IMAGE_FILE.bct.tosig
+
+echo "Create public key modulus from key file $KEY_FILE and save to $KEY_FILE.mod"
+$OPENSSL rsa -in $KEY_FILE -noout -modulus -out $KEY_FILE.mod
+# remove prefix
+$CUT -d= -f2 < $KEY_FILE.mod > $KEY_FILE.mod.tmp
+
+# convert from hexdecimal to binary
+$XXD -r -p -l 256 $KEY_FILE.mod.tmp $KEY_FILE.mod.bin
+
+echo "Update bct's rsa signature and modulus"
+echo "RsaPssSigBctFile = $IMAGE_FILE.bct.sig;" > $CONFIG_FILE
+echo "RsaKeyModulusFile = $KEY_FILE.mod.bin;" >> $CONFIG_FILE
+$CBOOTIMAGE -s tegra210 -u $CONFIG_FILE $IMAGE_FILE.tmp $TARGET_IMAGE
-- 
1.8.1.5

[cbootimage PATCH v5 5/5] Bump to version 1.6

[Jimmy Zhang]

Create a release that adds rsa-pss signature support. Currently
it has only been tested on T210.

Signed-off-by: Jimmy Zhang <jimmzhang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 configure.ac | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index f0f050946504..f251f09c211f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2,7 +2,7 @@
 # Process this file with autoconf to produce a configure script.
 
 AC_PREREQ([2.67])
-AC_INIT([cbootimage], [1.5], [pchiu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org])
+AC_INIT([cbootimage], [1.6], [jimmzhang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org])
 AM_INIT_AUTOMAKE
 AC_CONFIG_SRCDIR([src/cbootimage.c])
 AC_CONFIG_HEADERS([config.h])
-- 
1.8.1.5

Re: [cbootimage PATCH v5 1/5] Add support for update pubkey and rsa-pss signatures

[Stephen Warren]

On 10/09/2015 07:46 PM, Jimmy Zhang wrote:
> Create new configuration keywords:
>     RsaKeyModulusFile: pubkey modulus
>     RsaPssSigBlFile:   bootloader rsa pss signature
>     RsaPssSigBctFile:  bct rsa pss signature
>
> Sample Configuration file update_bl_sig.cfg
>     RsaKeyModulusFile = pubkey.mod;
>     RsaPssSigBlFile = bl.sig;
>
> where pubkey.mod and bl.sig are files that contain the public key
> modulus and bootloader's rsa-pss signature respectively.
>
> public key modulus and signature are created through utilities
> outside cbootimage.
>
> Command line example:
>   $ cbootimage -s tegra210 -u update_bl_sig.cfg image.bin image.bin-bl-signed
>
> Above three new keywords added in this CL are only implemented support
> for T210.

I'd like to see a changelog per patch so I don't have to refer back to 
the cover letter each time.

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

> +void
> +swap_endianness(

Nit: It's more like "byte order" (serialization) rather than endianness, 
although they're related concepts.

> +	u_int8_t *out,
> +	u_int8_t *in,

Nit: You could make "in" const to since it's not written.

> +	const u_int32_t size)

... certainly that'd be more useful than making size const.

> +{
> +	u_int32_t i;
> +
> +	for (i = 0; i < size / 2; i++) {
> +		u_int8_t b1 = in[i];
> +		u_int8_t b2 = in[size - 1 - i];
> +		out[i] = b2;
> +		out[size - 1 - i] = b1;

Nit: It'd be nice to put "size - 1 - i" into a variable rather than 
duplicate the calculation.

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

> +int
> +set_rsa_param(build_image_context *context, parse_token token,
> +		char *filename)
> +{
> +	int	result;
> +	u_int8_t *rsa_storage;	/* Holds the rsa param after reading */
> +	u_int32_t size;		/* Bytes to read */
> +	u_int32_t actual_size;	/* In bytes */
> +
> +	if (g_soc_config->get_value_size == NULL) {
> +		printf("Error: get_value_size() is not supported.\n");

I think code that calls that function should be able to assume the 
function pointer is non-NULL. This could be achieved by either having 
each SoC-specific file provide an implementation of that function. For 
the SoCs that don't support this, you could share a common dummy 
implementation that always returns an error.

> diff --git a/src/parse.h b/src/parse.h

>   	/*
> +	 * Get the size of specified bct field
> +	 *
> +	 * @param id  	The parse token
> +	 * @param data	Return size from bct field
> +	 * @param bct 	Bct pointer
> +	 * @return 0 and -ENODATA for success and failure
> +	 */
> +	int (*get_value_size)(parse_token id,
> +			void *data,
> +			u_int8_t *bct);

The existing get/set functions use a void* since they can operate on 
different fields with different data types. However, the result of 
retrieving a field's size is always a size_t. The "bct" parameter also 
doesn't seem to be used; do you think there could ever be a use for it? 
I think the following prototype should be used so that we can get as 
much type safety as possible:

// Returns >0 for size, -ve for error, never returns 0.
size_t (*get_value_size)(parse_token id);

Re: [cbootimage PATCH v5 2/5] Add support to dump rsa related fields for t210

[Stephen Warren]

On 10/09/2015 07:46 PM, Jimmy Zhang wrote:
> Add support to dump rsa pubkey, bct's rsa-pss signature and
> bootloader's rsa-pss signature.

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

> +#define ARSE_RSA_PARAM_MAX_BYTES	256
>   typedef union {
>   	u_int32_t val;
>   	u_int8_t uid[16];
> +	u_int8_t rsa_param[ARSE_RSA_PARAM_MAX_BYTES];
>   } param_types;

Shouldn't that be replaced by something that uses the new get_size() 
functionality now implemented in patch 1?

RE: [cbootimage PATCH v5 2/5] Add support to dump rsa related fields for t210

[Jimmy Zhang]

> -----Original Message-----
> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> Sent: Monday, October 12, 2015 3:51 PM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [cbootimage PATCH v5 2/5] Add support to dump rsa related
> fields for t210
> 
> On 10/09/2015 07:46 PM, Jimmy Zhang wrote:
> > Add support to dump rsa pubkey, bct's rsa-pss signature and
> > bootloader's rsa-pss signature.
> 
> > diff --git a/src/bct_dump.c b/src/bct_dump.c
> 
> > +#define ARSE_RSA_PARAM_MAX_BYTES	256
> >   typedef union {
> >   	u_int32_t val;
> >   	u_int8_t uid[16];
> > +	u_int8_t rsa_param[ARSE_RSA_PARAM_MAX_BYTES];
> >   } param_types;
> 
> Shouldn't that be replaced by something that uses the new get_size()
> functionality now implemented in patch 1?

For this data structure, I guess we better stay with a MAX constant.

For display functions, ie

values[i].format(...)
bl_values[j].format(...)

There is no id token being passed in. To use get_size(),  all format_xxx function prototype need to be redefined (by adding in id token). 

I can submit a new patch if you agree my observations.

RE: [cbootimage PATCH v5 1/5] Add support for update pubkey and rsa-pss signatures

[Jimmy Zhang]

> -----Original Message-----
> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> Sent: Monday, October 12, 2015 3:49 PM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [cbootimage PATCH v5 1/5] Add support for update pubkey and
> rsa-pss signatures
> 
> On 10/09/2015 07:46 PM, Jimmy Zhang wrote:
> > Create new configuration keywords:
> >     RsaKeyModulusFile: pubkey modulus
> >     RsaPssSigBlFile:   bootloader rsa pss signature
> >     RsaPssSigBctFile:  bct rsa pss signature
> >
> > Sample Configuration file update_bl_sig.cfg
> >     RsaKeyModulusFile = pubkey.mod;
> >     RsaPssSigBlFile = bl.sig;
> >
> > where pubkey.mod and bl.sig are files that contain the public key
> > modulus and bootloader's rsa-pss signature respectively.
> >
> > public key modulus and signature are created through utilities outside
> > cbootimage.
> >
> > Command line example:
> >   $ cbootimage -s tegra210 -u update_bl_sig.cfg image.bin
> > image.bin-bl-signed
> >
> > Above three new keywords added in this CL are only implemented support
> > for T210.
> 
> I'd like to see a changelog per patch so I don't have to refer back to the cover
> letter each time.
> 

OK

> > diff --git a/src/crypto.c b/src/crypto.c
> 
> > +void
> > +swap_endianness(
> 
> Nit: It's more like "byte order" (serialization) rather than endianness,
> although they're related concepts.
> 

This is the function name used by tegrasign. I am open if you have a better name. The reason for the swap because the string actually is a 256 byte long number. Tegra soc handles a number by little endian byte order.
 
> > +	u_int8_t *out,
> > +	u_int8_t *in,
> 
> Nit: You could make "in" const to since it's not written.
> 

OK.

> > +	const u_int32_t size)
> 
> ... certainly that'd be more useful than making size const.
> 
> > +{
> > +	u_int32_t i;
> > +
> > +	for (i = 0; i < size / 2; i++) {
> > +		u_int8_t b1 = in[i];
> > +		u_int8_t b2 = in[size - 1 - i];
> > +		out[i] = b2;
> > +		out[size - 1 - i] = b1;
> 
> Nit: It'd be nice to put "size - 1 - i" into a variable rather than duplicate the
> calculation.
> 
> > diff --git a/src/set.c b/src/set.c
> 
> > +int
> > +set_rsa_param(build_image_context *context, parse_token token,
> > +		char *filename)
> > +{
> > +	int	result;
> > +	u_int8_t *rsa_storage;	/* Holds the rsa param after reading */
> > +	u_int32_t size;		/* Bytes to read */
> > +	u_int32_t actual_size;	/* In bytes */
> > +
> > +	if (g_soc_config->get_value_size == NULL) {
> > +		printf("Error: get_value_size() is not supported.\n");
> 
> I think code that calls that function should be able to assume the function
> pointer is non-NULL. This could be achieved by either having each SoC-
> specific file provide an implementation of that function. For the SoCs that
> don't support this, you could share a common dummy implementation that
> always returns an error.
> 

OK

> > diff --git a/src/parse.h b/src/parse.h
> 
> >   	/*
> > +	 * Get the size of specified bct field
> > +	 *
> > +	 * @param id  	The parse token
> > +	 * @param data	Return size from bct field
> > +	 * @param bct 	Bct pointer
> > +	 * @return 0 and -ENODATA for success and failure
> > +	 */
> > +	int (*get_value_size)(parse_token id,
> > +			void *data,
> > +			u_int8_t *bct);
> 
> The existing get/set functions use a void* since they can operate on
> different fields with different data types. However, the result of retrieving a
> field's size is always a size_t. The "bct" parameter also doesn't seem to be
> used; do you think there could ever be a use for it?
> I think the following prototype should be used so that we can get as much
> type safety as possible:
> 
> // Returns >0 for size, -ve for error, never returns 0.
> size_t (*get_value_size)(parse_token id);

OK.

Re: [cbootimage PATCH v5 1/5] Add support for update pubkey and rsa-pss signatures

[Stephen Warren]

On 10/12/2015 08:02 PM, Jimmy Zhang wrote:
>
>
>> -----Original Message-----
>> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
>> Sent: Monday, October 12, 2015 3:49 PM
>> To: Jimmy Zhang
>> Cc: Allen Martin; Stephen Warren; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [cbootimage PATCH v5 1/5] Add support for update pubkey and
>> rsa-pss signatures
>>
>> On 10/09/2015 07:46 PM, Jimmy Zhang wrote:
>>> Create new configuration keywords:
>>>      RsaKeyModulusFile: pubkey modulus
>>>      RsaPssSigBlFile:   bootloader rsa pss signature
>>>      RsaPssSigBctFile:  bct rsa pss signature
>>>
>>> Sample Configuration file update_bl_sig.cfg
>>>      RsaKeyModulusFile = pubkey.mod;
>>>      RsaPssSigBlFile = bl.sig;
>>>
>>> where pubkey.mod and bl.sig are files that contain the public key
>>> modulus and bootloader's rsa-pss signature respectively.
>>>
>>> public key modulus and signature are created through utilities outside
>>> cbootimage.
>>>
>>> Command line example:
>>>    $ cbootimage -s tegra210 -u update_bl_sig.cfg image.bin
>>> image.bin-bl-signed
>>>
>>> Above three new keywords added in this CL are only implemented support
>>> for T210.
>>
>> I'd like to see a changelog per patch so I don't have to refer back to the cover
>> letter each time.
>>
>
> OK
>
>>> diff --git a/src/crypto.c b/src/crypto.c
>>
>>> +void
>>> +swap_endianness(
>>
>> Nit: It's more like "byte order" (serialization) rather than endianness,
>> although they're related concepts.
>
> This is the function name used by tegrasign. I am open if you have a better name. The reason for the swap because the string actually is a 256 byte long number. Tegra soc handles a number by little endian byte order.

reverse_byte_order()?

BTW, does cbootimage operate correctly if run in a big-endian host?

Re: [cbootimage PATCH v5 2/5] Add support to dump rsa related fields for t210

[Stephen Warren]

On 10/12/2015 06:56 PM, Jimmy Zhang wrote:
>
>
>> -----Original Message-----
>> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
>> Sent: Monday, October 12, 2015 3:51 PM
>> To: Jimmy Zhang
>> Cc: Allen Martin; Stephen Warren; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [cbootimage PATCH v5 2/5] Add support to dump rsa related
>> fields for t210
>>
>> On 10/09/2015 07:46 PM, Jimmy Zhang wrote:
>>> Add support to dump rsa pubkey, bct's rsa-pss signature and
>>> bootloader's rsa-pss signature.
>>
>>> diff --git a/src/bct_dump.c b/src/bct_dump.c
>>
>>> +#define ARSE_RSA_PARAM_MAX_BYTES	256
>>>    typedef union {
>>>    	u_int32_t val;
>>>    	u_int8_t uid[16];
>>> +	u_int8_t rsa_param[ARSE_RSA_PARAM_MAX_BYTES];
>>>    } param_types;
>>
>> Shouldn't that be replaced by something that uses the new get_size()
>> functionality now implemented in patch 1?
>
> For this data structure, I guess we better stay with a MAX constant.
>
> For display functions, ie
>
> values[i].format(...)
> bl_values[j].format(...)
>
> There is no id token being passed in. To use get_size(),  all format_xxx function prototype need to be redefined (by adding in id token).
>
> I can submit a new patch if you agree my observations.

If we can't pass the size through all the way to make everything fully 
generic, I'd recommend:

a)

Name the field and MAX_BYES constant something more generic so it could 
be re-used for arbitrary fields, e.g. such as:

#define PARAM_TYPE_BINARY_DATA_MAX_SIZE 256

u_int8_t binary[PARAM_TYPE_BINARY_DATA_MAX_SIZE]

b)

When filling in that field, call get_size() at that point in time, and 
validate that the value is equal to sizeof(binary) or 
PARAM_TYPE_BINARY_DATA_MAX_SIZE. At least that way we'll have a check 
that the sizes do actually match.


Or, perhaps we can make the field:

struct {
     size_t len;
     u_int8_t data[PARAM_TYPE_BINARY_DATA_MAX_SIZE];
} binary;

... and hence pass the size around that way?

RE: [cbootimage PATCH v5 1/5] Add support for update pubkey and rsa-pss signatures

[Jimmy Zhang]

> -----Original Message-----
> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> Sent: Tuesday, October 13, 2015 9:19 AM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [cbootimage PATCH v5 1/5] Add support for update pubkey and
> rsa-pss signatures
> 
> On 10/12/2015 08:02 PM, Jimmy Zhang wrote:
> >
> >
> >> -----Original Message-----
> >> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> >> Sent: Monday, October 12, 2015 3:49 PM
> >> To: Jimmy Zhang
> >> Cc: Allen Martin; Stephen Warren; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> Subject: Re: [cbootimage PATCH v5 1/5] Add support for update pubkey
> >> and rsa-pss signatures
> >>
> >> On 10/09/2015 07:46 PM, Jimmy Zhang wrote:
> >>> Create new configuration keywords:
> >>>      RsaKeyModulusFile: pubkey modulus
> >>>      RsaPssSigBlFile:   bootloader rsa pss signature
> >>>      RsaPssSigBctFile:  bct rsa pss signature
> >>>
> >>> Sample Configuration file update_bl_sig.cfg
> >>>      RsaKeyModulusFile = pubkey.mod;
> >>>      RsaPssSigBlFile = bl.sig;
> >>>
> >>> where pubkey.mod and bl.sig are files that contain the public key
> >>> modulus and bootloader's rsa-pss signature respectively.
> >>>
> >>> public key modulus and signature are created through utilities
> >>> outside cbootimage.
> >>>
> >>> Command line example:
> >>>    $ cbootimage -s tegra210 -u update_bl_sig.cfg image.bin
> >>> image.bin-bl-signed
> >>>
> >>> Above three new keywords added in this CL are only implemented
> >>> support for T210.
> >>
> >> I'd like to see a changelog per patch so I don't have to refer back
> >> to the cover letter each time.
> >>
> >
> > OK
> >
> >>> diff --git a/src/crypto.c b/src/crypto.c
> >>
> >>> +void
> >>> +swap_endianness(
> >>
> >> Nit: It's more like "byte order" (serialization) rather than
> >> endianness, although they're related concepts.
> >
> > This is the function name used by tegrasign. I am open if you have a better
> name. The reason for the swap because the string actually is a 256 byte long
> number. Tegra soc handles a number by little endian byte order.
> 
> reverse_byte_order()?
> 
> BTW, does cbootimage operate correctly if run in a big-endian host?


For this function,  it should be no difference.

RE: [cbootimage PATCH v5 1/5] Add support for update pubkey and rsa-pss signatures

[Jimmy Zhang]

> -----Original Message-----
> From: Jimmy Zhang
> Sent: Monday, October 12, 2015 7:02 PM
> To: 'Stephen Warren'
> Cc: Allen Martin; Stephen Warren; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: RE: [cbootimage PATCH v5 1/5] Add support for update pubkey and
> rsa-pss signatures
> 
> 
> 
> > -----Original Message-----
> > From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> > Sent: Monday, October 12, 2015 3:49 PM
> > To: Jimmy Zhang
> > Cc: Allen Martin; Stephen Warren; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Re: [cbootimage PATCH v5 1/5] Add support for update pubkey
> > and rsa-pss signatures
> >
> > On 10/09/2015 07:46 PM, Jimmy Zhang wrote:
> > > Create new configuration keywords:
> > >     RsaKeyModulusFile: pubkey modulus
> > >     RsaPssSigBlFile:   bootloader rsa pss signature
> > >     RsaPssSigBctFile:  bct rsa pss signature
> > >
> > > Sample Configuration file update_bl_sig.cfg
> > >     RsaKeyModulusFile = pubkey.mod;
> > >     RsaPssSigBlFile = bl.sig;
> > >
> > > where pubkey.mod and bl.sig are files that contain the public key
> > > modulus and bootloader's rsa-pss signature respectively.
> > >
> > > public key modulus and signature are created through utilities
> > > outside cbootimage.
> > >
> > > Command line example:
> > >   $ cbootimage -s tegra210 -u update_bl_sig.cfg image.bin
> > > image.bin-bl-signed
> > >
> > > Above three new keywords added in this CL are only implemented
> > > support for T210.
> >
> > I'd like to see a changelog per patch so I don't have to refer back to
> > the cover letter each time.
> >
> 
> OK
> 
> > > diff --git a/src/crypto.c b/src/crypto.c
> >
> > > +void
> > > +swap_endianness(
> >
> > Nit: It's more like "byte order" (serialization) rather than
> > endianness, although they're related concepts.
> >
> 
> This is the function name used by tegrasign. I am open if you have a better
> name. The reason for the swap because the string actually is a 256 byte long
> number. Tegra soc handles a number by little endian byte order.
> 
> > > +	u_int8_t *out,
> > > +	u_int8_t *in,
> >
> > Nit: You could make "in" const to since it's not written.
> >
> 
> OK.
> 

Actually this function allows output pointing to input, ie, reversing itself in byte order.

> > > +	const u_int32_t size)
> >
> > ... certainly that'd be more useful than making size const.
> >
> > > +{
> > > +	u_int32_t i;
> > > +
> > > +	for (i = 0; i < size / 2; i++) {
> > > +		u_int8_t b1 = in[i];
> > > +		u_int8_t b2 = in[size - 1 - i];
> > > +		out[i] = b2;
> > > +		out[size - 1 - i] = b1;
> >
> > Nit: It'd be nice to put "size - 1 - i" into a variable rather than
> > duplicate the calculation.
> >

OK

> > > diff --git a/src/set.c b/src/set.c
> >
> > > +int
> > > +set_rsa_param(build_image_context *context, parse_token token,
> > > +		char *filename)
> > > +{
> > > +	int	result;
> > > +	u_int8_t *rsa_storage;	/* Holds the rsa param after reading */
> > > +	u_int32_t size;		/* Bytes to read */
> > > +	u_int32_t actual_size;	/* In bytes */
> > > +
> > > +	if (g_soc_config->get_value_size == NULL) {
> > > +		printf("Error: get_value_size() is not supported.\n");
> >
> > I think code that calls that function should be able to assume the
> > function pointer is non-NULL. This could be achieved by either having
> > each SoC- specific file provide an implementation of that function.
> > For the SoCs that don't support this, you could share a common dummy
> > implementation that always returns an error.
> >
> 
> OK
> 
> > > diff --git a/src/parse.h b/src/parse.h
> >
> > >   	/*
> > > +	 * Get the size of specified bct field
> > > +	 *
> > > +	 * @param id  	The parse token
> > > +	 * @param data	Return size from bct field
> > > +	 * @param bct 	Bct pointer
> > > +	 * @return 0 and -ENODATA for success and failure
> > > +	 */
> > > +	int (*get_value_size)(parse_token id,
> > > +			void *data,
> > > +			u_int8_t *bct);
> >
> > The existing get/set functions use a void* since they can operate on
> > different fields with different data types. However, the result of
> > retrieving a field's size is always a size_t. The "bct" parameter also
> > doesn't seem to be used; do you think there could ever be a use for it?
> > I think the following prototype should be used so that we can get as
> > much type safety as possible:
> >
> > // Returns >0 for size, -ve for error, never returns 0.
> > size_t (*get_value_size)(parse_token id);
> 
> OK.

Re: [cbootimage PATCH v5 1/5] Add support for update pubkey and rsa-pss signatures

[Stephen Warren]

On 10/16/2015 06:21 PM, Jimmy Zhang wrote:
> Jimmy Zhang wrote at Monday, October 12, 2015 7:02 PM
>> Stephen Warren wrote at Monday, October 12, 2015 3:49 PM:
>>> On 10/09/2015 07:46 PM, Jimmy Zhang wrote:
>>>> Create new configuration keywords:
>>>>      RsaKeyModulusFile: pubkey modulus
>>>>      RsaPssSigBlFile:   bootloader rsa pss signature
>>>>      RsaPssSigBctFile:  bct rsa pss signature
>>>>
>>>> Sample Configuration file update_bl_sig.cfg
>>>>      RsaKeyModulusFile = pubkey.mod;
>>>>      RsaPssSigBlFile = bl.sig;
>>>>
>>>> where pubkey.mod and bl.sig are files that contain the public key
>>>> modulus and bootloader's rsa-pss signature respectively.
>>>>
>>>> public key modulus and signature are created through utilities
>>>> outside cbootimage.
>>>>
>>>> Command line example:
>>>>    $ cbootimage -s tegra210 -u update_bl_sig.cfg image.bin
>>>> image.bin-bl-signed
>>>>
>>>> Above three new keywords added in this CL are only implemented
>>>> support for T210.

>>>> diff --git a/src/crypto.c b/src/crypto.c
>>>
>>>> +void
>>>> +swap_endianness(
...
>> This is the function name used by tegrasign. I am open if you have a better
>> name. The reason for the swap because the string actually is a 256 byte long
>> number. Tegra soc handles a number by little endian byte order.
>>
>>>> +	u_int8_t *out,
>>>> +	u_int8_t *in,
>>>
>>> Nit: You could make "in" const to since it's not written.
>>>
>>
>> OK.
>
> Actually this function allows output pointing to input, ie, reversing itself in byte order.

Presumably however, the "in" pointer is only used for reads and the 
"out" point is only used for writes, so "in" can still be const?