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 >

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!

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,