Bug 1326091 - Incorrect processing of non-consecutive relocation tables
Summary: Incorrect processing of non-consecutive relocation tables
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Fedora
Classification: Fedora
Component: glibc
Version: 23
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Carlos O'Donell
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-04-11 19:24 UTC by Miloslav Trmač
Modified: 2016-05-02 18:01 UTC (History)
10 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-05-02 18:01:51 UTC
Type: Bug


Attachments (Terms of Use)
foo.c (223 bytes, text/x-csrc)
2016-04-12 13:54 UTC, Miloslav Trmač
no flags Details
linker-script (8.67 KB, text/x-csrc)
2016-04-12 13:59 UTC, Miloslav Trmač
no flags Details


Links
System ID Private Priority Status Summary Last Updated
Sourceware 14341 0 None None None 2016-04-30 03:29:38 UTC

Description Miloslav Trmač 2016-04-11 19:24:34 UTC
Version-Release number of selected component (if applicable):
glibc-2.22-11.fc23.x86_64
golang-1.5.3-1.fc23.x86_64

At least with golang-built executables, it is possible for the primary relocation table and the PLT relocation table to be non-consecutive:

>  RELA              0x00000000009deaa0
>  RELASZ            24 (bytes)
>  RELAENT           24 (bytes)
>  PLTREL            RELA
>  PLTRELSZ          600 (bytes)
>  JMPREL            0x00000000009deac0

> Relocation section [ 7] '.rela' at offset 0x5deaa0 contains 1 entry:
>   Offset              Type            Value               Addend Name
>   0x00000000009e0210  X86_64_GLOB_DAT 000000000000000000      +0 stderr
> 
> Relocation section [ 8] '.rela.plt' for section [ 2] '.plt' at offset 0x5deac0 contains 25 entries:
>   Offset              Type            Value               Addend Name
>   0x00000000009e0018  X86_64_JUMP_SLOT 000000000000000000      +0 __errno_location
>   0x00000000009e0020  X86_64_JUMP_SLOT 000000000000000000      +0 getnameinfo
…

In such cases, with LD_BIND_NOW=1, elf/dynamic-link.h seems to process relocations incorrectly.

Admittedly the ELF spec is pretty vague about the DT_RELA and DT_JMPREL relationship, but at least the comment above _ELF_DYNAMIC_DO_RELOC suggests that having such non-consecutive relocation tables is an expected case.

With LD_BIND_NOW,
>        if (! ELF_DURING_STARTUP && ((do_lazy) || ranges[0].size == 0))       \
is false due to do_lazy == 0, and control flows into the else branch which blindly increases the RELA size by DT_PLTRELSZ. But because the real start of JMPREL is 8 bytes after the end of RELA, this causes the r_offset of the first entry (for __errno_location) to be interpreted as an r_info, and linking fails with
> $path: error while loading shared libraries: unexpected reloc type 0x009e0018

Comment 1 Carlos O'Donell 2016-04-11 21:04:31 UTC
(In reply to Miloslav Trmač from comment #0)
> In such cases, with LD_BIND_NOW=1, elf/dynamic-link.h seems to process
> relocations incorrectly.

Please provide a reduced test case that shows the problem.

Thanks.

Comment 2 Miloslav Trmač 2016-04-12 13:54:36 UTC
Created attachment 1146459 [details]
foo.c

Comment 3 Miloslav Trmač 2016-04-12 13:59:06 UTC
Created attachment 1146464 [details]
linker-script

To reproduce:
> $ gcc -Wl,-T,linker-script -Wall -O2 -Wextra foo.c 
> $ ./a.out # This works fine
> Hello world
> $ LD_BIND_NOW=1 ./a.out 
> ./a.out: error while loading shared libraries: unexpected reloc type 0x00601018

linker-script substantially differs from the default in (ld --verbose) only in the added ALIGN directives on lines with “MODIFIED” in comments.

For a more extreme demonstration, uncomment the ALTERNATIVE line in linker-script, then:
> $ LD_BIND_NOW=1 ./a.out 
> Segmentation fault (core dumped)

Comment 4 Jakub Jelinek 2016-04-12 14:02:04 UTC
That is IMHO a golang bug or whatever adds the gaps in there.  Don't do that.

Comment 5 Miloslav Trmač 2016-04-12 14:08:10 UTC
*shrug* other than reciting the comment in dynamic-link.h I don’t feel qualified to say one way or the other, feel free to reassign to golang.

Comment 6 Miloslav Trmač 2016-04-12 14:17:01 UTC
FWIW https://go.googlesource.com/go/+/83b25d9342b0ce6620f38419ff74c6d16b43f554%5E!/ , though I have no idea whether that is enough to make the sections contiguous.

Comment 7 Carlos O'Donell 2016-04-30 03:29:39 UTC
How serious is this for F23?

This is fixed in F24.

commit fa19d5c48a6b36518ce68412e3bdde6bfa8aa4a6
Author: Petar Jovanovic <petar.jovanovic>
Date:   Wed Aug 20 00:50:54 2014 +0200

    Fix dynamic linker issue with bind-now
    
    Fix the bind-now case when DT_REL and DT_JMPREL sections are separate
    and there is a gap between them.
    
        [BZ #14341]
        * elf/dynamic-link.h (elf_machine_lazy_rel): Properly handle the
        case when there is a gap between DT_REL and DT_JMPREL sections.
        * sysdeps/x86_64/Makefile (tests): Add tst-split-dynreloc.
        (LDFLAGS-tst-split-dynreloc): New.
        (tst-split-dynreloc-ENV): Likewise.
        * sysdeps/x86_64/tst-split-dynreloc.c: New file.
        * sysdeps/x86_64/tst-split-dynreloc.lds: Likewise.

Comment 8 Miloslav Trmač 2016-04-30 03:39:03 UTC
Thanks for fixing this!

As far as I am concerned, this is not urgent. I have only used LD_BIND_NOW=1 for debugging, not for running production code, so this bug is not blocking any progress for me.

OTOH the behavior was pretty surprising and led me on a 2-day debugging chase, it would be nice to spare others the same pain if it were easy enough (and others were likely to run into this; I have no idea how frequently anyone tries LD_BIND_NOW=1 with golang).

Comment 9 Carlos O'Donell 2016-04-30 03:53:15 UTC
(In reply to Miloslav Trmač from comment #8)
> Thanks for fixing this!
> 
> As far as I am concerned, this is not urgent. I have only used LD_BIND_NOW=1
> for debugging, not for running production code, so this bug is not blocking
> any progress for me.
> 
> OTOH the behavior was pretty surprising and led me on a 2-day debugging
> chase, it would be nice to spare others the same pain if it were easy enough
> (and others were likely to run into this; I have no idea how frequently
> anyone tries LD_BIND_NOW=1 with golang).

Are you packaging any of those built binaries?

The Fedora Packaging Guidlines are pretty clear about this, you should be using `-Wl,-z,now' for increased security if the resulting binary meets the criteria spelled out in the guide.

https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#PIE

Comment 10 Miloslav Trmač 2016-04-30 04:18:33 UTC
> Are you packaging any of those built binaries?

Various golang binaries are packaged in Fedora in general. It is kind of hard to make promises about the binaries’ (lack of) use an odd number of .rela.dyn relocations :) Right now the tool which motivated this bug report is no longer such a case.

> The Fedora Packaging Guidlines are pretty clear about this, you should be using `-Wl,-z,now' for increased security if the resulting binary meets the criteria spelled out in the guide.

Oh, I didn't realize this bug also manifests for DT_BIND_NOW; I thought that only explicit LD_BIND_NOW would be affected.  This is a good point.

You’re right about the guidelines asking for DT_BIND_NOW; but right now, the default golang build macros don't use _hardened_build, and from a quick grep of /usr/lib/golang/src/cmd/link I am not sure the toolchain can set this flag at all.

Comment 11 Carlos O'Donell 2016-05-02 18:01:51 UTC
(In reply to Miloslav Trmač from comment #10)
> > The Fedora Packaging Guidlines are pretty clear about this, you should be using `-Wl,-z,now' for increased security if the resulting binary meets the criteria spelled out in the guide.
> 
> Oh, I didn't realize this bug also manifests for DT_BIND_NOW; I thought that
> only explicit LD_BIND_NOW would be affected.  This is a good point.

They enable the same behaviour.
 
> You’re right about the guidelines asking for DT_BIND_NOW; but right now, the
> default golang build macros don't use _hardened_build, and from a quick grep
> of /usr/lib/golang/src/cmd/link I am not sure the toolchain can set this
> flag at all.

Given the fix in F24 I'm going to consider this CLOSED/WONTFIX for F23.

I suggest you start work in F24 going forward to harden the builds to match Fedora Packaging Guidelines. That might include having a Fedora-specific patch to harden the link command for golang.


Note You need to log in before you can comment on or make changes to this bug.