[U-Boot] [PATCH 1/3] cmd: cbfs: fix reading the end_of_rom pointer for 64bit archs

[U-Boot] [PATCH 1/3] cmd: cbfs: fix reading the end_of_rom pointer for 64bit archs

[Andre Heider]

The cast breaks the pointer on 64bit archs, so lets get rid of it.

Signed-off-by: Andre Heider <a.heider@gmail.com>
---
 cmd/cbfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmd/cbfs.c b/cmd/cbfs.c
index 799ba01fcc..736f8c4527 100644
--- a/cmd/cbfs.c
+++ b/cmd/cbfs.c
@@ -22,7 +22,7 @@ static int do_cbfs_init(cmd_tbl_t *cmdtp, int flag, int argc,
 		return 0;
 	}
 	if (argc == 2) {
-		end_of_rom = (int)simple_strtoul(argv[1], &ep, 16);
+		end_of_rom = simple_strtoul(argv[1], &ep, 16);
 		if (*ep) {
 			puts("\n** Invalid end of ROM **\n");
 			return 1;
-- 
2.16.1

[U-Boot] [PATCH 2/3] fs: cbfs: fix locating the cbfs header

[Andre Heider]

The value at the end of the rom is not a pointer, it is an offset
relative to the end of rom.

Signed-off-by: Andre Heider <a.heider@gmail.com>
---
 fs/cbfs/cbfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
index 6e1107d751..46da8f134f 100644
--- a/fs/cbfs/cbfs.c
+++ b/fs/cbfs/cbfs.c
@@ -168,9 +168,9 @@ static int file_cbfs_load_header(uintptr_t end_of_rom,
 				 struct cbfs_header *header)
 {
 	struct cbfs_header *header_in_rom;
+	int32_t offset = *(u32 *)(end_of_rom - 3);
 
-	header_in_rom = (struct cbfs_header *)(uintptr_t)
-			*(u32 *)(end_of_rom - 3);
+	header_in_rom = (struct cbfs_header *)(end_of_rom + offset + 1);
 	swap_header(header, header_in_rom);
 
 	if (header->magic != good_magic || header->offset >
-- 
2.16.1

[U-Boot] [PATCH 2/3] fs: cbfs: fix locating the cbfs header

[Alexander Graf]

On 15.02.18 07:40, Andre Heider wrote:
> The value at the end of the rom is not a pointer, it is an offset
> relative to the end of rom.

Do you have any documentation pointers to this? Even just pointing to a
specific line of code in coreboot would be helpful in case anyone wants
to read it up.

> 
> Signed-off-by: Andre Heider <a.heider@gmail.com>
> ---
>  fs/cbfs/cbfs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
> index 6e1107d751..46da8f134f 100644
> --- a/fs/cbfs/cbfs.c
> +++ b/fs/cbfs/cbfs.c
> @@ -168,9 +168,9 @@ static int file_cbfs_load_header(uintptr_t end_of_rom,
>  				 struct cbfs_header *header)
>  {
>  	struct cbfs_header *header_in_rom;
> +	int32_t offset = *(u32 *)(end_of_rom - 3);

Accessing a pointer that gets subtracted by 3 looks terribly wrong.
Unfortunately it's correct, because the pointer itself is unaligned.

How about we change the logic so that we calculate an aligned pointer
(after_rom?) which we sanity check for alignment and base all
calculations on that instead?

That way the next time someone comes around he's not getting puzzled why
we're subtracting 3 and adding 1 to the pointer ;).


Alex

>  
> -	header_in_rom = (struct cbfs_header *)(uintptr_t)
> -			*(u32 *)(end_of_rom - 3);
> +	header_in_rom = (struct cbfs_header *)(end_of_rom + offset + 1);
>  	swap_header(header, header_in_rom);
>  
>  	if (header->magic != good_magic || header->offset >
> 

[U-Boot] [PATCH 2/3] fs: cbfs: fix locating the cbfs header

[Simon Glass]

Hi,

On 22 February 2018 at 08:08, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 15.02.18 07:40, Andre Heider wrote:
>> The value at the end of the rom is not a pointer, it is an offset
>> relative to the end of rom.
>
> Do you have any documentation pointers to this? Even just pointing to a
> specific line of code in coreboot would be helpful in case anyone wants
> to read it up.
>
>>
>> Signed-off-by: Andre Heider <a.heider@gmail.com>
>> ---
>>  fs/cbfs/cbfs.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
>> index 6e1107d751..46da8f134f 100644
>> --- a/fs/cbfs/cbfs.c
>> +++ b/fs/cbfs/cbfs.c
>> @@ -168,9 +168,9 @@ static int file_cbfs_load_header(uintptr_t end_of_rom,
>>                                struct cbfs_header *header)
>>  {
>>       struct cbfs_header *header_in_rom;
>> +     int32_t offset = *(u32 *)(end_of_rom - 3);
>
> Accessing a pointer that gets subtracted by 3 looks terribly wrong.
> Unfortunately it's correct, because the pointer itself is unaligned.
>
> How about we change the logic so that we calculate an aligned pointer
> (after_rom?) which we sanity check for alignment and base all
> calculations on that instead?
>
> That way the next time someone comes around he's not getting puzzled why
> we're subtracting 3 and adding 1 to the pointer ;).

Either that or a comment would be nice. But since this has been
sitting around for a while and fixes a bug I think it is best to take
it. Please feel free to send a follow-up patch..

We also have no tests for this code, so I'd really like to get some
tests in there!

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

[U-Boot] [PATCH 2/3] fs: cbfs: fix locating the cbfs header

[Simon Glass]

On 1 April 2018 at 22:19, Simon Glass <sjg@chromium.org> wrote:
> Hi,
>
> On 22 February 2018 at 08:08, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 15.02.18 07:40, Andre Heider wrote:
>>> The value at the end of the rom is not a pointer, it is an offset
>>> relative to the end of rom.
>>
>> Do you have any documentation pointers to this? Even just pointing to a
>> specific line of code in coreboot would be helpful in case anyone wants
>> to read it up.
>>
>>>
>>> Signed-off-by: Andre Heider <a.heider@gmail.com>
>>> ---
>>>  fs/cbfs/cbfs.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
>>> index 6e1107d751..46da8f134f 100644
>>> --- a/fs/cbfs/cbfs.c
>>> +++ b/fs/cbfs/cbfs.c
>>> @@ -168,9 +168,9 @@ static int file_cbfs_load_header(uintptr_t end_of_rom,
>>>                                struct cbfs_header *header)
>>>  {
>>>       struct cbfs_header *header_in_rom;
>>> +     int32_t offset = *(u32 *)(end_of_rom - 3);
>>
>> Accessing a pointer that gets subtracted by 3 looks terribly wrong.
>> Unfortunately it's correct, because the pointer itself is unaligned.
>>
>> How about we change the logic so that we calculate an aligned pointer
>> (after_rom?) which we sanity check for alignment and base all
>> calculations on that instead?
>>
>> That way the next time someone comes around he's not getting puzzled why
>> we're subtracting 3 and adding 1 to the pointer ;).
>
> Either that or a comment would be nice. But since this has been
> sitting around for a while and fixes a bug I think it is best to take
> it. Please feel free to send a follow-up patch..
>
> We also have no tests for this code, so I'd really like to get some
> tests in there!
>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-dm, thanks!

[U-Boot] [PATCH 3/3] cbfs: add support for cbfs header components

[Andre Heider]

This fixes walking the cbfs file list because the bound checks do not
apply to header components.

Output of coreboot's cbfstool:
    Name                           Offset     Type           Size   Comp
    cbfs master header             0x0        cbfs header        32 none
    fallback/romstage              0x80       stage           21344 none
    fallback/ramstage              0x5440     stage           36848 none
    config                         0xe480     raw               310 none
    revision                       0xe600     raw               575 none
    fallback/bl31                  0xe880     payload         15931 none
    fallback/payload               0x12700    payload        205449 none
    (empty)                        0x44a00    null           111768 none
    header pointer                 0x5fec0    cbfs header         4 none

Output of u-boot's cbfsls:
         size              type  name
    ------------------------------------------
           32       cbfs header  cbfs master header
        21344             stage  fallback/romstage
        36848             stage  fallback/ramstage
          310               raw  config
          575               raw  revision
        15931           payload  fallback/bl31
       205449           payload  fallback/payload
       111768              null  (empty)
            4       cbfs header  header pointer

Signed-off-by: Andre Heider <a.heider@gmail.com>
---
 cmd/cbfs.c     |  3 +++
 fs/cbfs/cbfs.c | 10 ++++++----
 include/cbfs.h |  1 +
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/cmd/cbfs.c b/cmd/cbfs.c
index 736f8c4527..f5ad04c45a 100644
--- a/cmd/cbfs.c
+++ b/cmd/cbfs.c
@@ -113,6 +113,9 @@ static int do_cbfs_ls(cmd_tbl_t *cmdtp, int flag, int argc,
 		printf(" %8d", file_cbfs_size(file));
 
 		switch (type) {
+		case CBFS_COMPONENT_CBFSHEADER:
+			type_name = "cbfs header";
+			break;
 		case CBFS_TYPE_STAGE:
 			type_name = "stage";
 			break;
diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
index 46da8f134f..389f60702b 100644
--- a/fs/cbfs/cbfs.c
+++ b/fs/cbfs/cbfs.c
@@ -97,10 +97,12 @@ static int file_cbfs_next_file(u8 *start, u32 size, u32 align,
 		}
 
 		swap_file_header(&header, fileHeader);
-		if (header.offset < sizeof(struct cbfs_fileheader) ||
-		    header.offset > header.len) {
-			file_cbfs_result = CBFS_BAD_FILE;
-			return -1;
+		if (header.type != CBFS_COMPONENT_CBFSHEADER) {
+			if (header.offset < sizeof(struct cbfs_fileheader) ||
+			    header.offset > header.len) {
+				file_cbfs_result = CBFS_BAD_FILE;
+				return -1;
+			}
 		}
 		newNode->next = NULL;
 		newNode->type = header.type;
diff --git a/include/cbfs.h b/include/cbfs.h
index f50280107b..d5d9d8ce97 100644
--- a/include/cbfs.h
+++ b/include/cbfs.h
@@ -19,6 +19,7 @@ enum cbfs_result {
 };
 
 enum cbfs_filetype {
+	CBFS_COMPONENT_CBFSHEADER = 0x02,
 	CBFS_TYPE_STAGE = 0x10,
 	CBFS_TYPE_PAYLOAD = 0x20,
 	CBFS_TYPE_OPTIONROM = 0x30,
-- 
2.16.1

[U-Boot] [PATCH 3/3] cbfs: add support for cbfs header components

[Alexander Graf]

On 15.02.18 07:40, Andre Heider wrote:
> This fixes walking the cbfs file list because the bound checks do not
> apply to header components.
> 
> Output of coreboot's cbfstool:
>     Name                           Offset     Type           Size   Comp
>     cbfs master header             0x0        cbfs header        32 none
>     fallback/romstage              0x80       stage           21344 none
>     fallback/ramstage              0x5440     stage           36848 none
>     config                         0xe480     raw               310 none
>     revision                       0xe600     raw               575 none
>     fallback/bl31                  0xe880     payload         15931 none
>     fallback/payload               0x12700    payload        205449 none
>     (empty)                        0x44a00    null           111768 none
>     header pointer                 0x5fec0    cbfs header         4 none
> 
> Output of u-boot's cbfsls:
>          size              type  name
>     ------------------------------------------
>            32       cbfs header  cbfs master header
>         21344             stage  fallback/romstage
>         36848             stage  fallback/ramstage
>           310               raw  config
>           575               raw  revision
>         15931           payload  fallback/bl31
>        205449           payload  fallback/payload
>        111768              null  (empty)
>             4       cbfs header  header pointer

I don't see a before/after comparison? What output exactly did get fixed?

I don't quite understand what case exactly this fixes. The bounds check
seems to try to find out whether the header is within the master header
range, right?

So doesn't this just mean we're reading beyond the expected fs size?


Alex

> 
> Signed-off-by: Andre Heider <a.heider@gmail.com>
> ---
>  cmd/cbfs.c     |  3 +++
>  fs/cbfs/cbfs.c | 10 ++++++----
>  include/cbfs.h |  1 +
>  3 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/cmd/cbfs.c b/cmd/cbfs.c
> index 736f8c4527..f5ad04c45a 100644
> --- a/cmd/cbfs.c
> +++ b/cmd/cbfs.c
> @@ -113,6 +113,9 @@ static int do_cbfs_ls(cmd_tbl_t *cmdtp, int flag, int argc,
>  		printf(" %8d", file_cbfs_size(file));
>  
>  		switch (type) {
> +		case CBFS_COMPONENT_CBFSHEADER:
> +			type_name = "cbfs header";
> +			break;
>  		case CBFS_TYPE_STAGE:
>  			type_name = "stage";
>  			break;
> diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
> index 46da8f134f..389f60702b 100644
> --- a/fs/cbfs/cbfs.c
> +++ b/fs/cbfs/cbfs.c
> @@ -97,10 +97,12 @@ static int file_cbfs_next_file(u8 *start, u32 size, u32 align,
>  		}
>  
>  		swap_file_header(&header, fileHeader);
> -		if (header.offset < sizeof(struct cbfs_fileheader) ||
> -		    header.offset > header.len) {
> -			file_cbfs_result = CBFS_BAD_FILE;
> -			return -1;
> +		if (header.type != CBFS_COMPONENT_CBFSHEADER) {
> +			if (header.offset < sizeof(struct cbfs_fileheader) ||
> +			    header.offset > header.len) {
> +				file_cbfs_result = CBFS_BAD_FILE;
> +				return -1;
> +			}
>  		}
>  		newNode->next = NULL;
>  		newNode->type = header.type;
> diff --git a/include/cbfs.h b/include/cbfs.h
> index f50280107b..d5d9d8ce97 100644
> --- a/include/cbfs.h
> +++ b/include/cbfs.h
> @@ -19,6 +19,7 @@ enum cbfs_result {
>  };
>  
>  enum cbfs_filetype {
> +	CBFS_COMPONENT_CBFSHEADER = 0x02,
>  	CBFS_TYPE_STAGE = 0x10,
>  	CBFS_TYPE_PAYLOAD = 0x20,
>  	CBFS_TYPE_OPTIONROM = 0x30,
> 

[U-Boot] [PATCH 3/3] cbfs: add support for cbfs header components

[Simon Glass]

Hi,

On 22 February 2018 at 08:15, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 15.02.18 07:40, Andre Heider wrote:
>> This fixes walking the cbfs file list because the bound checks do not
>> apply to header components.
>>
>> Output of coreboot's cbfstool:
>>     Name                           Offset     Type           Size   Comp
>>     cbfs master header             0x0        cbfs header        32 none
>>     fallback/romstage              0x80       stage           21344 none
>>     fallback/ramstage              0x5440     stage           36848 none
>>     config                         0xe480     raw               310 none
>>     revision                       0xe600     raw               575 none
>>     fallback/bl31                  0xe880     payload         15931 none
>>     fallback/payload               0x12700    payload        205449 none
>>     (empty)                        0x44a00    null           111768 none
>>     header pointer                 0x5fec0    cbfs header         4 none
>>
>> Output of u-boot's cbfsls:
>>          size              type  name
>>     ------------------------------------------
>>            32       cbfs header  cbfs master header
>>         21344             stage  fallback/romstage
>>         36848             stage  fallback/ramstage
>>           310               raw  config
>>           575               raw  revision
>>         15931           payload  fallback/bl31
>>        205449           payload  fallback/payload
>>        111768              null  (empty)
>>             4       cbfs header  header pointer
>
> I don't see a before/after comparison? What output exactly did get fixed?
>
> I don't quite understand what case exactly this fixes. The bounds check
> seems to try to find out whether the header is within the master header
> range, right?
>
> So doesn't this just mean we're reading beyond the expected fs size?

I don't understand this one either.

Andre please take a look and update your commit message.

Regards,
Simon

[U-Boot] [PATCH 1/3] cmd: cbfs: fix reading the end_of_rom pointer for 64bit archs

[Alexander Graf]

On 15.02.18 07:40, Andre Heider wrote:
> The cast breaks the pointer on 64bit archs, so lets get rid of it.
> 
> Signed-off-by: Andre Heider <a.heider@gmail.com>

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

> ---
>  cmd/cbfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cmd/cbfs.c b/cmd/cbfs.c
> index 799ba01fcc..736f8c4527 100644
> --- a/cmd/cbfs.c
> +++ b/cmd/cbfs.c
> @@ -22,7 +22,7 @@ static int do_cbfs_init(cmd_tbl_t *cmdtp, int flag, int argc,
>  		return 0;
>  	}
>  	if (argc == 2) {
> -		end_of_rom = (int)simple_strtoul(argv[1], &ep, 16);
> +		end_of_rom = simple_strtoul(argv[1], &ep, 16);
>  		if (*ep) {
>  			puts("\n** Invalid end of ROM **\n");
>  			return 1;
> 

[U-Boot] [PATCH 1/3] cmd: cbfs: fix reading the end_of_rom pointer for 64bit archs

[Simon Glass]

On 22 February 2018 at 07:59, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 15.02.18 07:40, Andre Heider wrote:
>> The cast breaks the pointer on 64bit archs, so lets get rid of it.
>>
>> Signed-off-by: Andre Heider <a.heider@gmail.com>
>
> Reviewed-by: Alexander Graf <agraf@suse.de>

Applied to u-boot-dm, thanks!