Hello Yann, All, Thanks for the detailed analysis!
Le 22/05/2021 à 03:08, Yann Sionneau a écrit : > Hello, > > I think my previous analysis showing the r31 and r13 computations are OK still > hold but my conclusion that the issue is with relocation is false. By enabling some debug option in uClibc-ng I tried to understand the working vs crashing case: I noticed a similar result regarding the "_dl_elf_main" address. https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/ldso/ldso/dl-startup.c#n364 In the crashing case (#if): transfering control to application @ 0xb0224 In the working case (#else): transfering control to application @ 0x1009a4b4 > > I attach a small patch that seems to greatly improve the situation (but I > think > it's not the final patch yet, some other things might need changes) > > I think the issue is due to the fact that now (since > https://github.com/buildroot/buildroot/commit/f9b539bf4054d55da69280b19f4b99a91cbe6e0b) > gcc is calling gas using -msecureplt, enable the secureplt. I didn't tried to revert this patch and enable BR2_PIC_PIE in Buildroot at the same time. > > Previous PLT was code written on-the-fly from the uClibc relocation handler > (which has the drawback of needing writable PLT): > https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/ldso/ldso/powerpc/elfinterp.c#n237 > > New PLT, instead of just having a relocation pointing to it contains real code > like this: > > 000b56a0 <00000000.plt_pic32.__uClibc_main>: > b56a0: 81 7e 03 7c lwz r11,892(r30) > b56a4: 7d 69 03 a6 mtctr r11 > b56a8: 4e 80 04 20 bctr > b56ac: 60 00 00 00 nop > > This is where the crash is, the lwz triggers a segfault because r30 is not > initialized. > > The new PLT uses r30 as got pointer. > > by just adding a "r30 = r31" in crt1.S I get buildroot to boot. (see attached > patch) > > We can see that in glibc start.S r30 is indeed set: > https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/powerpc/powerpc32/start.S;h=39ce1a18ff6fe19cf0f5c0d8344f46cbb1fff998;hb=HEAD#l80 Indeed, I noticed r30 being used by Glibc. > > Also see the comment above. > > So I think the right direction is keeping PPC_REL16 code and adding the > missing > r30 got pointer. > > I must still be missing something because for instance ldd crashes. > > What do you think? I guess we can follow Glibc developers and only support the HAVE_ASM_PPC_REL16 case. I don't think anyone is building a toolchain with an old binutils missing ppc REL16 support. Best regards, Romain > > Le 21/05/2021 à 16:48, Yann Sionneau a écrit : >> >> Hello, >> >> I've stayed silent so far but I've done some investigation on this one. >> >> It's not yet clear to me why there is this code path: >> >> |#ifdef __PIC__ # ifdef HAVE_ASM_PPC_REL16 bcl 20,31,1f 1: mflr r31 addis >> r31,r31,_GLOBAL_OFFSET_TABLE_-1b@ha addi r31,r31,_GLOBAL_OFFSET_TABLE_-1b@l # >> else bl _GLOBAL_OFFSET_TABLE_-4@local mflr r31 # endif #endif ||| >> >> If I understood correctly, the "new" (so I guess "better ?") way of doing is >> by using the PPC_REL16 reloc. >> >> Code seems bigger, and uses 2 relocs, compared to the other code path which >> uses just 1 reloc. >> >> So, why is the 1st better ? >> >> I am betting here that : it's because by using 2 16-bit relocs, you can >> indeed >> address 32 bit and reach the .got even if it is very far away from the >> current >> code. >> >> If you are stuck with only 16 bits ... it can fail. >> >> So, I am guessing (please if someone can confirm) that your fix will work in >> most cases ... but can fail when got is too far away to fit in the 16 bit >> reloc targeting the "bl" insn immediate. >> >> I think it would be better to understand why the first code path does not >> work >> well. >> >> Here is the current state of my investigations: >> >> I compiled both versions (with and without ppc_rel16) to compare what's >> happening in both cases. >> >> busybox_unstripped in the no_rel16 (#else) case: >> https://salamandar.fr/lufi/r/4TrBm3U7q5#z3J3Z3G24BTPJnQKD/EzGhAk/n3YHZSZF3hNde2tU98= >> >> busybox_unstripped in the ppc_rel16 (#if) case: >> https://salamandar.fr/lufi/r/pQ79KBb5wU#HFlcBZIVngZFVzvObe/afqeNgMEbMtwDk9ajmplFRtA= >> >> In the working case (#else): >> >> ################# >> >> the _start symbol contains: >> >> b022c: 48 03 fd f9 bl f0024 <__TMC_END__+0x10> >> b0230: 7f e8 02 a6 mflr r31 >> b0234: 81 bf ff f0 lwz r13,-16(r31) >> >> to load the r13 reg with SDA (Small Data Area) pointer. >> >> 0xf0024 is in the got which contains: >> >> 000f0014 <.got>: >> ... >> f0024: 4e 80 00 21 blrl >> >> 000f0028 <_GLOBAL_OFFSET_TABLE_>: >> f0028: 00 0e ff 34 .long 0xeff34 >> ... >> so basically we jump to the got, we execute "blrl" which IIUC jumps back to >> our _start but with LR (link register) == 0xf0028 (nextpc) >> >> then mflr (move from link register) makes it so that r31 == 0xf0028 >> >> Then we lwz r13 = * (0xf0028 - 16) == * 0xf0018 >> >> 0xf0018 is in the .got too and seems to be the target of a R_PPC_RELATIVE >> relocation which I *think* will end up with the 0xf0028 value which is the >> address of "_GLOBAL_OFFSET_TABLE_" symbol. >> >> In http://refspecs.linux-foundation.org/elf/elfspec_ppc.pdf we can read p.76: >> >> "In a shared object,_SDA_BASE_is defined to have the same value >> as_GLOBAL_OFFSET_TABLE_" >> >> So it seems all is indeed OK. >> >> |##############################################| >> >> |Now, in the crashing case (HAVE_ASM_PPC_REL16 = 1):| >> >> |||| >> >> the _start symbol contains: >> >> b022c: 42 9f 00 05 bcl 20,4*cr7+so,b0230 <_start+0xc> >> b0230: 7f e8 02 a6 mflr r31 >> b0234: 3f ff 00 05 addis r31,r31,5 >> b0238: 3b ff f8 6c addi r31,r31,-1940 >> b023c: 81 bf ff f4 lwz r13,-12(r31) >> >> So we branch to 0xb0230 (nextpc) to move link register to r31 which ends up >> with the value 0xb0230. >> >> we do r31 = r31 + 0x10000 * 5 = 0x100230 >> >> we do r31 = r31 - 1940 = 0xffa9c >> >> Then we load r13 = *(0xffa9c - 12) = *(0xffa90) >> >> 0xffa90 seems to be the target of a R_PPC_RELATIVE relocation that targets >> 0xffa9c which is the address of the _GLOBAL_OFFSET_TABLE_ symbol. >> >> The code seems correct, I am guessing the relocation handling might be the >> cause of the problem? >> >> It might be interesting to compare glibc and uclibc-ng for PowerPC for the >> handling of R_PPC_RELATIVE reloc. >> >> Sorry I don't have any quick fix, I'm just sharing my thoughts on the issue. >> >> Cheers! >> >> Yann >> >> On 21/05/2021 10:34, Waldemar Brodkorb wrote: >>> Hi Romain, >>> >>> can you confirm that attached patch works for you? >>> I tested with non-PIE and PIE and both seems to work. >>> >>> best regards >>> Waldemar >>> >>> Romain Naour wrote, >>> >>>> Hi Waldemar, >>>> >>>> Le 19/05/2021 à 00:09, Waldemar Brodkorb a écrit : >>>>> Hi Romain, >>>>> Romain Naour wrote, >>>>> >>>>>> Hello, >>>>>> >>>>>> Recently in Buildroot the option BR2_PIC_PIE has been enabled by default >>>>>> along >>>>>> with other hardening features [1]. Since then some ppc defconfig such >>>>>> qemu_ppc_e500mc_defconfig are failing to boot due to a segfault in init >>>>>> program. >>>>>> >>>>>> The segfault appear very early in __uClibc_main while starting any >>>>>> binaries, >>>>>> an issue located in crt1.S (powerpc)[2]. >>>>>> >>>>>> After some trial and error, removing HAVE_ASM_PPC_REL16 from CFLAGS [3] >>>>>> allow to generate a working system again. But this is actually wrong >>>>>> since >>>>>> instead we should consider HAVE_ASM_PPC_REL16 always true nowadays. >>>>> What if the assembly inside HAVE_ASM_PPC_REL16 isn't pie safe? >>>> Good question. >>>> >>>> I guess it should work with pie (see PIEFLAG_NAME:=-fpie) >>>> https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/Rules.mak?h=v1.0.38#n480 >>>> >>>> I did a try with Glibc without any problem with BR2_PIC_PIE enabled. >>>> >>>> Best regards, >>>> Romain >>>> >>>>>> Indeed, Glibc removed HAVE_ASM_PPC_REL16 since version 2.22 [4] since >>>>>> "the >>>>>> minimum binutils supports rel16 relocs". Binutils 2.22 supports >>>>>> R_PPC_REL16 as >>>>>> default. >>>>>> >>>>>> uClibc-ng should remove HAVE_ASM_PPC_REL16 but keep the code as it was >>>>>> defined. >>>>>> But this doesn't fix the initial issue. >>>>>> >>>>>> Any idea ? >>>>>> >>>>>> [1] >>>>>> https://git.buildroot.net/buildroot/commit/?id=810ba387bec3c5b6904e8893fb4cb6f9d3717466 >>>>>> [2] >>>>>> https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/libc/sysdeps/linux/powerpc/crt1.S?id=2bf4991c4dd7b50b74656011dea9c40464ff390c#n47 >>>>>> [3] >>>>>> https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/Rules.mak?id=2bf4991c4dd7b50b74656011dea9c40464ff390c#n486 >>>>>> [4] >>>>>> https://sourceware.org/git/?p=glibc.git;a=commit;h=59261ad3eb345e0d7b9f5c73e1a09d046991cea5 >>>>> best regards >>>>> Waldemar >>>>> >>>> >>>> _______________________________________________ >>>> devel mailing list >>>> devel@uclibc-ng.org >>>> https://mailman.uclibc-ng.org/cgi-bin/mailman/listinfo/devel >> >> _______________________________________________ >> devel mailing list >> devel@uclibc-ng.org >> https://mailman.uclibc-ng.org/cgi-bin/mailman/listinfo/devel > > _______________________________________________ > devel mailing list > devel@uclibc-ng.org > https://mailman.uclibc-ng.org/cgi-bin/mailman/listinfo/devel > _______________________________________________ devel mailing list devel@uclibc-ng.org https://mailman.uclibc-ng.org/cgi-bin/mailman/listinfo/devel
