diff mbox

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

Message ID 1446739402-14238-3-git-send-email-alban.bedel@avionic-design.de
State Deferred
Delegated to: Stephen Warren
Headers show

Commit Message

Alban Bedel Nov. 5, 2015, 4:03 p.m. UTC 
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@avionic-design.de>
---
 src/cbootimage.h  | 1 +
 src/data_layout.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

Comments

Stephen Warren Nov. 11, 2015, 4:36 p.m. UTC | #1 
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?

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alban Bedel Dec. 7, 2015, 11:51 a.m. UTC | #2 
On Wed, 11 Nov 2015 09:36:37 -0700
Stephen Warren <swarren@wwwdotorg.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
Stephen Warren Dec. 7, 2015, 5:20 p.m. UTC | #3 
On 12/07/2015 04:51 AM, Alban Bedel wrote:
> On Wed, 11 Nov 2015 09:36:37 -0700
> Stephen Warren <swarren@wwwdotorg.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; <<<<<<<<


--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alban Bedel Dec. 8, 2015, 2:27 p.m. UTC | #4 
On Mon, 7 Dec 2015 10:20:56 -0700
Stephen Warren <swarren@wwwdotorg.org> wrote:

> On 12/07/2015 04:51 AM, Alban Bedel wrote:
> > On Wed, 11 Nov 2015 09:36:37 -0700
> > Stephen Warren <swarren@wwwdotorg.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
diff mbox

Patch

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