Fix TLS relocations against local symbols on powerpc32, sparc32 and sparc64

* sysdeps/powerpc/powerpc32/dl-machine.h (elf_machine_rela):
	Assign sym_map to be map for local symbols, as TLS relocations
	use sym_map to determine whether the symbol is defined and to
	extract the TLS information.
	* sysdeps/sparc/sparc32/dl-machine.h (elf_machine_rela):
	Likewise.
	* sysdeps/sparc/sparc64/dl-machine.h (elf_machine_rela):
	Likewise.
---
 sysdeps/powerpc/powerpc32/dl-machine.h | 5 ++++-
 sysdeps/sparc/sparc32/dl-machine.h     | 1 +
 sysdeps/sparc/sparc64/dl-machine.h     | 1 +
 3 files changed, 6 insertions(+), 1 deletion(-)

Normally, TLS relocations against local symbols are optimised by the linker to
be absolute. However, gold does not do this, and so it is possible to end up
with, for example, R_SPARC_TLS_DTPMOD64 referring to a local symbol. Since
sym_map is left as null in elf_machine_rela for the special local symbol case,
the relocation handling thinks it has nothing to do, and so the module gets
left as 0. Havoc then ensues when the variable in question is accessed. I have
written http://paste.debian.net/plain/984146 to demonstrate the problem. Before
this fix, the main_local_gold program would receive a SIGBUS on sparc64, and
SIGSEGV on powerpc32. With this fix applied, that test now passes like the rest
of them.

--
2.14.1

On Sat, Sep 02, 2017 at 07:08:01PM +0100, James Clarke wrote:
> Normally, TLS relocations against local symbols are optimised by the linker to
> be absolute. However, gold does not do this, and so it is possible to end up
> with, for example, R_SPARC_TLS_DTPMOD64 referring to a local symbol.

That is actually correct behaviour.  The linker can't resolve DTPMOD
relocs in a shared library at link time, since it doesn't know the
tls module id that will be assigned at run time.  So a dynamic DTPMOD
reloc must be emitted against a local symbol or SHN_UNDEF (symbol
index zero).  There are other cases too where TLS relocs must be
dynamic in a shared library.  For example, TPREL relocs must be
dynamic because the linker doesn't know where the TLS segment of the
library will be laid out relative to the thread pointer.

I guess what you're really saying is that gold doesn't make use of the
fact that glibc ld.so handles symbol index zero in these cases, but
instead emits a local dynamic symbol to use by the dynamic reloc.

> Since
> sym_map is left as null in elf_machine_rela for the special local symbol case,
> the relocation handling thinks it has nothing to do, and so the module gets
> left as 0.

Yes, thanks for noticing!  The only time we should have sym_map NULL
is for an undefined weak symbol.  The patch looks good to me.

Ping?

On 3 Sep 2017, at 04:47, Alan Modra <amodra@gmail.com> wrote:
> On Sat, Sep 02, 2017 at 07:08:01PM +0100, James Clarke wrote:
>> Normally, TLS relocations against local symbols are optimised by the linker to
>> be absolute. However, gold does not do this, and so it is possible to end up
>> with, for example, R_SPARC_TLS_DTPMOD64 referring to a local symbol.
> 
> That is actually correct behaviour.  The linker can't resolve DTPMOD
> relocs in a shared library at link time, since it doesn't know the
> tls module id that will be assigned at run time.  So a dynamic DTPMOD
> reloc must be emitted against a local symbol or SHN_UNDEF (symbol
> index zero).  There are other cases too where TLS relocs must be
> dynamic in a shared library.  For example, TPREL relocs must be
> dynamic because the linker doesn't know where the TLS segment of the
> library will be laid out relative to the thread pointer.
> 
> I guess what you're really saying is that gold doesn't make use of the
> fact that glibc ld.so handles symbol index zero in these cases, but
> instead emits a local dynamic symbol to use by the dynamic reloc.

Yes, perhaps I should have been clearer in my original message. I never meant
to imply that gold was emitting incorrect relocations (for once it wasn't its
fault!).

>> Since
>> sym_map is left as null in elf_machine_rela for the special local symbol case,
>> the relocation handling thinks it has nothing to do, and so the module gets
>> left as 0.
> 
> Yes, thanks for noticing!  The only time we should have sym_map NULL
> is for an undefined weak symbol.  The patch looks good to me.

Regards,
James

James Clarke <jrtc27@jrtc27.com> writes:

> Ping?
>
> On 3 Sep 2017, at 04:47, Alan Modra <amodra@gmail.com> wrote:
>>
>> Yes, thanks for noticing!  The only time we should have sym_map NULL
>> is for an undefined weak symbol.  The patch looks good to me.

As far as I understand, Alan already reviewed and approved this patch.

However, it would be ideal if David Miller (SPARC maintainer, in Cc.) would
review it too.

I can push it after David's approval.

Thanks!

On 18 Sep 2017, at 14:57, Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com> wrote:
> 
> James Clarke <jrtc27@jrtc27.com> writes:
> 
>> Ping?
>> 
>> On 3 Sep 2017, at 04:47, Alan Modra <amodra@gmail.com> wrote:
>>> 
>>> Yes, thanks for noticing!  The only time we should have sym_map NULL
>>> is for an undefined weak symbol.  The patch looks good to me.
> 
> As far as I understand, Alan already reviewed and approved this patch.
> 
> However, it would be ideal if David Miller (SPARC maintainer, in Cc.) would
> review it too.
> 
> I can push it after David's approval.

David, ping?

Thanks,
James

(https://sourceware.org/ml/libc-alpha/2017-09/msg00120.html for the actual
patch in question to save you looking for it yourself.)

James Clarke <jrtc27@jrtc27.com> writes:

> 	* sysdeps/powerpc/powerpc32/dl-machine.h (elf_machine_rela):
> 	Assign sym_map to be map for local symbols, as TLS relocations
> 	use sym_map to determine whether the symbol is defined and to
> 	extract the TLS information.
> 	* sysdeps/sparc/sparc32/dl-machine.h (elf_machine_rela):
> 	Likewise.
> 	* sysdeps/sparc/sparc64/dl-machine.h (elf_machine_rela):
> 	Likewise.

Pushed as 864458880721.

Thanks!