Bug 14207 - linker can produce a NULL GNU_RELRO segment
Summary: linker can produce a NULL GNU_RELRO segment
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: 2.24
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL: http://sourceware.org/ml/binutils/201...
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-07 16:02 UTC by Nick Clifton
Modified: 2014-01-10 11:21 UTC (History)
1 user (show)

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


Attachments
Test source file (132 bytes, application/octet-stream)
2012-06-07 16:02 UTC, Nick Clifton
Details
Skip filesize check when matching segments. (983 bytes, patch)
2012-06-07 16:11 UTC, Nick Clifton
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clifton 2012-06-07 16:02:50 UTC
Created attachment 6438 [details]
Test source file

Using the uploaded relro.s source file, try the following using an x86_64-pc-linux-gnu linker:

  % as relro.s -o relro.o
  % ld -z relro -z now -shared relro.o
  % readelf -l a.out
  [snip]
 Program Headers:
  Type           Offset             VirtAddr           PhysAddr
                 FileSiz            MemSiz              Flags  Align
  LOAD           0x0000000000000000 0x0000000000000000 0x0000000000000000
                 0x00000000000001e0 0x00000000000001e0  R      200000
  LOAD           0x0000000000000b88 0x0000000000200b88 0x0000000000200b88
                 0x0000000000000470 0x0000000000000cb8  RW     200000
  DYNAMIC        0x0000000000000bd0 0x0000000000200bd0 0x0000000000200bd0
                 0x0000000000000180 0x0000000000000180  RW     8
  NULL           0x0000000000000000 0x0000000000000000 0x0000000000000000
                 0x0000000000000000 0x0000000000000000         0
  [snip]

Note how the GNU_RELRO segment has been replaced by an empty segment, and that no error or warning message was produced.
Comment 1 Nick Clifton 2012-06-07 16:11:00 UTC
Created attachment 6439 [details]
Skip filesize check when matching segments.

Here is a proposed patch.  It does two things:

  1,. It adds a warning when a NULL GNU_RELO segment is going to be generated.

  2. It skips the p_filesz check when matching real segments to the requested GNU_RELRO segment.

I suspect that 2) will rejected and the correct patch will be to fix the generation of the initial segment map.  But at least now the problem is out there for people to look at.

Cheers
  Nick
Comment 2 H.J. Lu 2012-06-09 17:04:36 UTC
It also happens in

objcopy -z relro (tdata3)
objcopy -shared -z relro (tbss1)

in ld-elf on Linux/x86-64. We never checked NULL segment
in output.
Comment 3 H.J. Lu 2012-06-10 17:56:47 UTC
A patch is posted at

http://sourceware.org/ml/binutils/2012-06/msg00100.html
Comment 4 cvs-commit@gcc.gnu.org 2012-06-12 06:31:14 UTC
CVSROOT:	/cvs/src
Module name:	src
Changes by:	amodra@sourceware.org	2012-06-12 06:31:07

Modified files:
	bfd            : ChangeLog elf.c 

Log message:
	PR ld/14207
	* elf.c (_bfd_elf_map_sections_to_segments): Disregard bss type
	sections at end of PT_LOAD segment when searching for segment
	that contains end of relro extent.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/bfd/ChangeLog.diff?cvsroot=src&r1=1.5723&r2=1.5724
http://sourceware.org/cgi-bin/cvsweb.cgi/src/bfd/elf.c.diff?cvsroot=src&r1=1.560&r2=1.561
Comment 5 cvs-commit@gcc.gnu.org 2012-06-12 12:55:15 UTC
CVSROOT:	/cvs/src
Module name:	src
Changes by:	hjl@sourceware.org	2012-06-12 12:55:11

Modified files:
	bfd            : ChangeLog elf.c 
	ld/testsuite   : ChangeLog 
	ld/testsuite/ld-x86-64: x86-64.exp 
Added files:
	ld/testsuite/ld-x86-64: pr14207.d pr14207.s 

Log message:
	Abort if PT_GNU_RELRO segment doesn't fit in PT_LOAD segment
	
	bfd/
	
	PR bfd/14207
	* elf.c (assign_file_positions_for_non_load_sections): Abort if
	PT_GNU_RELRO segment doesn't fit in PT_LOAD segment.
	
	ld/testsuite/
	
	PR ld/14207
	* ld-x86-64/x86-64.exp: Run pr14207.
	
	* ld-x86-64/pr14207.d: New file.
	* ld-x86-64/pr14207.s: Likewise.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/bfd/ChangeLog.diff?cvsroot=src&r1=1.5724&r2=1.5725
http://sourceware.org/cgi-bin/cvsweb.cgi/src/bfd/elf.c.diff?cvsroot=src&r1=1.561&r2=1.562
http://sourceware.org/cgi-bin/cvsweb.cgi/src/ld/testsuite/ChangeLog.diff?cvsroot=src&r1=1.1560&r2=1.1561
http://sourceware.org/cgi-bin/cvsweb.cgi/src/ld/testsuite/ld-x86-64/pr14207.d.diff?cvsroot=src&r1=NONE&r2=1.1
http://sourceware.org/cgi-bin/cvsweb.cgi/src/ld/testsuite/ld-x86-64/pr14207.s.diff?cvsroot=src&r1=NONE&r2=1.1
http://sourceware.org/cgi-bin/cvsweb.cgi/src/ld/testsuite/ld-x86-64/x86-64.exp.diff?cvsroot=src&r1=1.49&r2=1.50
Comment 6 H.J. Lu 2012-06-12 12:58:00 UTC
Fixed.
Comment 7 cvs-commit@gcc.gnu.org 2012-07-03 05:47:41 UTC
CVSROOT:	/cvs/src
Module name:	src
Changes by:	amodra@sourceware.org	2012-07-03 05:47:36

Modified files:
	bfd            : ChangeLog elf.c 

Log message:
	PR ld/14207
	* elf.c (assign_file_positions_for_load_sections): Remove assertions
	that only PT_LOAD headers include file header and section headers.
	(assign_file_positions_for_non_load_sections): Similarly don't
	assert PT_GNU_RELRO header does not include file and section headers.
	Compare first section vma rather than PT_LOAD p_vaddr against
	relro_start when looking for PT_LOAD covering PT_GNU_RELRO.  Replace
	abort with assertion.

Patches:
http://sourceware.org/cgi-bin/cvsweb.cgi/src/bfd/ChangeLog.diff?cvsroot=src&r1=1.5742&r2=1.5743
http://sourceware.org/cgi-bin/cvsweb.cgi/src/bfd/elf.c.diff?cvsroot=src&r1=1.564&r2=1.565
Comment 8 cvs-commit@gcc.gnu.org 2014-01-08 14:01:08 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "gdb and binutils".

The branch, master has been updated
       via  43a8475ca01b676fb764aaed0c4ed1cc16fc3c87 (commit)
      from  221fd5d598e7dcf7b953150986a501b462b99891 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=43a8475ca01b676fb764aaed0c4ed1cc16fc3c87

commit 43a8475ca01b676fb764aaed0c4ed1cc16fc3c87
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Thu Dec 12 10:35:47 2013 -0800

    Adjust LOAD segment to generate GNU_RELRO segment
    
    This patch fixes 2 GNU_RELRO segment bugs:
    
    1. lang_size_sections didn't properly align base to the maximum
    alignment power of sections between DATA_SEGMENT_ALIGN and
    DATA_SEGMENT_RELRO_END.
    2. ld failed to adjust LOAD segment to generate GNU_RELRO segment
    when LOAD segment doesn't fit GNU_RELRO segment.  This is
    
    https://sourceware.org/bugzilla/show_bug.cgi?id=14207
    
    We "fixed" ld by not generating GNU_RELRO segment.  This patch
    adjusts LOAD segment to generate GNU_RELRO segment.  It fixes
    PR ld/16322 and at the same time it also fixes PR binutils/16323
    since now we can adjust LOAD segment if it is too small.
    
    bfd/
    
    	PR ld/14207
    	PR ld/16322
    	PR binutils/16323
    	* elf.c (_bfd_elf_map_sections_to_segments): Don't check section
    	size for PT_GNU_RELRO segment.
    	(assign_file_positions_for_load_sections): If PT_LOAD segment
    	doesn't fit PT_GNU_RELRO segment, adjust its p_filesz and p_memsz.
    
    ld/
    
    	PR ld/14207
    	PR ld/16322
    	PR binutils/16323
    	* ldlang.c (lang_size_sections): Properly align RELRO base.
    
    ld/testsuite/
    
    	PR ld/14207
    	PR ld/16322
    	PR binutils/16323
    	* ld-elf/pr16322.d: New file.
    	* ld-elf/pr16322.s: Likewise.
    
    	* ld-x86-64/pr14207.d: Expect PT_GNU_RELRO segment.

-----------------------------------------------------------------------

Summary of changes:
 bfd/ChangeLog                    |   10 +++++++++
 bfd/elf.c                        |   41 +++++++++++++++++++++++++++++++++----
 ld/ChangeLog                     |    7 ++++++
 ld/ldlang.c                      |    3 +-
 ld/testsuite/ChangeLog           |   10 +++++++++
 ld/testsuite/ld-elf/pr16322.d    |    7 ++++++
 ld/testsuite/ld-elf/pr16322.s    |    6 +++++
 ld/testsuite/ld-x86-64/pr14207.d |   23 +++++++++++++++++---
 8 files changed, 97 insertions(+), 10 deletions(-)
 create mode 100644 ld/testsuite/ld-elf/pr16322.d
 create mode 100644 ld/testsuite/ld-elf/pr16322.s
Comment 9 cvs-commit@gcc.gnu.org 2014-01-10 11:21:04 UTC
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "gdb and binutils".

The branch, master has been updated
       via  eec2f3ed9f053653ed5d629eb50e08e3ee61e9bd (commit)
      from  a2cd8cfed14491303eb8338f90e206034c5a3fe2 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

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

commit eec2f3ed9f053653ed5d629eb50e08e3ee61e9bd
Author: Alan Modra <amodra@gmail.com>
Date:   Fri Jan 10 21:11:46 2014 +1030

    Don't adjust LOAD segment to match GNU_RELRO segment
    
    Instead, fix Jakub's original code setting up the PR_GNU_RELRO header
    from the PT_LOAD header.
    
    	PR ld/14207
    	PR ld/16322
    	PR binutils/16323
    bfd/
    	* elf.c (assign_file_positions_for_load_sections): Revert last change.
    	(assign_file_positions_for_non_load_sections): When setting up
    	PT_GNU_RELRO header, don't require a corresponding PT_LOAD
    	header that completely covers the relro region.
    ld/
    	* ldlang.c (lang_size_sections): Remove unneeded RELRO base
    	adjust.  Tidy comments.
    	* ld.texinfo (DATA_SEGMENT_RELRO_END): Correct description.
    ld/testsuite/
    	* ld-x86-64/pr14207.d: Adjust

-----------------------------------------------------------------------

Summary of changes:
 bfd/ChangeLog                    |   10 ++++++++++
 bfd/elf.c                        |   37 -------------------------------------
 ld/ChangeLog                     |    9 +++++++++
 ld/ld.texinfo                    |    6 ++++--
 ld/ldlang.c                      |   21 +++++++++------------
 ld/testsuite/ChangeLog           |    4 ++++
 ld/testsuite/ld-x86-64/pr14207.d |    2 +-
 7 files changed, 37 insertions(+), 52 deletions(-)