cmd: cbfs: fix reading the end_of_rom pointer for 64bit archs

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 <[email protected]>

---
 fs/cbfs/cbfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/cbfs/cbfs.c +0 -0

index 6e1107d751..46da8f134f 100644

fs/cbfs/cbfs.c +1 -2

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

Quoted 2 lines · Andre Heider

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.

Quoted 11 lines

Signed-off-by Andre Heider <[email protected]>

---
 fs/cbfs/cbfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/cbfs/cbfs.c +0 -0

index 6e1107d751..46da8f134f 100644

fs/cbfs/cbfs.c static int file_cbfs_load_header(uintptr_t end_of_rom, · line 171

Quoted 4 lines

fs/cbfs/cbfs.c +1 -0

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

Hi,

Quoted 29 lines · Alexander Graf

Quoted 2 lines · Andre Heider

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.

Quoted 11 lines

Signed-off-by Andre Heider <[email protected]>

---
 fs/cbfs/cbfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/cbfs/cbfs.c +0 -0

index 6e1107d751..46da8f134f 100644

fs/cbfs/cbfs.c static int file_cbfs_load_header(uintptr_t end_of_rom, · line 171

Quoted 4 lines

fs/cbfs/cbfs.c +1 -0

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

Quoted 6 lines

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 <[email protected]>

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 <[email protected]>

---
 cmd/cbfs.c     |  3 +++
 fs/cbfs/cbfs.c | 10 ++++++----
 include/cbfs.h |  1 +
 3 files changed, 10 insertions(+), 4 deletions(-)

cmd/cbfs.c +3 -0

index 736f8c4527..f5ad04c45a 100644
@@ -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;

fs/cbfs/cbfs.c +6 -4

index 46da8f134f..389f60702b 100644
@@ -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;

include/cbfs.h +1 -0

index f50280107b..d5d9d8ce97 100644
@@ -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,

Quoted 15 lines · 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

Quoted 12 lines

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?

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

Signed-off-by Andre Heider <[email protected]>

---
 cmd/cbfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

cmd/cbfs.c +1 -1

index 799ba01fcc..736f8c4527 100644
@@ -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;

Quoted 3 lines · Andre Heider

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

Signed-off-by Andre Heider <[email protected]>

Reviewed-by Alexander Graf <[email protected]>