Re: [PATCH] fix tls offsets when p_vaddr%p_align != 0 for TLS_ABOVE_TP

[Szabolcs Nagy <nsz@port70.net>]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]

* Rich Felker <dalias@libc.org> [2019-05-16 09:22:46 -0400]:
> > i can make the tls_offset fix a separate change from
> > the p_vaddr%p_align!=0 fix.
> 
> I'd like that, both because it was non-obvious to me that this was
> another major change, and because it fixes a separate, major
> user-facing bug (this is what was breaking rust, and could break lots
> of other things) rather than the new issue with lld's Bionic hack.

ok

[-- Attachment #2: 0001-fix-static-tls-offsets-of-shared-libs-on-TLS_ABOVE_T.patch --]
[-- Type: text/x-diff, Size: 2089 bytes --]

From 36d6bcf5b1a0add3f825aec57b72630520a18ccb Mon Sep 17 00:00:00 2001
From: Szabolcs Nagy <nsz@port70.net>
Date: Thu, 16 May 2019 17:15:33 +0000
Subject: [PATCH 1/2] fix static tls offsets of shared libs on TLS_ABOVE_TP
 targets

tls_offset should always point to the end of the allocated static tls
area, but this was not handled correctly on "tls variant 1" targets
in the dynamic linker:

after application tls was allocated, tls_offset was aligned up,
potentially wasting tls space. (alignment may be needed at the
begining of the tls area, not at the end, but that will be fixed
separately as it is unlikely to affect real binaries.)

when static tls was allocated for a shared library, tls_offset was
only updated with the size of the tls segment which does not include
alignment gaps, which can easily happen if the tls size update for
one library leaves tls_offset misaligned for the next one. this can
cause oob access in __copy_tls or arbitrary breakage at tls access.
(the issue was observed on aarch64 with rust binaries)
---
 ldso/dynlink.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index 42a5470d..566b69e6 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -1128,7 +1128,7 @@ static struct dso *load_library(const char *name, struct dso *needed_by)
 #ifdef TLS_ABOVE_TP
 		p->tls.offset = tls_offset + ( (tls_align-1) &
 			-(tls_offset + (uintptr_t)p->tls.image) );
-		tls_offset += p->tls.size;
+		tls_offset = p->tls.offset + p->tls.size;
 #else
 		tls_offset += p->tls.size + p->tls.align - 1;
 		tls_offset -= (tls_offset + (uintptr_t)p->tls.image)
@@ -1798,9 +1798,7 @@ _Noreturn void __dls3(size_t *sp)
 #ifdef TLS_ABOVE_TP
 		app.tls.offset = GAP_ABOVE_TP;
 		app.tls.offset += -GAP_ABOVE_TP & (app.tls.align-1);
-		tls_offset = app.tls.offset + app.tls.size
-			+ ( -((uintptr_t)app.tls.image + app.tls.size)
-			& (app.tls.align-1) );
+		tls_offset = app.tls.offset + app.tls.size;
 #else
 		tls_offset = app.tls.offset = app.tls.size
 			+ ( -((uintptr_t)app.tls.image + app.tls.size)
-- 
2.21.0


[-- Attachment #3: 0002-fix-tls-offsets-when-p_vaddr-p_align-0-on-TLS_ABOVE_.patch --]
[-- Type: text/x-diff, Size: 2412 bytes --]

From 28dd0b581b4d8b0903447b5e169155f11be94cf7 Mon Sep 17 00:00:00 2001
From: Szabolcs Nagy <nsz@port70.net>
Date: Mon, 13 May 2019 18:47:11 +0000
Subject: [PATCH 2/2] fix tls offsets when p_vaddr%p_align != 0 on TLS_ABOVE_TP
 targets

currently the bfd linker does not seem to create tls segments where
p_vaddr%p_align != 0, but this is valid in ELF and then the runtime
computed tls offset must satisfy

  offset%p_align == (base+p_vaddr)%p_align

and in case of local exec tls (main executable) the smallest such
offset must be used (otherwise it is incompatible with the offset
computed by the static linker). the !TLS_ABOVE_TP case is handled
correctly (the offset is negative then in the formula).

the ldso code for TLS_ABOVE_TP is changed so the static tls offset
of each module satisfies the formula.
---
 ldso/dynlink.c       | 7 ++++---
 src/env/__init_tls.c | 3 ++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/ldso/dynlink.c b/ldso/dynlink.c
index 566b69e6..f9a7cc06 100644
--- a/ldso/dynlink.c
+++ b/ldso/dynlink.c
@@ -1126,8 +1126,8 @@ static struct dso *load_library(const char *name, struct dso *needed_by)
 		p->tls_id = ++tls_cnt;
 		tls_align = MAXP2(tls_align, p->tls.align);
 #ifdef TLS_ABOVE_TP
-		p->tls.offset = tls_offset + ( (tls_align-1) &
-			-(tls_offset + (uintptr_t)p->tls.image) );
+		p->tls.offset = tls_offset + ( (p->tls_align-1) &
+			(-tls_offset + (uintptr_t)p->tls.image) );
 		tls_offset = p->tls.offset + p->tls.size;
 #else
 		tls_offset += p->tls.size + p->tls.align - 1;
@@ -1797,7 +1797,8 @@ _Noreturn void __dls3(size_t *sp)
 		app.tls_id = tls_cnt = 1;
 #ifdef TLS_ABOVE_TP
 		app.tls.offset = GAP_ABOVE_TP;
-		app.tls.offset += -GAP_ABOVE_TP & (app.tls.align-1);
+		app.tls.offset += (-GAP_ABOVE_TP + (uintptr_t)app.tls.image)
+			& (app.tls.align-1);
 		tls_offset = app.tls.offset + app.tls.size;
 #else
 		tls_offset = app.tls.offset = app.tls.size
diff --git a/src/env/__init_tls.c b/src/env/__init_tls.c
index 5f12500c..772baba3 100644
--- a/src/env/__init_tls.c
+++ b/src/env/__init_tls.c
@@ -115,7 +115,8 @@ static void static_init_tls(size_t *aux)
 		& (main_tls.align-1);
 #ifdef TLS_ABOVE_TP
 	main_tls.offset = GAP_ABOVE_TP;
-	main_tls.offset += -GAP_ABOVE_TP & (main_tls.align-1);
+	main_tls.offset += (-GAP_ABOVE_TP + (uintptr_t)main_tls.image)
+		& (main_tls.align-1);
 #else
 	main_tls.offset = main_tls.size;
 #endif
-- 
2.21.0