[U-Boot-Users] [PATCH] Off-by-two bug when relocating GOT
The first two entries are skipped but the number of relocated entries is not adjusted; as a result, the first __u_boot_cmd_* structure is smashed and no commands can be issued.
Signed-off-by: Vlad Lungu vlad@comsys.ro --- cpu/mips/start.S | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/cpu/mips/start.S b/cpu/mips/start.S index e91e213..1e5b302 100644 --- a/cpu/mips/start.S +++ b/cpu/mips/start.S @@ -349,6 +349,7 @@ in_ram: /* Now we want to update GOT. */ lw t3, -4(t0) /* t3 <-- num_got_entries */ + sub t3,2 /* If skipping two entries, adjust length*/ addi t4, gp, 8 /* Skipping first two entries. */ li t2, 2 1:
Vlad Lungu wrote:
The first two entries are skipped but the number of relocated entries is not adjusted; as a result, the first __u_boot_cmd_* structure is smashed and no commands can be issued.
This is a known, long stading, pretty critical, but not fixed problem. See below:
http://search.gmane.org/search.php?group=gmane.comp.boot-loaders.u-boot&...
I'm going to look closely into this.
Thanks,
Shinya Kuribayashi
Shinya Kuribayashi wrote:
Vlad Lungu wrote:
The first two entries are skipped but the number of relocated entries is not adjusted; as a result, the first __u_boot_cmd_* structure is smashed and no commands can be issued.
This is a known, long stading, pretty critical, but not fixed problem. See below:
http://search.gmane.org/search.php?group=gmane.comp.boot-loaders.u-boot&...
I'm going to look closely into this.
The thing I don't get is why skip the top two entries in the first place? Is it because _gp=ALIGN(16) ? Maybe Robert has a point:
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/25533
Vlad
Vlad Lungu wrote:
Shinya Kuribayashi wrote:
Vlad Lungu wrote:
The first two entries are skipped but the number of relocated entries is not adjusted; as a result, the first __u_boot_cmd_* structure is smashed and no commands can be issued.
This is a known, long stading, pretty critical, but not fixed problem. See below:
http://search.gmane.org/search.php?group=gmane.comp.boot-loaders.u-boot&...
I'm going to look closely into this.
The thing I don't get is why skip the top two entries in the first place? Is it because _gp=ALIGN(16) ? Maybe Robert has a point:
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/25533
Yes, Roberts patch sent 12/15/06 09:53 worked for me when my command table suddenly got corrupted.
It is still not applied to sources.
Is it rejected/pending/forgotten?
/Thomas
Thomas Lange wrote:
Vlad Lungu wrote:
Shinya Kuribayashi wrote:
Vlad Lungu wrote:
The first two entries are skipped but the number of relocated entries is not adjusted; as a result, the first __u_boot_cmd_* structure is smashed and no commands can be issued.
This is a known, long stading, pretty critical, but not fixed problem. See below:
http://search.gmane.org/search.php?group=gmane.comp.boot-loaders.u-boot&...
I'm going to look closely into this.
The thing I don't get is why skip the top two entries in the first place? Is it because _gp=ALIGN(16) ? Maybe Robert has a point:
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/25533
Yes, Roberts patch sent 12/15/06 09:53 worked for me when my command table suddenly got corrupted.
That's my case: 0x00000000bfc1d950 _gp = ALIGN (0x10) 0x00000000bfc1d94c __got_start = .
.got 0x00000000bfc1d950 0x500 *(.got) .got 0x00000000bfc1d950 0x500 cpu/mips/start.o 0x00000000bfc1d950 _GLOBAL_OFFSET_TABLE_ 0x00000000bfc1de50 __got_end = . 0x00000000bfc1de50 . = . 0x00000000bfc1de50 __u_boot_cmd_start = .
The thing is, num_got_entries=(__got_end - __got_start)>>2 and that's 0x141 and it should be only 0x140. That is what triggers the bug. In start.S, lines 353-354, $t4 is loaded with $gp+8 and $t2 with 2 and not with 0, so in effect if I substract 2 from $t3 I'm not relocating the last entry, and with Robert's patch I'm not relocating the last two. One more point: loading $gp with _GLOBAL_OFFSET_TABLE_ is not a good idea, it should be loaded with _gp. The value is the same at the moment, but it's not guaranteed at all, someone could start playing with the link scripts and break this.
It is still not applied to sources.
Is it rejected/pending/forgotten?
Well, it was not a "proper" patch so it kind of fell trought the cracks, probably. This one is a "proper" patch but it's actually wrong, so please don't apply it.
Vlad
Vlad Lungu wrote:
Thomas Lange wrote:
Vlad Lungu wrote:
Shinya Kuribayashi wrote:
Vlad Lungu wrote:
The first two entries are skipped but the number of relocated entries is not adjusted; as a result, the first __u_boot_cmd_* structure is smashed and no commands can be issued.
This is a known, long stading, pretty critical, but not fixed problem. See below:
http://search.gmane.org/search.php?group=gmane.comp.boot-loaders.u-boot&...
I'm going to look closely into this.
The thing I don't get is why skip the top two entries in the first place? Is it because _gp=ALIGN(16) ? Maybe Robert has a point:
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/25533
Yes, Roberts patch sent 12/15/06 09:53 worked for me when my command table suddenly got corrupted.
That's my case: 0x00000000bfc1d950 _gp = ALIGN (0x10) 0x00000000bfc1d94c __got_start = .
.got 0x00000000bfc1d950 0x500 *(.got) .got 0x00000000bfc1d950 0x500 cpu/mips/start.o 0x00000000bfc1d950 _GLOBAL_OFFSET_TABLE_ 0x00000000bfc1de50 __got_end = . 0x00000000bfc1de50 . = . 0x00000000bfc1de50 __u_boot_cmd_start = .
The thing is, num_got_entries=(__got_end - __got_start)>>2 and that's 0x141 and it should be only 0x140. That is what triggers the bug. In start.S, lines 353-354, $t4 is loaded with $gp+8 and $t2 with 2 and not with 0, so in effect if I substract 2 from $t3 I'm not relocating the last entry, and with Robert's patch I'm not relocating the last two. One more point: loading $gp with _GLOBAL_OFFSET_TABLE_ is not a good idea, it should be loaded with _gp. The value is the same at the moment, but it's not guaranteed at all, someone could start playing with the link scripts and break this.
It is still not applied to sources.
Is it rejected/pending/forgotten?
Well, it was not a "proper" patch so it kind of fell trought the cracks, probably. This one is a "proper" patch but it's actually wrong, so please don't apply it.
Hmm, are we talking about the same patch?
[PATCH] Fix: Bug in MIPS linker scripts http://article.gmane.org/gmane.comp.boot-loaders.u-boot/25541/
Looks proper to me.
/Thomas
Thomas Lange wrote:
Vlad Lungu wrote:
Thomas Lange wrote:
Vlad Lungu wrote:
Shinya Kuribayashi wrote:
Vlad Lungu wrote:
The first two entries are skipped but the number of relocated entries is not adjusted; as a result, the first __u_boot_cmd_* structure is smashed and no commands can be issued.
This is a known, long stading, pretty critical, but not fixed problem. See below:
http://search.gmane.org/search.php?group=gmane.comp.boot-loaders.u-boot&...
I'm going to look closely into this.
The thing I don't get is why skip the top two entries in the first place? Is it because _gp=ALIGN(16) ? Maybe Robert has a point:
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/25533
Yes, Roberts patch sent 12/15/06 09:53 worked for me when my command table suddenly got corrupted.
That's my case: 0x00000000bfc1d950 _gp = ALIGN (0x10) 0x00000000bfc1d94c __got_start = .
.got 0x00000000bfc1d950 0x500 *(.got) .got 0x00000000bfc1d950 0x500 cpu/mips/start.o 0x00000000bfc1d950 _GLOBAL_OFFSET_TABLE_ 0x00000000bfc1de50 __got_end = . 0x00000000bfc1de50 . = . 0x00000000bfc1de50 __u_boot_cmd_start = .
The thing is, num_got_entries=(__got_end - __got_start)>>2 and that's 0x141 and it should be only 0x140. That is what triggers the bug. In start.S, lines 353-354, $t4 is loaded with $gp+8 and $t2 with 2 and not with 0, so in effect if I substract 2 from $t3 I'm not relocating the last entry, and with Robert's patch I'm not relocating the last two. One more point: loading $gp with _GLOBAL_OFFSET_TABLE_ is not a good idea, it should be loaded with _gp. The value is the same at the moment, but it's not guaranteed at all, someone could start playing with the link scripts and break this.
It is still not applied to sources.
Is it rejected/pending/forgotten?
Well, it was not a "proper" patch so it kind of fell trought the cracks, probably. This one is a "proper" patch but it's actually wrong, so please don't apply it.
Hmm, are we talking about the same patch?
[PATCH] Fix: Bug in MIPS linker scripts http://article.gmane.org/gmane.comp.boot-loaders.u-boot/25541/
Didn't see this one. I was talking about
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/25533
Well, who knows? There are no commits in the MIPS tree since March, and the patch is not applied there, even if the dude is listed as custodian of the tree.
Vlad
Vlad Lungu wrote:
Thomas Lange wrote:
Vlad Lungu wrote:
Shinya Kuribayashi wrote:
[snip]
I'm going to look closely into this.
Here's my proposal for RFC. This patch fixes 1) __got_start and _GLOBAL_OFFSET_TABLE_ miss-alignment, and 2) duplicated .sdata declaration.
diff --git a/board/purple/u-boot.lds b/board/purple/u-boot.lds index 1bdac1f..51be57f 100644 --- a/board/purple/u-boot.lds +++ b/board/purple/u-boot.lds @@ -53,15 +53,15 @@ SECTIONS . = ALIGN(4); .data : { *(.data) }
- . = ALIGN(4); - .sdata : { *(.sdata) } - - _gp = ALIGN(16); - - __got_start = .; - .got : { *(.got) } - __got_end = .; + . = ALIGN(16); + .got : { + _gp = .; + __got_start = .; + *(.got) + __got_end = .; + }
+ . = ALIGN(4); .sdata : { *(.sdata) }
. = .; _
.data.rel.ro.local 0x00000000b0023280 0x4 .data.rel.ro.local 0x00000000b0023280 0x4 lib_mips/libmips.a(board.o) 0x00000000b0023290 . = ALIGN (0x10)
.got 0x00000000b0023290 0x4f4 0x00000000b0023290 _gp = . 0x00000000b0023290 __got_start = . *(.got) .got 0x00000000b0023290 0x4f4 cpu/mips/start.o 0x00000000b0023290 _GLOBAL_OFFSET_TABLE_ 0x00000000b0023784 __got_end = . 0x00000000b0023784 . = ALIGN (0x4)
.sdata *(.sdata) 0x00000000b0023784 . = . 0x00000000b0023784 __u_boot_cmd_start = . _
I think this style is easier to understand than before. But I'm still wondering where _gp can be used?
Any comments are welcome.
The thing I don't get is why skip the top two entries in the first place? Is it because _gp=ALIGN(16) ? Maybe Robert has a point:
[snip]
That is what triggers the bug. In start.S, lines 353-354, $t4 is loaded with $gp+8 and $t2 with 2 and not with 0, so in effect if I substract 2 from $t3 I'm not relocating the last entry, and with Robert's patch I'm not relocating the last two.
skuribay@debian:~/devel/u-boot.git$ mips-linux-readelf -S u-boot |grep got [ 9] .got PROGBITS b0023290 033290 0004f4 04 WAp 0 0 16
skuribay@debian:~/devel/u-boot.git$ mips-linux-readelf -x 9 u-boot | head -n 8
Hex dump of section '.got': 0xb0023290 00000000 80000000 b0000000 b0020000 ................ 0xb00232a0 b0010000 00000000 00000000 00000000 ................ 0xb00232b0 00000000 b0028394 b001d430 b00016dc ...........0.... 0xb00232c0 b001b4b0 b001c630 b000c290 00000000 .......0........ 0xb00232d0 00000000 b000f180 b0000754 b00283bc ...........T.... 0xb00232e0 b0024dcc b0010468 b0003230 b0019140 ..M....h..20...@
got[0](=0x00000000) and got[1](=0x80000000) are always reserved by GNU ld. When updating the contents of GOT entries at in_ram:, leave first two entries as they are. This is the reason for skipping two entries. And as you know, this is nothing related with corrupting command table. That's caused by relocation itself, not by updating GOT entries.
One more point: loading $gp with _GLOBAL_OFFSET_TABLE_ is not a good idea, it should be loaded with _gp. The value is the same at the moment, but it's not guaranteed at all, someone could start playing with the link scripts and break this.
Hmm, I have to consider more.
It is still not applied to sources.
Is it rejected/pending/forgotten?
Well, it was not a "proper" patch so it kind of fell trought the cracks, probably. This one is a "proper" patch but it's actually wrong, so please don't apply it.
thanks,
Shinya
Shinya Kuribayashi wrote: [snip]
Here's my proposal for RFC. This patch fixes
__got_start and _GLOBAL_OFFSET_TABLE_ miss-alignment, and
duplicated .sdata declaration.
[snip]
.got : {_gp = .;__got_start = .;*(.got)__got_end = .;}
[snip] That doesn't look right. Don't put _gp inside .got section.
I think this style is easier to understand than before.
But I'm still wondering where _gp can be used?
Any comments are welcome.
It should be loaded into the $gp register.
got[0](=0x00000000) and got[1](=0x80000000) are always reserved by
GNU ld. When updating the contents of GOT entries at in_ram:, leave
first two entries as they are. This is the reason for skipping two
entries. And as you know, this is nothing related with corrupting
command table. That's caused by relocation itself, not by updating
GOT entries.
.got it :-)
One more point: loading $gp with _GLOBAL_OFFSET_TABLE_ is not a good idea, it should be loaded with _gp. The value
is the same at the moment, but it's not guaranteed at all, someone could start playing with the link scripts and break this.
Hmm, I have to consider more.
Here's a good example:
http://www.sourceware.org/ml/ecos-discuss/2004-02/msg00327.html
There are some sections (.sdata/.sbss/.scommon) that contain objects referenced via $gp and not via GOT.
Try nm -n -f sysv u-boot|grep scommon.
I can also send you a patch with _gp != __got_start , and if you don't load $gp with _gp but with __got_start, it will crash and burn when doing
/* Initialize any external memory. */ la t9, lowlevel_init jalr t9
It will actually jump to _serial_puts(), believe it or not.
Vlad
Vlad Lungu wrote:
I can also send you a patch with _gp != __got_start , and if you don't
Actually, __got_start should be read as _GLOBAL_OFFSET_TABLE_ here.
load $gp with _gp but with __got_start, it will crash and burn when doing
/* Initialize any external memory. */ la t9, lowlevel_init jalr t9
Vlad Lungu wrote:
.got : {_gp = .;__got_start = .;*(.got)__got_end = .;}[snip] That doesn't look right. Don't put _gp inside .got section.
Agreed, will fix.
One more point: loading $gp with _GLOBAL_OFFSET_TABLE_ is not a good idea, it should be loaded with _gp. The value is the same at the moment, but it's not guaranteed at all, someone could start playing with the link scripts and break this.
Hmm, I have to consider more.
Thanks for your comment. Now investigating further, so I need some time.
Here's a good example:
http://www.sourceware.org/ml/ecos-discuss/2004-02/msg00327.html
There are some sections (.sdata/.sbss/.scommon) that contain objects referenced via $gp and not via GOT.
Try nm -n -f sysv u-boot|grep scommon.
This is caused by lacking of -G0 in PLATFORM_LDFLAGS. I'm preparing a patch as below, and will submit for review in the near future.
diff --git a/mips_config.mk b/mips_config.mk index d8aa5fa..93324ad 100644 --- a/mips_config.mk +++ b/mips_config.mk @@ -22,3 +22,36 @@ #
PLATFORM_CPPFLAGS += -DCONFIG_MIPS -D__MIPS__ + +# +# GCC uses -G 0 -mabicalls -fpic as default. We don't want PIC in the kernel +# code since it only slows down the whole thing. At some point we might make +# use of global pointer optimizations but their use of $28 conflicts with +# the current pointer optimization. +# +# The DECStation requires an ECOFF kernel for remote booting, other MIPS +# machines may also. Since BFD is incredibly buggy with respect to +# crossformat linking we rely on the elf2ecoff tool for format conversion. +# +#cflags-y += -G 0 -mno-abicalls -fno-pic -pipe +#cflags-y += -msoft-float +#LDFLAGS_vmlinux += -G 0 -static -n -nostdlib +#MODFLAGS += -mlong-calls +# +# But we U-Boot rely on PIC and need abicalls for now. +# +PLATFORM_CPPFLAGS += -G 0 -mabicalls -fpic -pipe +PLATFORM_CPPFLAGS += -msoft-float +PLATFORM_LDFLAGS += -G 0 -static -n -nostdlib + +PLATFORM_CPPFLAGS += -ffreestanding +
According to mail archives of binutils, linux-mips, etc., GCC for MIPS uses -G 0 as default, but ld does not. If calling ld directly, you need to pass -G 0 explicitly like Linux. As a result, we'll get no small data.
That's all for now.
thanks,
Shinya
Shinya Kuribayashi wrote:
This is caused by lacking of -G0 in PLATFORM_LDFLAGS. I'm preparing a patch as below, and will submit for review in the near future.
There is nothing wrong with having sdata/scommon/sbbs. I was just pointing out that _gp and _G_O_T_ are not conceptually the same and if they are not equal, then the whole thing blows up with the current cpu/mips/start.S. See http://www.nabble.com/-PATCH--Fixed-the-_gp-_G_O_T_-confusion-for-good-tf461...
As long as there is nothing between the .got section and __got_end, it works, regardless of the alignment of _gp or __got_start(or even his presence) and num_got_entries is redundant at this point.
Vlad
Dear Shinya Kuribayashi,
on IRC you asked:
In Current u-boot.lds for MIPS ports, _gp is basically equals to __got_start. Is this intentional, or not? If this is intentional for simplification, I'll take into account such original intention. If not, I'll take Vlad suggests because what he changes is basically right as PIC code.
I'm afraid I don't remember this any more. The code is as old as the very first MIPS port for the Infineon INCA-IP board, back in March 2003. It is easily possible that we didn't know any better by then, and everybody just copied the code without thinking since.
Speaking for myself, I'm not much of a MIPS expert and will rely on your judgment. All I can do is helping to test changes on the INCA-IP board.
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Dear Shinya Kuribayashi,
on IRC you asked:
In Current u-boot.lds for MIPS ports, _gp is basically equals to __got_start.
Is equal to _GLOBAL_OFFSET_TABLE_ in current tree. Only if you change it to .=ALIGN(16); gp=.; it's always equal to __got_start.
Is this intentional, or not? If this is intentional for simplification, I'll take into account such original intention.
It might have been done to simplify the code or shave a few insns, but it had the potential to bite us, and it did exactly that.
Cheers, Vlad
On 10/16/07, Wolfgang Denk wd@denx.de wrote:
Dear Shinya Kuribayashi,
on IRC you asked:
In Current u-boot.lds for MIPS ports, _gp is basically equals to __got_start. Is this intentional, or not? If this is intentional for simplification, I'll take into account such original intention. If not, I'll take Vlad suggests because what he changes is basically right as PIC code.
I'm afraid I don't remember this any more. The code is as old as the very first MIPS port for the Infineon INCA-IP board, back in March 2003. It is easily possible that we didn't know any better by then, and everybody just copied the code without thinking since.
Speaking for myself, I'm not much of a MIPS expert and will rely on your judgment. All I can do is helping to test changes on the INCA-IP board.
Typically _gp is set in the linker script to 0x7ff0 above the start of the 'small' sections (all ELF sections with SHF_MIPS_GPREL flag), so that all small items can be accessed by a 16 bit signed offset off of the register allocated for _gp. The 16-bit signed offset corresponds to the native MIPS addressing mode. The .got section usually comes first in the link map and so .got + 0x7ff0 = _gp.
Somewhere in the linking process the linker calculates the signed offset necessary for each instruction accessing data in these sections based on the _gp symbol from the link scripts and puts that into the appropriate offset field in the instruction.
For us, I don't believe it matters where _gp is, as long as the relative offsets to all small items can be held in the 16 bit field and that _gp is aligned properly. Obviously we need to fixup the _gp register correctly during relocation. Since _gp is only a pointer, IMHO it makes sense to adhere to the standard map with a 0x7ff0 offset.
(BTW, ${CROSS_COMPILE}ld --verbose will dump the normal link script that gets used in an ELDK setup)
Hi Andrew,
Andrew Dyer wrote:
Typically _gp is set in the linker script to 0x7ff0 above the start of the 'small' sections (all ELF sections with SHF_MIPS_GPREL flag), so that all small items can be accessed by a 16 bit signed offset off of the register allocated for _gp. The 16-bit signed offset corresponds to the native MIPS addressing mode. The .got section usually comes first in the link map and so .got + 0x7ff0 = _gp.
Somewhere in the linking process the linker calculates the signed offset necessary for each instruction accessing data in these sections based on the _gp symbol from the link scripts and puts that into the appropriate offset field in the instruction.
For us, I don't believe it matters where _gp is, as long as the relative offsets to all small items can be held in the 16 bit field and that _gp is aligned properly. Obviously we need to fixup the _gp register correctly during relocation. Since _gp is only a pointer, IMHO it makes sense to adhere to the standard map with a 0x7ff0 offset.
(BTW, ${CROSS_COMPILE}ld --verbose will dump the normal link script that gets used in an ELDK setup)
Good abstract. I hope that I don't miss anything. I'll submit patches later, please review them.
thanks a lot,
Shinya
Dear Wolfgang,
Wolfgang Denk wrote:
I'm afraid I don't remember this any more. The code is as old as the very first MIPS port for the Infineon INCA-IP board, back in March 2003. It is easily possible that we didn't know any better by then, and everybody just copied the code without thinking since.
Thanks for your kind information. I see what you mean.
Speaking for myself, I'm not much of a MIPS expert and will rely on your judgment. All I can do is helping to test changes on the INCA-IP board.
I'll submit patches later. If you have some spare time, give it a try. Unfortunately I don't have any targets maintained in upstream, that'll be very helpful.
thanks,
Shinya
participants (6)
-
Andrew Dyer -
Shinya Kuribayashi -
Shinya Kuribayashi -
Thomas Lange -
Vlad Lungu -
Wolfgang Denk