[U-Boot-Users] [PATCH] Off-by-two bug when relocating GOT

[Shinya Kuribayashi]

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