[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 at 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 at 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 at chromium.org>

Regards,
Simon