Add alignment support to __libc_allocate_tls().

[mmel]

Summary

For statically linked binaries, where all relocation are solved by static
linker, the linker expect that offset to TLS section is aligned. Additionaly,
to maintain absolute alignment, TLS TCB should by also aligned.

Obtained from: CheriBSD(initial version)
MFC after: 1 month

---

I also tested extended version of this patch, which allows variable TCB size.
But this code led to very ugly pointer gymnastic and opened some shadow
corners, e.g. can be __libc_allocate_tls() called with oldtls != NULL and
with different tcbsize as in original call?

Due to this and because we have no consumer for this feature, I decided to
keep the code simple.

[brooks]

Looks good to me. We should probably make similar changes to Variant II.

[kib]

lib/libc/gen/tls.c

128 ↗

(On Diff #34660)

Just a question. I am sure that malloc_aligned and free_aligned were copied from rtld xmalloc.c. Why memshift variable is needed ?

177 ↗

(On Diff #34660)

Libraries must not assert, they should either report error or, if not possible to report, avoid doing wrong operation. Policy of aborting or continuing should be kept in consumers.

180 ↗

(On Diff #34660)

Is it worth checking for result and return NULL there instead of faulting ? IMO yes.

201 ↗

(On Diff #34660)

Space is needed between return and '('.

248 ↗

(On Diff #34660)

Would be nice to fix type II as well.

[brooks]

lib/libc/gen/tls.c

128 ↗

(On Diff #34660)

It isn't if pointers are just integers. In CHERI roundup2() isn't safe for pointers and memshift fixes this.

177 ↗

(On Diff #34660)

I suppose we could return NULL instead and get a confusing failure in jemalloc initialization instead. The idea that recovery is possible isn't really plausible. All not asserting does is move the error somewhere confusing.

[mmel]

lib/libc/gen/tls.c

128 ↗

(On Diff #34660)

I stole this from CHERI, 1:1. But yes, we operate only on integers and xmalloc.c variant looks more readable. I will fix this.

180 ↗

(On Diff #34660)

hmm. _init_tls() returns void. So if we return NULL, then we will end with NULL TP and any kind of later damage...
Does this make more sense for you?

write(STDOUT_FILENO, ....
abort();

248 ↗

(On Diff #34660)

I can fix this, but I have only very limited resources for test it. At this moment, I have only single AMD box, my build server, located nearly 40 km from me in server room.

[kib]

lib/libc/gen/tls.c

128 ↗

(On Diff #34660)

I am fine with either variant, but suggest that rtld and libc versions should be same. I.e. change either in rtld or there.

Perhaps rtld should be updated to cheri version, with a comment explaining why it is done this way.

248 ↗

(On Diff #34660)

You do not need the whole server. To test, link statically with custom-build libc.

[mmel]

• malloc_aligned() modified to rtld version
• added alignment fix also for Variant I
• check malloc() return values
• use custom version of assert()

[mmel]

lib/libc/gen/tls.c

128 ↗

(On Diff #34660)

At this point, I don't want to touch another file. The CERI based fix is more appropriate for separated commit, I think.

[kib]

lib/libc/gen/tls.c

240 ↗

(On Diff #34682)

free_aligned((void *)tlsstart);

Note the spacing.

259 ↗

(On Diff #34682)

I do not quite understand this. tls is aligned, but the effective base tls address is segbase - tls_static_space == tls + size - tls_static_size == tls + roundup2(tls_static_space, tcbalign) - tls_static_size, which seems to be perfectly misaligned by the align adjustment.

What I am missing ?

[mmel]

lib/libc/gen/tls.c

240 ↗

(On Diff #34682)

Will be fixed in commit.

259 ↗

(On Diff #34682)

tls_static_size is already aligned to TLS section requirements at line 361. The line 255 is only relevant for TLS_TCB_ALIGN > tls_init_align case, hard to say if this is possible in real life.

Anyway, this code looks like subject for much bigger rewrite. But I only want to fix immediate damage caused by r324938 ASAP, as we currently distribute unbootable ARM images (/bin/init is also statically linked).