Bug 20113 - Partial relro support for s390x ld
Summary: Partial relro support for s390x ld
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: 2.27
: P2 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-18 18:24 UTC by David Edelsohn
Modified: 2019-03-03 18:54 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Built upon 2.31.1 (62.12 KB, application/x-rar)
2018-10-23 19:19 UTC, maamountki
Details
partial relro support for ld (4.20 KB, patch)
2018-12-08 06:50 UTC, maamountki
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Edelsohn 2016-05-18 18:24:07 UTC
Add support to GNU ld to separate got related plt entries from normal ones in order to be able to switch the non-plt got entries to read-only after startup, conforming to revised Linux for zSeries ABI.
Comment 1 maamountki 2018-10-23 19:19:57 UTC
Created attachment 11363 [details]
Built upon 2.31.1

Add full and partial relro support for s390x ld. Tested by the available testsuite and some simple tests, Wider testing is still required in your side before merging.
Comment 2 Nick Clifton 2018-11-06 15:36:48 UTC
Hi maamountki,

> Created attachment 11363 [details]
> Built upon 2.31.1

Would it be possible for you to provide these fixes as a set of context diffs ?

Also - do you have an FSF copyright assignment for the Binutils project ?
Without it we will be unable to accept these patches. :-(

Cheers
  Nick
Comment 3 maamountki 2018-11-06 17:36:02 UTC
(In reply to Nick Clifton from comment #2)
> Also - do you have an FSF copyright assignment for the Binutils project ?

No, Actually these changes weren't meant to be merged directly into binutils project but to be sent to IBM team in response to bounty they offered. If that's inappropriate, You can discard this patch, Sad to see this issue re-patched, tho. Otherwise, Let me know so I can provide set of context diffs.
Comment 4 David Edelsohn 2018-11-06 17:38:02 UTC
The bounty is resolution of this issue, which means merger of the patch into Binutils. There is no bounty to deliver the patches directly to IBM.
Comment 5 Nick Clifton 2018-11-06 17:54:14 UTC
Hi mammountki,

> > Also - do you have an FSF copyright assignment for the Binutils project ?
> 
> No, 

OK, well in order to get them in to the binutils you need to get an
FSF copyright assignment in place.  So please could you fill out one of
the forms from Gnulib documentation project:

http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=tree;f=doc/Copyright;h=22db9a802b4da96ad455cd933351c1359108f95d;hb=HEAD

(I would suggest that the request-assign.changes is the most appropriate
one, unless you want to make more contributions in the future).  

Fill it out, and send it off to the FSF so that they can process it, and
then once that is done we can start the patch review/acceptance process.


> Otherwise, Let me know so I can provide set of context diffs.

Onec the assignment is in place, please could you resend the changes as
context diffs, as this does make them a lot easier to review.  Thanks.

Cheers
  Nick
Comment 6 maamountki 2018-11-10 10:47:46 UTC
Hi Nick,
Sorry for the delay but so far I haven't received any response from assign@gnu.org so It's likely to be delayed even further, Sorry again and thanks for your patience.
Comment 7 maamountki 2018-12-08 06:50:02 UTC
Created attachment 11438 [details]
partial relro support for ld

The assignment is in place by now, I attached the context diffs patch.
Info about the the patch:
According to document here https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=bfd/elf32-s390.c;h=56008a13eb2aaf17927e6706126d0351f239b335;hb=0860693812fff944ab0602e72b762a4a2078da5b#l467 the first 3 words of GOT (.got.plt if exist or .got otherwise) are reserved we can safely put them in the relro area in the partial relro case so the size of these entries can be set to the SEPARATE_GOTPLT argument.
Since the linker default relro option is partial, most of the ld s390 tests will fail, this patch fixed them too.
note: I just notice the full relro option of ld is working properly so this patch actually just add partial one, Sorry for claiming that first.
Comment 8 Nick Clifton 2019-01-02 11:17:11 UTC
Hi maamountki,

  (Thanks for obtaining the FSF copyright assignment.  We can now really
  review this patch...)

> Created attachment 11438 [details]
> partial relro support for ld

Thanks for creating this patch - it was much easier to apply it to the
sources, and examine the changes this way.  It does seem to me that the
patch does not actually do very much, other than setting values for
SEPARATE_GOTPLT in the linker emulation parameter files, and then adjusting
the linker tests to match.  Is this correct, or should there be more to
the patch ?  [I am not complaining, just asking...]

Also, with the patch applied I am seeing three new linker testsuite 
failures for a toolchain configured as: --target=s390x-ibm-tpf  This
strikes me as a little odd, as I thought that the tpf configuration
was basically the same as ordinary elf.  Anyway, please could you
investigate and fix the patch as necessary ?

Cheers
  Nick
Comment 9 maamountki 2019-01-02 18:42:36 UTC
Hi Nick,
Well, It's correct but there one thing we should care about is the relocations, Testsuite doesn't cover all the relocations types I tested a few more but not all of them because I see the relocations are bounded properly to .got and .got.plt sections in bfd while the linker script take care of positioning these sections so I think it's ok btw this is not the case for gold which require me to do a little work on relocations since the sections are positioned manually there.

While toolchain configured as: --target=s390x-ibm-tpf I got the same testsuite 
failures without the patch applied.
Comment 10 Nick Clifton 2019-01-14 16:04:52 UTC
(In reply to maamountki from comment #9)

Hi Maamount,

  OK, then I have now applied the patch.  Thanks very much for persisting with this.

Cheers
  Nick
Comment 11 Nick Clifton 2019-01-14 16:09:44 UTC
oops - the commit message was accidentally sent to PR 20133 rather than this PR.
Comment 12 Andreas Krebbel 2019-02-28 09:56:45 UTC
I've implemented the relro support for Binutils based on the work from Marcin Kościelnicki. Marcin unfortunately didn't finish the bounty so I had to complete it myself. I've decided to do this only for S/390 64 bit since 32 bit binaries do not have a bright future anyway.

Unfortunately I forgot to close this bugzilla accordingly.

I think the patch posted here is wrong. SEPARATE_GOTPLT must remain as 0 on our platform. Please revert the patch.


Here the patches which implement partial relro for IBM Z:

commit afca762f598d453c563f244cd3777715b1a0cb72
Author: Andreas Krebbel <krebbel@linux.vnet.ibm.com>
Date:   Thu Dec 21 13:12:03 2017 +0100

    S/390: Improve partial relro support for 64 bit

commit a38137289e91fd548fc27fb6566a439233b94d65
Author: Andreas Krebbel <krebbel@linux.vnet.ibm.com>
Date:   Mon Jun 11 13:23:00 2018 +0200

    ld: Enable using separate linker script for -z relro
    
    With this patch dedicated linker scripts can be generated for partial
    relro triggered by defining GENERATE_RELRO_SCRIPT in the target
    specific scripts.
    
    This is necessary for e.g. S/390 where usually the .got.plt comes
    first and prevents the relro segment from being extended across the
    non-plt GOT entries.
    
    The patch started with the work from Marcin taken from the mwk user
    branches.  However, the patch needed substantial changes due to the
    'separate code' feature which got committed in the meantime.
    
    ld/ChangeLog:
    
    2018-07-18  Andreas Krebbel  <krebbel@linux.ibm.com>
            Marcin Kościelnicki <koriakin@0x04.net>
Comment 13 Andreas Krebbel 2019-02-28 10:17:45 UTC
Unfortunately the patch also went into 2.32 release :(

At least for 64 bit the patch doesn't hurt since it is a nop. The code currently looks like:

EXTRA_EM_FILE=s390
SEPARATE_GOTPLT=24      <--- the line added with the patch
IREL_IN_PLT=
SEPARATE_GOTPLT=0       <--- the line added with my patch
test -z "$RELRO" && unset SEPARATE_GOTPLT
Comment 14 Andreas Krebbel 2019-03-01 07:42:02 UTC
Maamoun, 

was your patch ever posted on the Binutils mailing list? I could not find it there.
Comment 15 cvs-commit@gcc.gnu.org 2019-03-01 14:42:31 UTC
The master branch has been updated by Andreas Krebbel <krebbel@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=24801b157631434446c2fc5a40994a5555a1db38

commit 24801b157631434446c2fc5a40994a5555a1db38
Author: Andreas Krebbel <krebbel@linux.ibm.com>
Date:   Fri Mar 1 15:23:42 2019 +0100

    Revert "Add support to GNU ld to separate got related plt entries"
    
    bfd/ChangeLog:
    
    2019-03-01  Andreas Krebbel  <krebbel@linux.ibm.com>
    
    	This reverts commit 5a12586d44fa8d5dfc74cbca4f2f36a273a16335.
    	2019-01-14  Maamoun Tarsha  <maamountk@hotmail.com>
    
    	PR 20113
    	* elf32-s390.c (allocate_dynrelocs): Update comment.
    
    ld/ChangeLog:
    
    2019-03-01  Andreas Krebbel  <krebbel@linux.ibm.com>
    
    	This reverts commit 5a12586d44fa8d5dfc74cbca4f2f36a273a16335.
    	2019-01-14  Maamoun Tarsha  <maamountk@hotmail.com>
    
    	PR 20113
    	* emulparams/elf64_s390.sh (SEPARATE_GOTPLT): Define.
    	* emulparams/elf_s390.sh (SEPARATE_GOTPLT): Define.
    	* testsuite/ld-s390/gotreloc_31-1.dd: Update expected output.
    	* testsuite/ld-s390/tlsbin.dd: Likewise.
    	* testsuite/ld-s390/tlsbin.rd: Likewise.
    	* testsuite/ld-s390/tlsbin.sd: Likewise.
    	* testsuite/ld-s390/tlsbin_64.dd: Likewise.
    	* testsuite/ld-s390/tlsbin_64.rd: Likewise.
    	* testsuite/ld-s390/tlsbin_64.sd: Likewise.
    	* testsuite/ld-s390/tlspic.dd: Likewise.
    	* testsuite/ld-s390/tlspic.rd: Likewise.
    	* testsuite/ld-s390/tlspic.sd: Likewise.
    	* testsuite/ld-s390/tlspic_64.dd: Likewise.
    	* testsuite/ld-s390/tlspic_64.rd: Likewise.
    	* testsuite/ld-s390/tlspic_64.sd: Likewise.
    	* testsuite/ld-s390/s390.exp: Skip s390 tests for tpf targets.
Comment 16 cvs-commit@gcc.gnu.org 2019-03-01 14:50:01 UTC
The binutils-2_32-branch branch has been updated by Andreas Krebbel <krebbel@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=9db2b33d363e2047df53f89ad34e7d1104299d12

commit 9db2b33d363e2047df53f89ad34e7d1104299d12
Author: Andreas Krebbel <krebbel@linux.ibm.com>
Date:   Fri Mar 1 15:23:42 2019 +0100

    Revert "Add support to GNU ld to separate got related plt entries"
    
    bfd/ChangeLog:
    
    2019-03-01  Andreas Krebbel  <krebbel@linux.ibm.com>
    
    	This reverts commit 5a12586d44fa8d5dfc74cbca4f2f36a273a16335.
    	2019-01-14  Maamoun Tarsha  <maamountk@hotmail.com>
    
    	PR 20113
    	* elf32-s390.c (allocate_dynrelocs): Update comment.
    
    ld/ChangeLog:
    
    2019-03-01  Andreas Krebbel  <krebbel@linux.ibm.com>
    
    	This reverts commit 5a12586d44fa8d5dfc74cbca4f2f36a273a16335.
    	2019-01-14  Maamoun Tarsha  <maamountk@hotmail.com>
    
    	PR 20113
    	* emulparams/elf64_s390.sh (SEPARATE_GOTPLT): Define.
    	* emulparams/elf_s390.sh (SEPARATE_GOTPLT): Define.
    	* testsuite/ld-s390/gotreloc_31-1.dd: Update expected output.
    	* testsuite/ld-s390/tlsbin.dd: Likewise.
    	* testsuite/ld-s390/tlsbin.rd: Likewise.
    	* testsuite/ld-s390/tlsbin.sd: Likewise.
    	* testsuite/ld-s390/tlsbin_64.dd: Likewise.
    	* testsuite/ld-s390/tlsbin_64.rd: Likewise.
    	* testsuite/ld-s390/tlsbin_64.sd: Likewise.
    	* testsuite/ld-s390/tlspic.dd: Likewise.
    	* testsuite/ld-s390/tlspic.rd: Likewise.
    	* testsuite/ld-s390/tlspic.sd: Likewise.
    	* testsuite/ld-s390/tlspic_64.dd: Likewise.
    	* testsuite/ld-s390/tlspic_64.rd: Likewise.
    	* testsuite/ld-s390/tlspic_64.sd: Likewise.
    	* testsuite/ld-s390/s390.exp: Skip s390 tests for tpf targets.
Comment 17 maamountki 2019-03-03 06:26:58 UTC
sorry for late response.
I don't see any reason to revert the path, the three entries that my path extend the relro segment across are documented here https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=bfd/elf32-s390.c;h=56008a13eb2aaf17927e6706126d0351f239b335;hb=0860693812fff944ab0602e72b762a4a2078da5b#l467
It's safe to do that and most platforms have done something similar to this.
Comment 18 maamountki 2019-03-03 18:16:40 UTC
*patch

I just noticed that SEPARATE_GOTPLT is duplicated in the 64 bit script and partial relro support is already implemented so yes my patch should be reverted, I'm contacting Bountysource to revert the claim submission too. Anyway I'm not sure why you think that SEPARATE_GOTPLT must remain as 0 hence the relro segment doesn't extend across those entries, SEPARATE_GOTPLT have this argument for the exact reason and architectures such as x86 and aarch64 take advantage of this feature and there is a reason actually there are no harm in making those entries non-exploitable.
Comment 19 Andreas Krebbel 2019-03-03 18:54:56 UTC
(In reply to maamountki from comment #18)
> *patch
> 
> I just noticed that SEPARATE_GOTPLT is duplicated in the 64 bit script and
> partial relro support is already implemented so yes my patch should be
> reverted, I'm contacting Bountysource to revert the claim submission too.
> Anyway I'm not sure why you think that SEPARATE_GOTPLT must remain as 0
> hence the relro segment doesn't extend across those entries, SEPARATE_GOTPLT
> have this argument for the exact reason and architectures such as x86 and
> aarch64 take advantage of this feature and there is a reason actually there
> are no harm in making those entries non-exploitable.

For IBM Z we had to chose a slightly different scheme. Our ABI requires that *all* GOT entries (.got + .got.plt) are accessible via _GLOBAL_OFFSET_TABLE_ symbol and a *positive* displacement. Just setting SEPARATE_GOTPLT makes _GLOBAL_OFFSET_TABLE_ to point at the start of .got.plt which would come after the other .got entries. Hence a negative offset would be needed to address .got entries. More changes to the backend were required to make _GLOBAL_OFFSET_TABLE_ end up at the start of .got.

As you say the value of SEPARATE_GOTPLT is used to make the relro segment cover the magic entries assumed to be located at start of .got.plt. With moving _GLOBAL_OFFSET_TABLE_ to start of .got I also had to move the 3 magic entries to the start of .got. So they will be covered by the relro segment anyway. Setting it to 0 is the right value for us then.

I apologize for not monitoring the bugzilla and closing it after the relro work was finished. I know you are doing a great job for other of our bounties (e.g. OpenBlas). Please keep up your great work!