* [U-Boot] [PATCH 1/3] cmd: cbfs: fix reading the end_of_rom pointer for 64bit archs
@ 2018-02-15 6:40 Andre Heider
2018-02-15 6:40 ` [U-Boot] [PATCH 2/3] fs: cbfs: fix locating the cbfs header Andre Heider
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Andre Heider @ 2018-02-15 6:40 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 2/3] fs: cbfs: fix locating the cbfs header
2018-02-15 6:40 [U-Boot] [PATCH 1/3] cmd: cbfs: fix reading the end_of_rom pointer for 64bit archs Andre Heider
@ 2018-02-15 6:40 ` Andre Heider
2018-02-22 0:08 ` Alexander Graf
2018-02-15 6:40 ` [U-Boot] [PATCH 3/3] cbfs: add support for cbfs header components Andre Heider
2018-02-21 23:59 ` [U-Boot] [PATCH 1/3] cmd: cbfs: fix reading the end_of_rom pointer for 64bit archs Alexander Graf
2 siblings, 1 reply; 10+ messages in thread
From: Andre Heider @ 2018-02-15 6:40 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 3/3] cbfs: add support for cbfs header components
2018-02-15 6:40 [U-Boot] [PATCH 1/3] cmd: cbfs: fix reading the end_of_rom pointer for 64bit archs Andre Heider
2018-02-15 6:40 ` [U-Boot] [PATCH 2/3] fs: cbfs: fix locating the cbfs header Andre Heider
@ 2018-02-15 6:40 ` Andre Heider
2018-02-22 0:15 ` Alexander Graf
2018-02-21 23:59 ` [U-Boot] [PATCH 1/3] cmd: cbfs: fix reading the end_of_rom pointer for 64bit archs Alexander Graf
2 siblings, 1 reply; 10+ messages in thread
From: Andre Heider @ 2018-02-15 6:40 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 1/3] cmd: cbfs: fix reading the end_of_rom pointer for 64bit archs
2018-02-15 6:40 [U-Boot] [PATCH 1/3] cmd: cbfs: fix reading the end_of_rom pointer for 64bit archs Andre Heider
2018-02-15 6:40 ` [U-Boot] [PATCH 2/3] fs: cbfs: fix locating the cbfs header Andre Heider
2018-02-15 6:40 ` [U-Boot] [PATCH 3/3] cbfs: add support for cbfs header components Andre Heider
@ 2018-02-21 23:59 ` Alexander Graf
2018-04-01 14:28 ` Simon Glass
2 siblings, 1 reply; 10+ messages in thread
From: Alexander Graf @ 2018-02-21 23:59 UTC (permalink / raw)
To: u-boot
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;
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 2/3] fs: cbfs: fix locating the cbfs header
2018-02-15 6:40 ` [U-Boot] [PATCH 2/3] fs: cbfs: fix locating the cbfs header Andre Heider
@ 2018-02-22 0:08 ` Alexander Graf
2018-04-01 14:19 ` Simon Glass
0 siblings, 1 reply; 10+ messages in thread
From: Alexander Graf @ 2018-02-22 0:08 UTC (permalink / raw)
To: u-boot
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 >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 3/3] cbfs: add support for cbfs header components
2018-02-15 6:40 ` [U-Boot] [PATCH 3/3] cbfs: add support for cbfs header components Andre Heider
@ 2018-02-22 0:15 ` Alexander Graf
2018-04-01 14:19 ` Simon Glass
0 siblings, 1 reply; 10+ messages in thread
From: Alexander Graf @ 2018-02-22 0:15 UTC (permalink / raw)
To: u-boot
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,
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 2/3] fs: cbfs: fix locating the cbfs header
2018-02-22 0:08 ` Alexander Graf
@ 2018-04-01 14:19 ` Simon Glass
2018-04-01 14:28 ` Simon Glass
0 siblings, 1 reply; 10+ messages in thread
From: Simon Glass @ 2018-04-01 14:19 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 3/3] cbfs: add support for cbfs header components
2018-02-22 0:15 ` Alexander Graf
@ 2018-04-01 14:19 ` Simon Glass
0 siblings, 0 replies; 10+ messages in thread
From: Simon Glass @ 2018-04-01 14:19 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 1/3] cmd: cbfs: fix reading the end_of_rom pointer for 64bit archs
2018-02-21 23:59 ` [U-Boot] [PATCH 1/3] cmd: cbfs: fix reading the end_of_rom pointer for 64bit archs Alexander Graf
@ 2018-04-01 14:28 ` Simon Glass
0 siblings, 0 replies; 10+ messages in thread
From: Simon Glass @ 2018-04-01 14:28 UTC (permalink / raw)
To: u-boot
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!
^ permalink raw reply [flat|nested] 10+ messages in thread
* [U-Boot] [PATCH 2/3] fs: cbfs: fix locating the cbfs header
2018-04-01 14:19 ` Simon Glass
@ 2018-04-01 14:28 ` Simon Glass
0 siblings, 0 replies; 10+ messages in thread
From: Simon Glass @ 2018-04-01 14:28 UTC (permalink / raw)
To: u-boot
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!
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-04-01 14:28 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-15 6:40 [U-Boot] [PATCH 1/3] cmd: cbfs: fix reading the end_of_rom pointer for 64bit archs Andre Heider
2018-02-15 6:40 ` [U-Boot] [PATCH 2/3] fs: cbfs: fix locating the cbfs header Andre Heider
2018-02-22 0:08 ` Alexander Graf
2018-04-01 14:19 ` Simon Glass
2018-04-01 14:28 ` Simon Glass
2018-02-15 6:40 ` [U-Boot] [PATCH 3/3] cbfs: add support for cbfs header components Andre Heider
2018-02-22 0:15 ` Alexander Graf
2018-04-01 14:19 ` Simon Glass
2018-02-21 23:59 ` [U-Boot] [PATCH 1/3] cmd: cbfs: fix reading the end_of_rom pointer for 64bit archs Alexander Graf
2018-04-01 14:28 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.