Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

musl and glibc dynamic stubs are deeply flawed #8896

Closed
LemonBoy opened this issue May 25, 2021 · 3 comments · Fixed by #10330
Closed

musl and glibc dynamic stubs are deeply flawed #8896

LemonBoy opened this issue May 25, 2021 · 3 comments · Fixed by #10330
Labels
bug Observed behavior contradicts documented or intended behavior miscompilation The compiler reports success but produces semantically incorrect code. os-linux
Milestone

Comments

@LemonBoy
Copy link
Contributor

  • glibc stubs are generated by merging together functions (marked with F in the abilist files) and variables (marked with D) and treating them as if they were functions (STT_FUNC entries). This is a huge problem when the linker performs copy relocations (eg. a static library references __stack_chk_guard using absolute addresses, even though that symbol is provided by an external shared object). Upon seeing a STT_FUNC symbol lld processes it as if it were a function, creating a PLT (!!!) entry instead of emitting a copy relocation or a GOT indirection (afaics the latter is not really used).

To understand the implications of this error here's a small snippet of ARM assembly, showing a typical function prologue where we load the stack cookie from __stack_chk_guard and store it on stack:

   3ee78: 10 4c 2d e9  	push	{r4, r10, r11, lr}
   3ee7c: 08 b0 8d e2  	add	r11, sp, #8
   3ee80: d0 d0 4d e2  	sub	sp, sp, #208
   3ee84: b0 c0 0e e3  	movw	r12, #57520
   3ee88: 07 c0 40 e3  	movt	r12, #7
   # Load from 0x7e0b0
   3ee8c: 00 c0 9c e5  	ldr	r12, [r12]
   3ee90: 0c c0 0b e5  	str	r12, [r11, #-12]

And, guess what, that address points to a PLT entry:

0007e0b0 <$a>:
   7e0b0: 00 c6 8f e2  	add	r12, pc, #0, #12
   7e0b4: 20 ca 8c e2  	add	r12, r12, #32, #20
   7e0b8: 68 f3 bc e5  	ldr	pc, [r12, #872]!

In short the program is using garbage for stack cookie and, what's worse, it's not even random data!

The fix is simple, the abilist files contain the symbol size, it's only a matter of passing this info to the .txt files used to synthesize the .s file.

  • musl stubs are a bit better, some symbols are correctly marked as objects (STT_OBJECT) but are zero-sized, making lld promptly reject those when attempting to do a copy relocation. The fix is, again, to emit .size <sym>, <size> for the symbols marked as %object.

Note that the symbol sizes may be different across platforms (eg. 32 vs 64 bit), getting the symbol size wrong is dangerous.

cc @flyx, this is the problem you were hitting.

@andrewrk andrewrk added bug Observed behavior contradicts documented or intended behavior os-linux miscompilation The compiler reports success but produces semantically incorrect code. labels Jun 8, 2021
@andrewrk andrewrk added this to the 0.8.1 milestone Jun 8, 2021
@andrewrk andrewrk modified the milestones: 0.8.1, 0.9.1 Aug 31, 2021
@andrewrk andrewrk modified the milestones: 0.9.1, 0.9.0 Nov 20, 2021
@motiejus
Copy link
Contributor

motiejus commented Nov 26, 2021

The fix is simple, the abilist files contain the symbol size

I wasn't able to locate the symbol size in the abilist files. E.g. on glibc-2.32, looking for symbol size of pthead_sigmask:

otiejus ~/code/glibc/sysdeps/unix/sysv/linux/x86_64/64 $ git grep pthread_sigmask
libc.abilist:GLIBC_2.2.5 pthread_sigmask F
libc.abilist:GLIBC_2.32 pthread_sigmask F
motiejus ~/code/glibc/sysdeps/unix/sysv/linux/x86_64/64 $ 

Can you help locate it?

@LemonBoy
Copy link
Contributor Author

Can you help locate it?

Aye, look here.

andrewrk added a commit that referenced this issue Dec 9, 2021
This is the result of the work on tools/gen_stubs.zig. It now uses the
preprocessor to emit different symbols and sizes depending on the
architecture. The data is collected directly from multiple libc.so files
on disk built with upstream musl.

Closes #8178
Addresses #8896 for musl

There is still room for further improvement to this, which is to
put `.ds` directives after symbols that are not followed by aliases, to
avoid the potential problem of a linker believing that all symbols are
aliases of each other.
@andrewrk
Copy link
Member

andrewrk commented Dec 9, 2021

musl stubs addressed in 01cb0bd.

There is still room for further improvement to this, which is to put .ds directives after symbols that are not followed by aliases, to avoid the potential problem of a linker believing that all symbols are aliases of each other. But perhaps that improvement can wait until we see evidence that it will cause a problem.

@andrewrk andrewrk changed the title glibc and musl dynamic stubs are deeply flawed glibc dynamic stubs are deeply flawed Dec 9, 2021
andrewrk added a commit that referenced this issue Dec 13, 2021
This commit upgrades glibc shared library stub-creating code to use the
new abilists file which is generated by the new glibc-abi-tool project:
https://github.com/ziglang/glibc-abi-tool/

The abilists file is different in these ways:
 * It additionally encodes whether a symbol is a function or an object,
   and if it is an object, it additionally encodes the size in bytes.
 * It additionally encodes migrations of symbols from one library to
   another between glibc versions.
 * It is binary data instead of ascii.
 * It is one file instead of three.
 * It is 165 KB instead of 200 KB.

This solves a handful of bugs.

Fixes #5882
Fixes #7667
Fixes #8714
Fixes #8896
andrewrk added a commit that referenced this issue Dec 13, 2021
This commit upgrades glibc shared library stub-creating code to use the
new abilists file which is generated by the new glibc-abi-tool project:
https://github.com/ziglang/glibc-abi-tool/

The abilists file is different in these ways:
 * It additionally encodes whether a symbol is a function or an object,
   and if it is an object, it additionally encodes the size in bytes.
 * It additionally encodes migrations of symbols from one library to
   another between glibc versions.
 * It is binary data instead of ascii.
 * It is one file instead of three.
 * It is 165 KB instead of 200 KB.

This solves three bugs:

Fixes #7667
Fixes #8714
Fixes #8896
andrewrk added a commit that referenced this issue Dec 13, 2021
This commit upgrades glibc shared library stub-creating code to use the
new abilists file which is generated by the new glibc-abi-tool project:
https://github.com/ziglang/glibc-abi-tool/

The abilists file is different in these ways:
 * It additionally encodes whether a symbol is a function or an object,
   and if it is an object, it additionally encodes the size in bytes.
 * It additionally encodes migrations of symbols from one library to
   another between glibc versions.
 * It is binary data instead of ascii.
 * It is one file instead of three.
 * It is 165 KB instead of 200 KB.

This solves three bugs:

Fixes #7667
Fixes #8714
Fixes #8896
@andrewrk andrewrk changed the title glibc dynamic stubs are deeply flawed musl and glibc dynamic stubs are deeply flawed Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Observed behavior contradicts documented or intended behavior miscompilation The compiler reports success but produces semantically incorrect code. os-linux
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants