| Message ID | 20180215064012.28557-2-a.heider@gmail.com |
|---|---|
| State | Accepted |
| Commit | 44683170f818ecaf914ea4c77d35021f60e38b04 |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [U-Boot,1/3] cmd: cbfs: fix reading the end_of_rom pointer for 64bit archs | expand |
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 > >
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
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!
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 >
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(-)