[tegrarcm PATCH v2 0/4] Enable --update option support for t210

[tegrarcm PATCH v2 0/4] Enable --update option support for t210

[Jimmy Zhang]

Addressed comments made in v1:

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).


Jimmy Zhang (4):
  Enable -u | --update option support for t210
  Add support for update pubkey and rsa-pss signatures
  Add support to dump rsa related fields for t210
  Add new configuration keyword "ReSignBl"

 src/bct_dump.c             | 38 ++++++++++++++++++++++++++++++++++
 src/cbootimage.c           |  9 ++++----
 src/cbootimage.h           |  5 +++++
 src/crypto.c               | 34 +++++++++++++++++++++++++++++++
 src/crypto.h               |  6 ++++++
 src/data_layout.c          | 51 ++++++++++++++++++++++++++++++++++++++++++++++
 src/data_layout.h          |  2 ++
 src/parse.c                | 44 +++++++++++++++++++++++++++++++++++++++
 src/parse.h                |  5 +++++
 src/set.c                  | 39 +++++++++++++++++++++++++++++++++++
 src/set.h                  |  5 +++++
 src/t210/nvbctlib_t210.c   | 39 ++++++++++++++++++++++++++++++++++-
 src/t210/nvboot_bct_t210.h |  2 --
 13 files changed, 272 insertions(+), 7 deletions(-)

-- 
1.8.1.5

[tegrarcm PATCH v2 1/4] Enable -u | --update option support for t210

[Jimmy Zhang]

Signed-off-by: Jimmy Zhang <jimmzhang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 src/cbootimage.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/cbootimage.c b/src/cbootimage.c
index 1dfb719c819b..b62cedc47ec0 100644
--- a/src/cbootimage.c
+++ b/src/cbootimage.c
@@ -79,7 +79,7 @@ usage(void)
 	printf("                          Default: tegra20.\n");
 	printf("    -u|--update           Copy input image data and update bct\n");
 	printf("                          configs into new image file.\n");
-	printf("                          This feature is only for tegra114/124.\n");
+	printf("                          This feature is currently not supported on tegra20/30.\n");
 	printf("    configfile            File with configuration information\n");
 	printf("    inputimage            Input image name. This is required\n");
 	printf("                          if -u|--update option is used.\n");
@@ -169,9 +169,10 @@ process_command_line(int argc, char *argv[], build_image_context *context)
 	/* Record the input image filename if update_image is necessary */
 	if (context->update_image)
 	{
-		if (context->boot_data_version != BOOTDATA_VERSION_T114 &&
-			context->boot_data_version != BOOTDATA_VERSION_T124) {
-			printf("Update image feature is only for Tegra114 and Tegra124.\n");
+		if (context->boot_data_version == BOOTDATA_VERSION_T20 ||
+			context->boot_data_version == BOOTDATA_VERSION_T30) {
+			printf("Update image feature is not supported on"
+				" Tegra20/30.\n");
 			return -EINVAL;
 		}
 
-- 
1.8.1.5

Re: [tegrarcm PATCH v2 1/4] Enable -u | --update option support for t210

[Stephen Warren]

On 10/02/2015 02:56 PM, Jimmy Zhang wrote:
> Signed-off-by: Jimmy Zhang <jimmzhang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

I've applied this patch. I'll push once I'm not reviewing the other patches.

I fixed up a minor capitalization issue in the help text. A patch 
description might have been nice.

[tegrarcm PATCH v2 2/4] 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           |  4 ++++
 src/parse.c                | 35 +++++++++++++++++++++++++++++++++++
 src/parse.h                |  4 ++++
 src/set.c                  | 39 +++++++++++++++++++++++++++++++++++++++
 src/set.h                  |  5 +++++
 src/t210/nvbctlib_t210.c   | 23 ++++++++++++++++++++++-
 src/t210/nvboot_bct_t210.h |  2 --
 7 files changed, 109 insertions(+), 3 deletions(-)

diff --git a/src/cbootimage.h b/src/cbootimage.h
index 9706b2c1edb8..1ce8af6f6584 100644
--- a/src/cbootimage.h
+++ b/src/cbootimage.h
@@ -49,6 +49,9 @@
 
 #define MAX_MTS_SIZE (4 * 1024 * 1024)
 
+#define ARSE_RSA_MAX_MODULUS_SIZE	2048
+#define ARSE_RSA_PARAM_MAX_BYTES	(ARSE_RSA_MAX_MODULUS_SIZE / 8)
+
 #define NVBOOT_CONFIG_TABLE_SIZE_MAX (10 * 1024)
 
 /*
@@ -60,6 +63,7 @@ typedef enum
 	file_type_bl = 0,
 	file_type_bct,
 	file_type_mts,
+	file_type_bin,
 } file_type;
 
 /*
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..16242a5c2701 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,
diff --git a/src/set.c b/src/set.c
index 73af52111360..d9252ef1d706 100644
--- a/src/set.c
+++ b/src/set.c
@@ -147,6 +147,45 @@ 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 actual_size;	/* In bytes */
+	file_type rsa_filetype = file_type_bin;
+
+        /* Read the image into memory. */
+	result = read_from_image(filename,
+				0,
+				ARSE_RSA_PARAM_MAX_BYTES,
+				&rsa_storage,
+				&actual_size,
+				rsa_filetype);
+
+	if (result) {
+		printf("Error reading file %s.\n", filename);
+		exit(1);
+	}
+
+	if (actual_size != ARSE_RSA_PARAM_MAX_BYTES) {
+		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..91a42ec8367f 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
@@ -2198,6 +2201,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:
+		memcpy(&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.
+		 */
+		memcpy(&bct_ptr->bootloader[0].signature.rsa_pss_sig,
+			data, sizeof(nvboot_rsa_pss_sig));
+		break;
+
+	case token_rsa_pss_sig_bct:
+		memcpy(&bct_ptr->signature.rsa_pss_sig,
+			data, sizeof(nvboot_rsa_pss_sig));
+		break;
+
 	default:
 		return -ENODATA;
 	}
diff --git a/src/t210/nvboot_bct_t210.h b/src/t210/nvboot_bct_t210.h
index 90841f63feb6..c790ee97106d 100644
--- a/src/t210/nvboot_bct_t210.h
+++ b/src/t210/nvboot_bct_t210.h
@@ -94,8 +94,6 @@
  */
 #define NVBOOT_MAX_BCT_SEARCH_BLOCKS	64
 
-#define ARSE_RSA_MAX_MODULUS_SIZE	2048
-
 /**
  * Defines the RSA modulus length in bits and bytes used for PKC secure boot.
  */
-- 
1.8.1.5

Re: [tegrarcm PATCH v2 2/4] Add support for update pubkey and rsa-pss signatures

[Stephen Warren]

On 10/02/2015 02:56 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/cbootimage.h b/src/cbootimage.h

> +#define ARSE_RSA_MAX_MODULUS_SIZE	2048
> +#define ARSE_RSA_PARAM_MAX_BYTES	(ARSE_RSA_MAX_MODULUS_SIZE / 8)

These values are currently defined in the chip-specific location 
src/tNNN/nvboot_bct_tNNN.h, and this patch only removes a single on of 
those 3 copies when creating this new centralized value. At best that 
makes the code inconsistent, and at worst could cause compile errors due 
to multiple definitions.

Instead, can you leave this value in the T210-specific location. More 
related to this below.

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

> +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;

That doesn't look like the correct error check. Rather, other function 
that parse filenames (and other fields) appear to do something like the 
following after parsing all the fields:

         /* Parse the end state. */
         rest = parse_end_state(rest, e_state, MAX_STR_LEN);
         if (rest == NULL)
                 return 1;

I suspect that code should be present in this function too?

> 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 actual_size;	/* In bytes */
> +	file_type rsa_filetype = file_type_bin;
> +
> +        /* Read the image into memory. */
> +	result = read_from_image(filename,
> +				0,
> +				ARSE_RSA_PARAM_MAX_BYTES,
> +				&rsa_storage,
> +				&actual_size,
> +				rsa_filetype);
> +
> +	if (result) {
> +		printf("Error reading file %s.\n", filename);
> +		exit(1);
> +	}
> +
> +	if (actual_size != ARSE_RSA_PARAM_MAX_BYTES) {
> +		printf("Error: invalid size, file %s.\n", filename);
> +		exit(1);
> +        }

I would suggest removing anything from this function that restricts the 
file size to any particular value, since this is generic code, and in 
theory at least each SoC defines its own RSA size parameters.

Looks like an indentation error there too.

> diff --git a/src/t210/nvbctlib_t210.c b/src/t210/nvbctlib_t210.c

> @@ -2198,6 +2201,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:
> +		memcpy(&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.
> +		 */
> +		memcpy(&bct_ptr->bootloader[0].signature.rsa_pss_sig,
> +			data, sizeof(nvboot_rsa_pss_sig));
> +		break;
> +
> +	case token_rsa_pss_sig_bct:
> +		memcpy(&bct_ptr->signature.rsa_pss_sig,
> +			data, sizeof(nvboot_rsa_pss_sig));
> +		break;

Instead, I think the file size can be validated here?

Hmm, I don't see a size parameter to this function:-(

Given that, I think that either:

a) Add a size parameter to this function. This has the disadvantage of 
needing to update a bunch of call sites. I don't know how many; perhaps 
this isn't a big deal?

or:

b) Modify set_rsa_param() so that it doesn't check the file size against 
a single global define ARSE_RSA_PARAM_MAX_BYTES, but rather calls a 
function that's implemented in src/t210/*.c that tells it what size the 
token should be.

This would also solve my other objection that currently set_rsa_param() 
assumes that all the RSA-related parameters must be the same size. While 
that is true at present, I'm not sure that it my be true in all cases in 
the future.

This is probably the most flexible, and not too hard to implement.

or:

c) Remove the duplicate defines (e.g. ARSE_RSA_MAX_MODULUS_SIZE) from 
the T114/124/132 files too. This has the disadvantage that if some 
future chip changes the RSA sizes, we'll have to undo this change at 
that time, and switch back to (a) or (b).

Re: [tegrarcm PATCH v2 2/4] Add support for update pubkey and rsa-pss signatures

[Stephen Warren]

In V3 (which actually should have been labelled v4 I think) none of the 
review comments below were addressed.

On 10/07/2015 10:33 AM, Stephen Warren wrote:
> On 10/02/2015 02:56 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/cbootimage.h b/src/cbootimage.h
>
>> +#define ARSE_RSA_MAX_MODULUS_SIZE    2048
>> +#define ARSE_RSA_PARAM_MAX_BYTES    (ARSE_RSA_MAX_MODULUS_SIZE / 8)
>
> These values are currently defined in the chip-specific location
> src/tNNN/nvboot_bct_tNNN.h, and this patch only removes a single on of
> those 3 copies when creating this new centralized value. At best that
> makes the code inconsistent, and at worst could cause compile errors due
> to multiple definitions.
>
> Instead, can you leave this value in the T210-specific location. More
> related to this below.
>
>> diff --git a/src/parse.c b/src/parse.c
>
>> +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;
>
> That doesn't look like the correct error check. Rather, other function
> that parse filenames (and other fields) appear to do something like the
> following after parsing all the fields:
>
>          /* Parse the end state. */
>          rest = parse_end_state(rest, e_state, MAX_STR_LEN);
>          if (rest == NULL)
>                  return 1;
>
> I suspect that code should be present in this function too?
>
>> 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 actual_size;    /* In bytes */
>> +    file_type rsa_filetype = file_type_bin;
>> +
>> +        /* Read the image into memory. */
>> +    result = read_from_image(filename,
>> +                0,
>> +                ARSE_RSA_PARAM_MAX_BYTES,
>> +                &rsa_storage,
>> +                &actual_size,
>> +                rsa_filetype);
>> +
>> +    if (result) {
>> +        printf("Error reading file %s.\n", filename);
>> +        exit(1);
>> +    }
>> +
>> +    if (actual_size != ARSE_RSA_PARAM_MAX_BYTES) {
>> +        printf("Error: invalid size, file %s.\n", filename);
>> +        exit(1);
>> +        }
>
> I would suggest removing anything from this function that restricts the
> file size to any particular value, since this is generic code, and in
> theory at least each SoC defines its own RSA size parameters.
>
> Looks like an indentation error there too.
>
>> diff --git a/src/t210/nvbctlib_t210.c b/src/t210/nvbctlib_t210.c
>
>> @@ -2198,6 +2201,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:
>> +        memcpy(&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.
>> +         */
>> +        memcpy(&bct_ptr->bootloader[0].signature.rsa_pss_sig,
>> +            data, sizeof(nvboot_rsa_pss_sig));
>> +        break;
>> +
>> +    case token_rsa_pss_sig_bct:
>> +        memcpy(&bct_ptr->signature.rsa_pss_sig,
>> +            data, sizeof(nvboot_rsa_pss_sig));
>> +        break;
>
> Instead, I think the file size can be validated here?
>
> Hmm, I don't see a size parameter to this function:-(
>
> Given that, I think that either:
>
> a) Add a size parameter to this function. This has the disadvantage of
> needing to update a bunch of call sites. I don't know how many; perhaps
> this isn't a big deal?
>
> or:
>
> b) Modify set_rsa_param() so that it doesn't check the file size against
> a single global define ARSE_RSA_PARAM_MAX_BYTES, but rather calls a
> function that's implemented in src/t210/*.c that tells it what size the
> token should be.
>
> This would also solve my other objection that currently set_rsa_param()
> assumes that all the RSA-related parameters must be the same size. While
> that is true at present, I'm not sure that it my be true in all cases in
> the future.
>
> This is probably the most flexible, and not too hard to implement.
>
> or:
>
> c) Remove the duplicate defines (e.g. ARSE_RSA_MAX_MODULUS_SIZE) from
> the T114/124/132 files too. This has the disadvantage that if some
> future chip changes the RSA sizes, we'll have to undo this change at
> that time, and switch back to (a) or (b).

RE: [tegrarcm PATCH v2 2/4] Add support for update pubkey and rsa-pss signatures

[Jimmy Zhang]

> -----Original Message-----
> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> Sent: Wednesday, October 07, 2015 9:34 AM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [tegrarcm PATCH v2 2/4] Add support for update pubkey and
> rsa-pss signatures
> 
> On 10/02/2015 02:56 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/cbootimage.h b/src/cbootimage.h
> 
> > +#define ARSE_RSA_MAX_MODULUS_SIZE	2048
> > +#define ARSE_RSA_PARAM_MAX_BYTES
> 	(ARSE_RSA_MAX_MODULUS_SIZE / 8)
> 
> These values are currently defined in the chip-specific location
> src/tNNN/nvboot_bct_tNNN.h, and this patch only removes a single on of
> those 3 copies when creating this new centralized value. At best that makes
> the code inconsistent, and at worst could cause compile errors due to
> multiple definitions.
> 
> Instead, can you leave this value in the T210-specific location. More related
> to this below.

In this case we can't use get_value() because it returns the value.  What we need here is the size of value.  Will add function * get_value_size().
 
> 
> > diff --git a/src/parse.c b/src/parse.c
> 
> > +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;
> 
> That doesn't look like the correct error check. Rather, other function that
> parse filenames (and other fields) appear to do something like the following
> after parsing all the fields:
> 
>          /* Parse the end state. */
>          rest = parse_end_state(rest, e_state, MAX_STR_LEN);
>          if (rest == NULL)
>                  return 1;
> 
> I suspect that code should be present in this function too?
> 

Here I basically followed function parse_bct_file().

Function parse_end_state() is called in preparing for scanning next string  "Complete". It doesn't apply to this case.

Error checking only checks whether a terminator is found or not. 
 
> > 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 actual_size;	/* In bytes */
> > +	file_type rsa_filetype = file_type_bin;
> > +
> > +        /* Read the image into memory. */
> > +	result = read_from_image(filename,
> > +				0,
> > +				ARSE_RSA_PARAM_MAX_BYTES,
> > +				&rsa_storage,
> > +				&actual_size,
> > +				rsa_filetype);
> > +
> > +	if (result) {
> > +		printf("Error reading file %s.\n", filename);
> > +		exit(1);
> > +	}
> > +
> > +	if (actual_size != ARSE_RSA_PARAM_MAX_BYTES) {
> > +		printf("Error: invalid size, file %s.\n", filename);
> > +		exit(1);
> > +        }
> 
> I would suggest removing anything from this function that restricts the file
> size to any particular value, since this is generic code, and in theory at least
> each SoC defines its own RSA size parameters.
> 
> Looks like an indentation error there too.
> 
> > diff --git a/src/t210/nvbctlib_t210.c b/src/t210/nvbctlib_t210.c
> 
> > @@ -2198,6 +2201,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:
> > +		memcpy(&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.
> > +		 */
> > +		memcpy(&bct_ptr->bootloader[0].signature.rsa_pss_sig,
> > +			data, sizeof(nvboot_rsa_pss_sig));
> > +		break;
> > +
> > +	case token_rsa_pss_sig_bct:
> > +		memcpy(&bct_ptr->signature.rsa_pss_sig,
> > +			data, sizeof(nvboot_rsa_pss_sig));
> > +		break;
> 
> Instead, I think the file size can be validated here?
> 
> Hmm, I don't see a size parameter to this function:-(
> 
> Given that, I think that either:
> 
> a) Add a size parameter to this function. This has the disadvantage of
> needing to update a bunch of call sites. I don't know how many; perhaps this
> isn't a big deal?
> 
> or:
> 
> b) Modify set_rsa_param() so that it doesn't check the file size against a
> single global define ARSE_RSA_PARAM_MAX_BYTES, but rather calls a
> function that's implemented in src/t210/*.c that tells it what size the token
> should be.
> 
> This would also solve my other objection that currently set_rsa_param()
> assumes that all the RSA-related parameters must be the same size. While
> that is true at present, I'm not sure that it my be true in all cases in the
> future.
> 
> This is probably the most flexible, and not too hard to implement.
> 
> or:
> 
> c) Remove the duplicate defines (e.g. ARSE_RSA_MAX_MODULUS_SIZE)
> from the T114/124/132 files too. This has the disadvantage that if some
> future chip changes the RSA sizes, we'll have to undo this change at that
> time, and switch back to (a) or (b).

Will implement option b.

Re: [tegrarcm PATCH v2 2/4] Add support for update pubkey and rsa-pss signatures

[Stephen Warren]

On 10/02/2015 02:56 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/set.c b/src/set.c

> +set_rsa_param(build_image_context *context, parse_token token,
> +		char *filename)

> +	file_type rsa_filetype = file_type_bin;
> +
> +        /* Read the image into memory. */
> +	result = read_from_image(filename,
> +				0,
> +				ARSE_RSA_PARAM_MAX_BYTES,
> +				&rsa_storage,
> +				&actual_size,
> +				rsa_filetype);

Why do we need a variable "ras_filetype"? Why not just pass 
file_type_bin directly as the function parameter?

RE: [tegrarcm PATCH v2 2/4] Add support for update pubkey and rsa-pss signatures

[Jimmy Zhang]

> -----Original Message-----
> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> Sent: Wednesday, October 07, 2015 10:01 AM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [tegrarcm PATCH v2 2/4] Add support for update pubkey and
> rsa-pss signatures
> 
> On 10/02/2015 02:56 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/set.c b/src/set.c
> 
> > +set_rsa_param(build_image_context *context, parse_token token,
> > +		char *filename)
> 
> > +	file_type rsa_filetype = file_type_bin;
> > +
> > +        /* Read the image into memory. */
> > +	result = read_from_image(filename,
> > +				0,
> > +				ARSE_RSA_PARAM_MAX_BYTES,
> > +				&rsa_storage,
> > +				&actual_size,
> > +				rsa_filetype);
> 
> Why do we need a variable "ras_filetype"? Why not just pass file_type_bin
> directly as the function parameter?

Agree. Will clean it up.

Re: [tegrarcm PATCH v2 2/4] Add support for update pubkey and rsa-pss signatures

[Allen Martin]

On Fri, Oct 02, 2015 at 01:56:58PM -0700, Jimmy Zhang wrote:
> 
> 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.

I think this needs some documentation to show the whole flow of how
it's expected to be used in conjunction with openssl.  We don't have a
man page for cbootimage now, but if I create a patch to add one can
you add some instructions there about the whole flow?


-Allen

Re: [tegrarcm PATCH v2 2/4] Add support for update pubkey and rsa-pss signatures

[Stephen Warren]

On 10/07/2015 11:08 AM, Allen Martin wrote:
> On Fri, Oct 02, 2015 at 01:56:58PM -0700, Jimmy Zhang wrote:
>>
>> 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.
>
> I think this needs some documentation to show the whole flow of how
> it's expected to be used in conjunction with openssl.  We don't have a
> man page for cbootimage now, but if I create a patch to add one can
> you add some instructions there about the whole flow?

A README.txt would simplify the process of reading the documentation 
(i.e. plain text rather than man page) or building the SW. At least for 
people reading the docs in the source tree. I suppose if someone 
actually installs the SW rather than simply building it in place, then a 
man page is more convenient:-)

Re: [tegrarcm PATCH v2 2/4] Add support for update pubkey and rsa-pss signatures

[Allen Martin]

On Wed, Oct 07, 2015 at 11:14:13AM -0600, Stephen Warren wrote:
> On 10/07/2015 11:08 AM, Allen Martin wrote:
> >I think this needs some documentation to show the whole flow of how
> >it's expected to be used in conjunction with openssl.  We don't have a
> >man page for cbootimage now, but if I create a patch to add one can
> >you add some instructions there about the whole flow?
> 
> A README.txt would simplify the process of reading the documentation
> (i.e. plain text rather than man page) or building the SW. At least
> for people reading the docs in the source tree. I suppose if someone
> actually installs the SW rather than simply building it in place,
> then a man page is more convenient:-)

We could have a Makefile target to run nroff on the man page to
generate a text file in the src directory.  The interaction with
openssl is an issue for end users, so should be in some documentation
installed for end users (which doesn't exist right now).

-Allen

RE: [tegrarcm PATCH v2 2/4] Add support for update pubkey and rsa-pss signatures

[Jimmy Zhang]

> -----Original Message-----
> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> Sent: Wednesday, October 07, 2015 10:14 AM
> To: Allen Martin; Jimmy Zhang
> Cc: Stephen Warren; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [tegrarcm PATCH v2 2/4] Add support for update pubkey and
> rsa-pss signatures
> 
> On 10/07/2015 11:08 AM, Allen Martin wrote:
> > On Fri, Oct 02, 2015 at 01:56:58PM -0700, Jimmy Zhang wrote:
> >>
> >> 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.
> >
> > I think this needs some documentation to show the whole flow of how
> > it's expected to be used in conjunction with openssl.  We don't have a
> > man page for cbootimage now, but if I create a patch to add one can
> > you add some instructions there about the whole flow?
> 
> A README.txt would simplify the process of reading the documentation (i.e.
> plain text rather than man page) or building the SW. At least for people
> reading the docs in the source tree. I suppose if someone actually installs the
> SW rather than simply building it in place, then a man page is more
> convenient:-)

That will be great.  I thought about where I should submit the sample script that I have tested. I will add it to README.txt and the man page after you create it.

[tegrarcm PATCH v2 3/4] 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           | 38 ++++++++++++++++++++++++++++++++++++++
 src/t210/nvbctlib_t210.c | 16 ++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/src/bct_dump.c b/src/bct_dump.c
index be7b85dc72d6..27e3dbb5e6be 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);
 
@@ -42,6 +44,7 @@ typedef struct {
 typedef union {
 	u_int32_t val;
 	u_int8_t uid[16];
+	u_int8_t rsa_param[256];
 } param_types;
 
 #define MAX_PARAM_SIZE sizeof(param_types)
@@ -54,6 +57,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 +78,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 +116,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 && ((byte_index + 1) % 64 == 0))
+			printf(";\n");
+	}
+
+	if (byte_index && (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 91a42ec8367f..10e2a4756e1a 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,11 @@ t210_getbl_param(u_int32_t set,
 		sizeof(nvboot_hash));
 		break;
 
+	case token_rsa_pss_sig_bl:
+		memcpy(data, &bct_ptr->bootloader[set].signature.rsa_pss_sig,
+			sizeof(nvboot_rsa_pss_sig));
+		break;
+
 	default:
 		return -ENODATA;
 	}
@@ -2130,6 +2137,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:
+		memcpy(data, &bct_ptr->key, sizeof(nvboot_rsa_key_modulus));
+		break;
+
+	case token_rsa_pss_sig_bct:
+		memcpy(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

Re: [tegrarcm PATCH v2 3/4] Add support to dump rsa related fields for t210

[Stephen Warren]

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

It also seems to dump some AES hashes too.

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

> +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 && ((byte_index + 1) % 64 == 0))
> +			printf(";\n");
> +	}
> +
> +	if (byte_index && (byte_index % 64 != 0))
> +			printf(";\n");
> +}

The same comment about hard-coding ARSE_RSA_PARAM_MAX_BYTES applies here 
as in the previous commit.

It would be nice if this function wrote the values to a file. That way, 
the output of bct_dump would be something you could feed into cbootimage 
directly. With this patch, the user has to manually convert the inline 
hex data into a binary file before the output is useful.

Still, solving that might be challenging. If bct_dump wrote to a 
user-supplied filename, the files could simply be named 
"${filename}.RsaKeyModulus" etc. However, since bct_dump prints to 
stdout, that's not possible. Perhaps since bct_dump is mostly a debug 
aid, we can live with this issue for now.

RE: [tegrarcm PATCH v2 3/4] Add support to dump rsa related fields for t210

[Jimmy Zhang]

> -----Original Message-----
> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> Sent: Wednesday, October 07, 2015 9:45 AM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [tegrarcm PATCH v2 3/4] Add support to dump rsa related fields
> for t210
> 
> On 10/02/2015 02:56 PM, Jimmy Zhang wrote:
> > Add support to dump rsa pubkey, bct's rsa-pss signature and
> > bootloader's rsa-pss signature.
> 
> It also seems to dump some AES hashes too.
> 
> > diff --git a/src/bct_dump.c b/src/bct_dump.c
> 
> > +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 && ((byte_index + 1) % 64 == 0))
> > +			printf(";\n");
> > +	}
> > +
> > +	if (byte_index && (byte_index % 64 != 0))
> > +			printf(";\n");
> > +}
> 
> The same comment about hard-coding ARSE_RSA_PARAM_MAX_BYTES
> applies here as in the previous commit.
> 
> It would be nice if this function wrote the values to a file. That way, the
> output of bct_dump would be something you could feed into cbootimage
> directly. With this patch, the user has to manually convert the inline hex data
> into a binary file before the output is useful.
> 
> Still, solving that might be challenging. If bct_dump wrote to a user-supplied
> filename, the files could simply be named "${filename}.RsaKeyModulus" etc.
> However, since bct_dump prints to stdout, that's not possible. Perhaps since
> bct_dump is mostly a debug aid, we can live with this issue for now.

Maybe we should add a command line option to save RsaKey modulus to a given file.  I can come up another patch after this series is merged.

Re: [tegrarcm PATCH v2 3/4] Add support to dump rsa related fields for t210

[Stephen Warren]

On 10/07/2015 12:57 PM, Jimmy Zhang wrote:
>
>
>> -----Original Message-----
>> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
>> Sent: Wednesday, October 07, 2015 9:45 AM
>> To: Jimmy Zhang
>> Cc: Allen Martin; Stephen Warren; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [tegrarcm PATCH v2 3/4] Add support to dump rsa related fields
>> for t210
>>
>> On 10/02/2015 02:56 PM, Jimmy Zhang wrote:
>>> Add support to dump rsa pubkey, bct's rsa-pss signature and
>>> bootloader's rsa-pss signature.
>>
>> It also seems to dump some AES hashes too.
>>
>>> diff --git a/src/bct_dump.c b/src/bct_dump.c
>>
>>> +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 && ((byte_index + 1) % 64 == 0))
>>> +			printf(";\n");
>>> +	}
>>> +
>>> +	if (byte_index && (byte_index % 64 != 0))
>>> +			printf(";\n");
>>> +}
>>
>> The same comment about hard-coding ARSE_RSA_PARAM_MAX_BYTES
>> applies here as in the previous commit.
>>
>> It would be nice if this function wrote the values to a file. That way, the
>> output of bct_dump would be something you could feed into cbootimage
>> directly. With this patch, the user has to manually convert the inline hex data
>> into a binary file before the output is useful.
>>
>> Still, solving that might be challenging. If bct_dump wrote to a user-supplied
>> filename, the files could simply be named "${filename}.RsaKeyModulus" etc.
>> However, since bct_dump prints to stdout, that's not possible. Perhaps since
>> bct_dump is mostly a debug aid, we can live with this issue for now.
>
> Maybe we should add a command line option to save RsaKey modulus to a given file.  I can come up another patch after this series is merged.

That would work. The issue is there are 3 tokens which implies 3 
command-line options (and may be more tokens if we extend cbootimage to 
support N BCT copies, N bootloaders, etc.).

Perhaps best would be:

bct_dump foo.bin
-> Prints results to stdout, with inline dumps of the RSA fields

bct_dump -o foo.cfg foo.bin
-> "Prints" results to foo.cfg, and puts all the RSA fields (as binary) 
into foo.cfg.RsaKeyModulus (and a variety of other auto-named files)?

RE: [tegrarcm PATCH v2 3/4] Add support to dump rsa related fields for t210

[Jimmy Zhang]

> -----Original Message-----
> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> Sent: Wednesday, October 07, 2015 12:29 PM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [tegrarcm PATCH v2 3/4] Add support to dump rsa related fields
> for t210
> 
> On 10/07/2015 12:57 PM, Jimmy Zhang wrote:
> >
> >
> >> -----Original Message-----
> >> From: Stephen Warren [mailto:swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org]
> >> Sent: Wednesday, October 07, 2015 9:45 AM
> >> To: Jimmy Zhang
> >> Cc: Allen Martin; Stephen Warren; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> Subject: Re: [tegrarcm PATCH v2 3/4] Add support to dump rsa related
> >> fields for t210
> >>
> >> On 10/02/2015 02:56 PM, Jimmy Zhang wrote:
> >>> Add support to dump rsa pubkey, bct's rsa-pss signature and
> >>> bootloader's rsa-pss signature.
> >>
> >> It also seems to dump some AES hashes too.
> >>
> >>> diff --git a/src/bct_dump.c b/src/bct_dump.c
> >>
> >>> +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 && ((byte_index + 1) % 64 == 0))
> >>> +			printf(";\n");
> >>> +	}
> >>> +
> >>> +	if (byte_index && (byte_index % 64 != 0))
> >>> +			printf(";\n");
> >>> +}
> >>
> >> The same comment about hard-coding ARSE_RSA_PARAM_MAX_BYTES
> applies
> >> here as in the previous commit.
> >>
> >> It would be nice if this function wrote the values to a file. That
> >> way, the output of bct_dump would be something you could feed into
> >> cbootimage directly. With this patch, the user has to manually
> >> convert the inline hex data into a binary file before the output is useful.
> >>
> >> Still, solving that might be challenging. If bct_dump wrote to a
> >> user-supplied filename, the files could simply be named
> "${filename}.RsaKeyModulus" etc.
> >> However, since bct_dump prints to stdout, that's not possible.
> >> Perhaps since bct_dump is mostly a debug aid, we can live with this issue
> for now.
> >
> > Maybe we should add a command line option to save RsaKey modulus to a
> given file.  I can come up another patch after this series is merged.
> 
> That would work. The issue is there are 3 tokens which implies 3 command-
> line options (and may be more tokens if we extend cbootimage to support N
> BCT copies, N bootloaders, etc.).
> 
> Perhaps best would be:
> 
> bct_dump foo.bin
> -> Prints results to stdout, with inline dumps of the RSA fields
> 
> bct_dump -o foo.cfg foo.bin
> -> "Prints" results to foo.cfg, and puts all the RSA fields (as binary)
> into foo.cfg.RsaKeyModulus (and a variety of other auto-named files)?

OK. Will do.

[tegrarcm PATCH v2 4/4] Add new configuration keyword "ReSignBl"

[Jimmy Zhang]

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

How to use:
  Add keyword "ReSignBl" to configuration file, for example resign.cfg:
    ReSignBl;

  Invoke cbootimage to resign image, for example bootloader.bin:
    $ cbootimage -s tegra210 --update resign.cfg bootloader.bin bootloader.bin-resigned

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

Signed-off-by: Jimmy Zhang <jimmzhang-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 src/cbootimage.h  |  1 +
 src/crypto.c      | 34 ++++++++++++++++++++++++++++++++++
 src/crypto.h      |  6 ++++++
 src/data_layout.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/data_layout.h |  2 ++
 src/parse.c       |  9 +++++++++
 src/parse.h       |  1 +
 7 files changed, 104 insertions(+)

diff --git a/src/cbootimage.h b/src/cbootimage.h
index 1ce8af6f6584..98aa8f16d8f1 100644
--- a/src/cbootimage.h
+++ b/src/cbootimage.h
@@ -64,6 +64,7 @@ typedef enum
 	file_type_bct,
 	file_type_mts,
 	file_type_bin,
+	file_type_blocks,
 } file_type;
 
 /*
diff --git a/src/crypto.c b/src/crypto.c
index 99e9f085763c..d6889cb602c9 100644
--- a/src/crypto.c
+++ b/src/crypto.c
@@ -297,3 +297,37 @@ sign_bct(build_image_context *context,
 	free(hash_buffer);
 	return e;
 }
+
+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 d7151e0cd191..936ca9c4c0eb 100644
--- a/src/crypto.h
+++ b/src/crypto.h
@@ -44,4 +44,10 @@ sign_data_block(u_int8_t *source,
 		u_int32_t length,
 		u_int8_t *signature);
 
+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..fe5380ac94bb 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_blocks)) {
+		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;
+}
\ No newline at end of file
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..55d4125b7cb2 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 },
+	{ "ReSignBl",       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 16242a5c2701..69f7abe1d405 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

Re: [tegrarcm PATCH v2 4/4] Add new configuration keyword "ReSignBl"

[Stephen Warren]

On 10/02/2015 02:57 PM, Jimmy Zhang wrote:
> This feature is needed in case an image is updated at later stage
> after it has been created.
>
> How to use:
>    Add keyword "ReSignBl" to configuration file, for example resign.cfg:
>      ReSignBl;
>
>    Invoke cbootimage to resign image, for example bootloader.bin:
>      $ cbootimage -s tegra210 --update resign.cfg bootloader.bin bootloader.bin-resigned
>
>    Where bootloader.bin-resigned is the resigned bootloader.bin

Since the public key signing code has all been moved outside of 
cbootimage, I think this feature is now just recomputing the AES hash. 
I'm not sure that signing is the correct word now, is it? I wonder if 
the keyword should be RehashBl rather than ReSignBl?

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

> @@ -64,6 +64,7 @@ typedef enum
>   	file_type_bct,
>   	file_type_mts,
>   	file_type_bin,
> +	file_type_blocks,
>   } file_type;

The only place this is used is as a parameter to read_from_image(). That 
function only seems to care whether this parameter is equal to 
file_type_bl or not. Doesn't re-using file_type_bin make sense?

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

> +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);

Ah, so there's already a function that can return the size of various 
objects in the BCT. That will make option (b) in my review of patch 2 
much easier then...

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

> +int resign_bl(build_image_context *context)
...
> +}
> \ No newline at end of file

There should be one.

RE: [tegrarcm PATCH v2 4/4] Add new configuration keyword "ReSignBl"

[Jimmy Zhang]

> -----Original Message-----
> From: linux-tegra-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-tegra-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Stephen Warren
> Sent: Wednesday, October 07, 2015 10:11 AM
> To: Jimmy Zhang
> Cc: Allen Martin; Stephen Warren; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [tegrarcm PATCH v2 4/4] Add new configuration keyword
> "ReSignBl"
> 
> On 10/02/2015 02:57 PM, Jimmy Zhang wrote:
> > This feature is needed in case an image is updated at later stage
> > after it has been created.
> >
> > How to use:
> >    Add keyword "ReSignBl" to configuration file, for example resign.cfg:
> >      ReSignBl;
> >
> >    Invoke cbootimage to resign image, for example bootloader.bin:
> >      $ cbootimage -s tegra210 --update resign.cfg bootloader.bin
> > bootloader.bin-resigned
> >
> >    Where bootloader.bin-resigned is the resigned bootloader.bin
> 
> Since the public key signing code has all been moved outside of cbootimage, I
> think this feature is now just recomputing the AES hash.
> I'm not sure that signing is the correct word now, is it? I wonder if the
> keyword should be RehashBl rather than ReSignBl?
>

Agree. 
 
> > diff --git a/src/cbootimage.h b/src/cbootimage.h
> 
> > @@ -64,6 +64,7 @@ typedef enum
> >   	file_type_bct,
> >   	file_type_mts,
> >   	file_type_bin,
> > +	file_type_blocks,
> >   } file_type;
> 
> The only place this is used is as a parameter to read_from_image(). That
> function only seems to care whether this parameter is equal to file_type_bl
> or not. Doesn't re-using file_type_bin make sense?
> 
OK

> > diff --git a/src/crypto.c b/src/crypto.c
> 
> > +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);
> 
> Ah, so there's already a function that can return the size of various objects in
> the BCT. That will make option (b) in my review of patch 2 much easier then...
> 

Not sure what you mean exactly.

> > diff --git a/src/data_layout.c b/src/data_layout.c
> 
> > +int resign_bl(build_image_context *context)
> ...
> > +}
> > \ No newline at end of file
> 
> There should be one.

OK.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the
> body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

Re: [tegrarcm PATCH v2 4/4] Add new configuration keyword "ReSignBl"

[Stephen Warren]

On 10/07/2015 04:45 PM, Jimmy Zhang wrote:
> Stephen Warren wrote at Wednesday, October 07, 2015 10:11 AM:
>> On 10/02/2015 02:57 PM, Jimmy Zhang wrote:
>>> This feature is needed in case an image is updated at later stage
>>> after it has been created.
>>>
>>> How to use:
>>>     Add keyword "ReSignBl" to configuration file, for example resign.cfg:
>>>       ReSignBl;
>>>
>>>     Invoke cbootimage to resign image, for example bootloader.bin:
>>>       $ cbootimage -s tegra210 --update resign.cfg bootloader.bin
>>> bootloader.bin-resigned
>>>
>>>     Where bootloader.bin-resigned is the resigned bootloader.bin

>>> diff --git a/src/crypto.c b/src/crypto.c
>>
>>> +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);
>>
>> Ah, so there's already a function that can return the size of various objects in
>> the BCT. That will make option (b) in my review of patch 2 much easier then...
>
> Not sure what you mean exactly.

When I reviewed patch 2/4 I proposed 3 options for a change to ensure 
that t210_bct_set_value() wasn't tied to chip-specific RSA parameter 
sizes. Option (b) relied on t210_bct_set_value() calling into an 
SoC-specific function to retrieve the RSA parameter sizes, which might 
have meant creating new infra-structure to allow such a call. However, 
given that g_soc_config->get_value() already exists, and is already used 
by core code to retrieve the SoC-specific size of some objects, it turns 
out that implementing option (b) should actually be trivial. Hence, it's 
the best option.