[cbootimage PATCH 0/3] RSA on T124 and misc fixes

[cbootimage PATCH 0/3] RSA on T124 and misc fixes

[Alban Bedel]

Hi all,

this small series fix a few bugs when updating small images and add support
for the RSA fields on T124. Using these patches and an adapted version of
the sign.sh script I was able to generate images that works properly on a
fused SoC.

Alban

Alban Bedel (3):
  Fix the error reporting of get_bct_size_from_image()
  Fix image update with image smaller than 10KiB
  Add support to read and write rsa related fields on t124

 src/cbootimage.c         |  2 +-
 src/cbootimage.h         |  1 +
 src/data_layout.c        |  6 ++---
 src/t124/nvbctlib_t124.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++--
 4 files changed, 69 insertions(+), 6 deletions(-)

-- 
2.6.2

[cbootimage PATCH 1/3] Fix the error reporting of get_bct_size_from_image()

[Alban Bedel]

get_bct_size_from_image() should return negative error codes, so add
the missing minus signs. Also fix the return value check on
get_bct_size_from_image(), a negative value indicate an error not zero.

Signed-off-by: Alban Bedel <alban.bedel-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
 src/cbootimage.c  | 2 +-
 src/data_layout.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/cbootimage.c b/src/cbootimage.c
index da1d1a5..fc99af2 100644
--- a/src/cbootimage.c
+++ b/src/cbootimage.c
@@ -239,7 +239,7 @@ main(int argc, char *argv[])
 
 		/* Get BCT_SIZE from input image file  */
 		bct_size = get_bct_size_from_image(&context);
-		if (!bct_size) {
+		if (bct_size < 0) {
 			printf("Error: Invalid input image file %s\n",
 			context.input_image_filename);
 			goto fail;
diff --git a/src/data_layout.c b/src/data_layout.c
index 5d3fe10..e91d13c 100644
--- a/src/data_layout.c
+++ b/src/data_layout.c
@@ -1050,11 +1050,11 @@ int get_bct_size_from_image(build_image_context *context)
 
 	fp = fopen(context->input_image_filename, "r");
 	if (!fp)
-		return ENODATA;
+		return -ENODATA;
 
 	if (fread(buffer, 1, NVBOOT_CONFIG_TABLE_SIZE_MAX, fp) != NVBOOT_CONFIG_TABLE_SIZE_MAX) {
 		fclose(fp);
-		return ENODATA;
+		return -ENODATA;
 	}
 
 	context->bct = buffer;
-- 
2.6.2

Re: [cbootimage PATCH 1/3] Fix the error reporting of get_bct_size_from_image()

[Stephen Warren]

On 11/05/2015 09:03 AM, Alban Bedel wrote:
> get_bct_size_from_image() should return negative error codes, so add
> the missing minus signs. Also fix the return value check on
> get_bct_size_from_image(), a negative value indicate an error not zero.

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

> @@ -1050,11 +1050,11 @@ int get_bct_size_from_image(build_image_context *context)
>
>   	fp = fopen(context->input_image_filename, "r");
>   	if (!fp)
> -		return ENODATA;
> +		return -ENODATA;

I see the exact same bug in read_bct_file() in the same source file. 
Since that's the same logical bug, any chance you could fix that too in 
the same patch?

[cbootimage PATCH 2/3] Fix image update with image smaller than 10KiB

[Alban Bedel]

The BCT size check assume a quiet large image, however if the image
doesn't contains a bootloader it won't be that large. Change the size
check to check for the smallest possible BCT size which is currently
the T20 BCT.

Signed-off-by: Alban Bedel <alban.bedel-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
 src/cbootimage.h  | 1 +
 src/data_layout.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/cbootimage.h b/src/cbootimage.h
index 63f0ee9..b94ed52 100644
--- a/src/cbootimage.h
+++ b/src/cbootimage.h
@@ -49,6 +49,7 @@
 
 #define MAX_MTS_SIZE (4 * 1024 * 1024)
 
+#define NVBOOT_CONFIG_TABLE_SIZE_MIN 4080
 #define NVBOOT_CONFIG_TABLE_SIZE_MAX (10 * 1024)
 
 /*
diff --git a/src/data_layout.c b/src/data_layout.c
index e91d13c..8ac7ddf 100644
--- a/src/data_layout.c
+++ b/src/data_layout.c
@@ -1052,7 +1052,7 @@ int get_bct_size_from_image(build_image_context *context)
 	if (!fp)
 		return -ENODATA;
 
-	if (fread(buffer, 1, NVBOOT_CONFIG_TABLE_SIZE_MAX, fp) != NVBOOT_CONFIG_TABLE_SIZE_MAX) {
+	if (fread(buffer, 1, NVBOOT_CONFIG_TABLE_SIZE_MAX, fp) < NVBOOT_CONFIG_TABLE_SIZE_MIN) {
 		fclose(fp);
 		return -ENODATA;
 	}
-- 
2.6.2

Re: [cbootimage PATCH 2/3] Fix image update with image smaller than 10KiB

[Stephen Warren]

On 11/05/2015 09:03 AM, Alban Bedel wrote:
> The BCT size check assume a quiet large image, however if the image
> doesn't contains a bootloader it won't be that large. Change the size
> check to check for the smallest possible BCT size which is currently

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

> +#define NVBOOT_CONFIG_TABLE_SIZE_MIN 4080

I think a comment is warranted here. This value needs to be (a) small 
enough that it isn't larger than the total BCT size on any chip, and (b) 
large enough that it includes the bct->boot_data_version field for all 
chips. (Hopefully those two constraints can continue to be met with a 
single value in the future...)

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

> @@ -1052,7 +1052,7 @@ int get_bct_size_from_image(build_image_context *context)
>   	if (!fp)
>   		return -ENODATA;
>
> -	if (fread(buffer, 1, NVBOOT_CONFIG_TABLE_SIZE_MAX, fp) != NVBOOT_CONFIG_TABLE_SIZE_MAX) {
> +	if (fread(buffer, 1, NVBOOT_CONFIG_TABLE_SIZE_MAX, fp) < NVBOOT_CONFIG_TABLE_SIZE_MIN) {

Can you please also update the size of buffer[]:

int get_bct_size_from_image(build_image_context *context)
{
         u_int8_t buffer[NVBOOT_CONFIG_TABLE_SIZE_MAX];

I wonder if it's worth updating all SoCs' versions of 
if_bct_is_tNNN_get_soc_config() so that they validate that the end 
offset of bct->boot_data_version is < NVBOOT_CONFIG_TABLE_SIZE_MIN, or 
perhaps that the offset is < context->bct_size, in which case 
get_bct_size_from_image() would need to be enhanced to set/clear that 
value when setting/clearing context->bct?

Re: [cbootimage PATCH 2/3] Fix image update with image smaller than 10KiB

[Alban Bedel]

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

On Wed, 11 Nov 2015 09:36:37 -0700
Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:

> On 11/05/2015 09:03 AM, Alban Bedel wrote:
> > The BCT size check assume a quiet large image, however if the image
> > doesn't contains a bootloader it won't be that large. Change the size
> > check to check for the smallest possible BCT size which is currently
> 
> > diff --git a/src/cbootimage.h b/src/cbootimage.h
> 
> > +#define NVBOOT_CONFIG_TABLE_SIZE_MIN 4080
> 
> I think a comment is warranted here. This value needs to be (a) small 
> enough that it isn't larger than the total BCT size on any chip, and (b) 
> large enough that it includes the bct->boot_data_version field for all 
> chips. (Hopefully those two constraints can continue to be met with a 
> single value in the future...)

I'll add this in the next patch.

> > diff --git a/src/data_layout.c b/src/data_layout.c
> 
> > @@ -1052,7 +1052,7 @@ int get_bct_size_from_image(build_image_context *context)
> >   	if (!fp)
> >   		return -ENODATA;
> >
> > -	if (fread(buffer, 1, NVBOOT_CONFIG_TABLE_SIZE_MAX, fp) != NVBOOT_CONFIG_TABLE_SIZE_MAX) {
> > +	if (fread(buffer, 1, NVBOOT_CONFIG_TABLE_SIZE_MAX, fp) < NVBOOT_CONFIG_TABLE_SIZE_MIN) {
> 
> Can you please also update the size of buffer[]:

No, we still need to read up to NVBOOT_CONFIG_TABLE_SIZE_MAX if needed.

> int get_bct_size_from_image(build_image_context *context)
> {
>          u_int8_t buffer[NVBOOT_CONFIG_TABLE_SIZE_MAX];
> 
> I wonder if it's worth updating all SoCs' versions of 
> if_bct_is_tNNN_get_soc_config() so that they validate that the end 
> offset of bct->boot_data_version is < NVBOOT_CONFIG_TABLE_SIZE_MIN, or 
> perhaps that the offset is < context->bct_size, in which case 
> get_bct_size_from_image() would need to be enhanced to set/clear that 
> value when setting/clearing context->bct?

It would be better but it is currently not really needed. Currently the
furthest boot_data_version field is on T124/T132 at an offset of 1744
bytes. Still quiet far from the 4080 bytes that are needed for the
smallest BCT.

Alban

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

Re: [cbootimage PATCH 2/3] Fix image update with image smaller than 10KiB

[Stephen Warren]

On 12/07/2015 04:51 AM, Alban Bedel wrote:
> On Wed, 11 Nov 2015 09:36:37 -0700
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>
>> On 11/05/2015 09:03 AM, Alban Bedel wrote:
>>> The BCT size check assume a quiet large image, however if the image
>>> doesn't contains a bootloader it won't be that large. Change the size
>>> check to check for the smallest possible BCT size which is currently
>>
>>> diff --git a/src/cbootimage.h b/src/cbootimage.h
>>
>>> +#define NVBOOT_CONFIG_TABLE_SIZE_MIN 4080
>>
>> I think a comment is warranted here. This value needs to be (a) small
>> enough that it isn't larger than the total BCT size on any chip, and (b)
>> large enough that it includes the bct->boot_data_version field for all
>> chips. (Hopefully those two constraints can continue to be met with a
>> single value in the future...)
>
> I'll add this in the next patch.
>
>>> diff --git a/src/data_layout.c b/src/data_layout.c
>>
>>> @@ -1052,7 +1052,7 @@ int get_bct_size_from_image(build_image_context *context)
>>>    	if (!fp)
>>>    		return -ENODATA;
>>>
>>> -	if (fread(buffer, 1, NVBOOT_CONFIG_TABLE_SIZE_MAX, fp) != NVBOOT_CONFIG_TABLE_SIZE_MAX) {
>>> +	if (fread(buffer, 1, NVBOOT_CONFIG_TABLE_SIZE_MAX, fp) < NVBOOT_CONFIG_TABLE_SIZE_MIN) {
>>
>> Can you please also update the size of buffer[]:
>
> No, we still need to read up to NVBOOT_CONFIG_TABLE_SIZE_MAX if needed.

Why? This data that's read is solely used to determine the size of the 
BCT, and then thrown away. I don't believe any of the data between 
SIZE_MIN and SIZE_MAX should ever be used; observe the following at the 
end of the function:

         context->bct = buffer;
         if (data_is_valid_bct(context) && g_soc_config->get_bct_size)
                 bct_size = g_soc_config->get_bct_size();

         fclose(fp);
         context->bct = 0; <<<<<<<<

Re: [cbootimage PATCH 2/3] Fix image update with image smaller than 10KiB

[Alban Bedel]

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

On Mon, 7 Dec 2015 10:20:56 -0700
Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:

> On 12/07/2015 04:51 AM, Alban Bedel wrote:
> > On Wed, 11 Nov 2015 09:36:37 -0700
> > Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> >
> >> On 11/05/2015 09:03 AM, Alban Bedel wrote:
> >>> The BCT size check assume a quiet large image, however if the image
> >>> doesn't contains a bootloader it won't be that large. Change the size
> >>> check to check for the smallest possible BCT size which is currently
> >>
> >>> diff --git a/src/cbootimage.h b/src/cbootimage.h
> >>
> >>> +#define NVBOOT_CONFIG_TABLE_SIZE_MIN 4080
> >>
> >> I think a comment is warranted here. This value needs to be (a) small
> >> enough that it isn't larger than the total BCT size on any chip, and (b)
> >> large enough that it includes the bct->boot_data_version field for all
> >> chips. (Hopefully those two constraints can continue to be met with a
> >> single value in the future...)
> >
> > I'll add this in the next patch.
> >
> >>> diff --git a/src/data_layout.c b/src/data_layout.c
> >>
> >>> @@ -1052,7 +1052,7 @@ int get_bct_size_from_image(build_image_context *context)
> >>>    	if (!fp)
> >>>    		return -ENODATA;
> >>>
> >>> -	if (fread(buffer, 1, NVBOOT_CONFIG_TABLE_SIZE_MAX, fp) != NVBOOT_CONFIG_TABLE_SIZE_MAX) {
> >>> +	if (fread(buffer, 1, NVBOOT_CONFIG_TABLE_SIZE_MAX, fp) < NVBOOT_CONFIG_TABLE_SIZE_MIN) {
> >>
> >> Can you please also update the size of buffer[]:
> >
> > No, we still need to read up to NVBOOT_CONFIG_TABLE_SIZE_MAX if needed.
> 
> Why? This data that's read is solely used to determine the size of the 
> BCT, and then thrown away. I don't believe any of the data between 
> SIZE_MIN and SIZE_MAX should ever be used; observe the following at the 
> end of the function:
> 
>          context->bct = buffer;
>          if (data_is_valid_bct(context) && g_soc_config->get_bct_size)
>                  bct_size = g_soc_config->get_bct_size();
> 
>          fclose(fp);
>          context->bct = 0; <<<<<<<<

Right, I overlooked that, v3 is under way.

Alban


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

[cbootimage PATCH 3/3] Add support to read and write rsa related fields on t124

[Alban Bedel]

This allow creating and reading signed images for secure boot on t124.

Signed-off-by: Alban Bedel <alban.bedel-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
 src/t124/nvbctlib_t124.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 64 insertions(+), 2 deletions(-)

diff --git a/src/t124/nvbctlib_t124.c b/src/t124/nvbctlib_t124.c
index 5b760ad..ce0a34b 100644
--- a/src/t124/nvbctlib_t124.c
+++ b/src/t124/nvbctlib_t124.c
@@ -113,7 +113,10 @@ parse_token t124_root_token_list[] = {
 	token_crypto_length,
 	token_max_bct_search_blks,
 	token_unique_chip_id,
-	token_secure_jtag_control
+	token_secure_jtag_control,
+	token_rsa_key_modulus,
+	token_rsa_pss_sig_bl,
+	token_rsa_pss_sig_bct
 };
 
 int
@@ -876,6 +879,12 @@ t124_getbl_param(u_int32_t set,
 		sizeof(nvboot_hash));
 		break;
 
+	case token_rsa_pss_sig_bl:
+		reverse_byte_order((u_int8_t *)data,
+			(const u_int8_t *)&bct_ptr->bootloader[set].signature.rsa_pss_sig,
+			sizeof(nvboot_rsa_pss_sig));
+		break;
+
 	default:
 		return -ENODATA;
 	}
@@ -974,6 +983,17 @@ t124_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:
+		reverse_byte_order(data, (const u_int8_t *)&bct_ptr->key,
+				sizeof(nvboot_rsa_key_modulus));
+		break;
+
+	case token_rsa_pss_sig_bct:
+		reverse_byte_order(data,
+			(const u_int8_t *)&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;
@@ -1020,6 +1040,28 @@ t124_bct_get_value(parse_token id, void *data, u_int8_t *bct)
 }
 
 int
+t124_bct_get_value_size(parse_token id)
+{
+	switch (id) {
+	case token_rsa_key_modulus:
+		return sizeof(nvboot_rsa_key_modulus);
+
+	case token_rsa_pss_sig_bl:
+		return sizeof(nvboot_rsa_pss_sig);
+
+	case token_rsa_pss_sig_bct:
+		return sizeof(nvboot_rsa_pss_sig);
+
+	/*
+	 * Other bct fields can be added in when needed
+	 */
+	default:
+		return -ENODATA;
+	}
+	return 0;
+}
+
+int
 t124_bct_set_value(parse_token id, void *data, u_int8_t *bct)
 {
 	nvboot_config_table *bct_ptr = (nvboot_config_table *)bct;
@@ -1044,6 +1086,26 @@ t124_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:
+		reverse_byte_order((u_int8_t *)&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.
+		 */
+		reverse_byte_order(
+			(u_int8_t *)&bct_ptr->bootloader[0].signature.rsa_pss_sig,
+			data, sizeof(nvboot_rsa_pss_sig));
+		break;
+
+	case token_rsa_pss_sig_bct:
+		reverse_byte_order((u_int8_t *)&bct_ptr->signature.rsa_pss_sig,
+			data, sizeof(nvboot_rsa_pss_sig));
+		break;
+
 	default:
 		return -ENODATA;
 	}
@@ -1125,7 +1187,7 @@ cbootimage_soc_config tegra124_config = {
 	.getbl_param				= t124_getbl_param,
 	.set_value					= t124_bct_set_value,
 	.get_value					= t124_bct_get_value,
-	.get_value_size					= bct_get_unsupported,
+	.get_value_size					= t124_bct_get_value_size,
 	.set_data					= t124_bct_set_data,
 	.get_bct_size				= t124_get_bct_size,
 	.token_supported			= t124_bct_token_supported,
-- 
2.6.2

Re: [cbootimage PATCH 3/3] Add support to read and write rsa related fields on t124

[Stephen Warren]

On 11/05/2015 09:03 AM, Alban Bedel wrote:
> This allow creating and reading signed images for secure boot on t124.

This patch looks like it's identical to the equivalent code for T210. 
Does it depend on your other two patches, or is it independent, such 
that I can apply it now?

It'd be nice if we could share code between src/tNNN/nvbctlib_tNNN.c 
somehow rather than duplicating it all, but that's not a problem this 
patch introduces, so no need to attempt that now.

Re: [cbootimage PATCH 3/3] Add support to read and write rsa related fields on t124

[Alban Bedel]

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

On Wed, 11 Nov 2015 09:41:08 -0700
Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:

> On 11/05/2015 09:03 AM, Alban Bedel wrote:
> > This allow creating and reading signed images for secure boot on t124.
> 
> This patch looks like it's identical to the equivalent code for T210. 
> Does it depend on your other two patches, or is it independent, such 
> that I can apply it now?

Yes, this can be applied without the rest.

> It'd be nice if we could share code between src/tNNN/nvbctlib_tNNN.c 
> somehow rather than duplicating it all, but that's not a problem this 
> patch introduces, so no need to attempt that now.

That would be quiet nice, but that would probably require quiet a large
rework of the current abstraction. I suspect it would need to move to
some description of the BCT that include the offset of each field. That
way the parsers/set/get functions wouldn't need to be reimplemented for
each BCT version.

Alban

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

Re: [cbootimage PATCH 3/3] Add support to read and write rsa related fields on t124

[Stephen Warren]

On 11/11/2015 09:56 AM, Alban Bedel wrote:
> On Wed, 11 Nov 2015 09:41:08 -0700
> Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>
>> On 11/05/2015 09:03 AM, Alban Bedel wrote:
>>> This allow creating and reading signed images for secure boot on t124.
>>
>> This patch looks like it's identical to the equivalent code for T210.
>> Does it depend on your other two patches, or is it independent, such
>> that I can apply it now?
>
> Yes, this can be applied without the rest.

OK, I've applied this patch.

>> It'd be nice if we could share code between src/tNNN/nvbctlib_tNNN.c
>> somehow rather than duplicating it all, but that's not a problem this
>> patch introduces, so no need to attempt that now.
>
> That would be quiet nice, but that would probably require quiet a large
> rework of the current abstraction. I suspect it would need to move to
> some description of the BCT that include the offset of each field. That
> way the parsers/set/get functions wouldn't need to be reimplemented for
> each BCT version.

My idea was to compile a single C file once per SoC, with the 
pre-processor include path set differently each time in order to pick up 
a per-SoC header containing the definition of the BCT structure type, 
plus some configuration options to indicate which fields exist in the 
structure, in order to ifdef out support for missing fields. I've seen 
this technique used in other places and it worked out well there.

Re: [cbootimage PATCH 0/3] RSA on T124 and misc fixes

[Stephen Warren]

On 11/05/2015 09:03 AM, Alban Bedel wrote:
> Hi all,
>
> this small series fix a few bugs when updating small images and add support
> for the RSA fields on T124. Using these patches and an adapted version of
> the sign.sh script I was able to generate images that works properly on a
> fused SoC.

Nit: Note that essentially all Tegra SoCs are "fused" in some way. The 
question is more whether an SBK/PKC has been enabled (via fuses).