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

x86: fix ELF R_X86_64_GOT32/R_X86_64_PLT32 reloc #910

Merged
merged 1 commit into from Aug 26, 2019

Conversation

@aroulin
Copy link
Contributor

commented Aug 15, 2019

R_X86_64_GOT32/R_X86_64_PLT32 are 32-bit entries but the ELF
RelocationHandler rewrites longs (64-bit), overwriting the following
bytes with the 64-bit sign-extension thus overwriting the next instructions.

We can demonstrate with the following example:

/* shared.c */
void function(void);
void shared_function(void)
{
        function();
        function();
}

Compile with

cc  -fPIC -c -o shared.o shared.c

and analyze with Ghidra

plt32_reloc

Rewrite int (32-bit) instead and properly relocate entries using
addend and offset fields as defined in AMD64 ABI gives:

pl32_reloc_fixed

I am not sure if this could break in other places but it sounds to me like writing 64-bit values back is wrong.

@mumbel

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

Looks like there's an issue still. going off screenshot: SUB_00201011 and SUB_00201016 should be the same based on your example

Unless those are actually two seperate wrappers of function()
not 100% on got32, but based on the plt32 this example produces, maybe:

case X86_64_ElfRelocationConstants.R_X86_64_GOT32:
	value = symbolValue + addend;
	memory.setInt(relocationAddress, (int)value);
	break;
case X86_64_ElfRelocationConstants.R_X86_64_PLT32:
	value = symbolValue + addend - offset;
	memory.setInt(relocationAddress, (int)value);
	break;			
R_X86_64_GOT32 and R_X86_64_PLT32 are 32-bit entries but the ELF
RelocationHandler would rewrite longs (64-bit), overwriting the
following bytes with the 64-bit sign-extension thus overwriting
the next instructions.

Rewrite int (32-bit) instead and properly relocate entries using
addend and offset fields as defined in AMD64 ABI.

Signed-off-by: Andy Roulin <andy.roulin@gmail.com>
@aroulin aroulin force-pushed the aroulin:plt32-reloc branch from 53f2db8 to 1cc1751 Aug 15, 2019
@aroulin

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Thanks indeed. I think the GOT32 relocation you described without the offset is good according to the AMD64 ABI.

I got confused by the comment in the code and thought that the two function calls could not be resolved yet to the same symbol. I removed the comment in the new version of the commit because I don't think it makes any sense.

Old screenshot for reference
plt32_reloc_fixed

I edited the PR description with the new result and fix.

@emteere

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

Nice catch. Would definitely affect someone trying to make FID files. Many relocations are only found in .o's and those haven't been exercised as much. Not sure about this one.

I think we need a review of the other relocations that setLong and may also be 32 bit values, for example R_X86_64_GOTPC32.

@aroulin

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

I stumbled across that bug when reversing a kernel module :)
I can look at the other cases, either in this PR or another one.

@emteere

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

I've got most of the others spec-ed out, other than the TLS related ones. There are great files that generate test files with forced relocations in them. I think we need a regression test for all the relocations, maybe based on these files.

My plan was to accept your pull request and check in some more fixes on top of it. The GOTPC32 is indeed incorrect.

@emteere

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

Have you run into any TLS symbols and/or TLS relocations?

@aroulin

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

No, I have not run into them. It would be good to check indeed.

@mumbel

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

@emteere A good resource would be llvm or gcc toolchains.

This ELF directory is huge so github browser craps out, and of course x86-64 is at the bottom that doesn't get listed, but for example:
https://github.com/llvm-mirror/lld/blob/master/test/ELF/x86-64-tls-dynamic.s
gives: R_X86_64_TLSGD and R_X86_64_TLSLD

and

https://github.com/llvm-mirror/lld/blob/master/test/ELF/x86-64-tlsdesc-gd.s
gives: R_X86_64_TLSDESC

edit: hopefully those binaries are actually usable and its not just some linker object that wouldn't do anything in elf loading

@emteere

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2019

Good pointers.

I was looking at the "gas" testsuite for the normal relocations, but these look better. The ELF import code currently doesn't process TLS symbols, either creating them, or processing relocations to them. I've experimented with some minor changes to allow the TLS symbols to go through, but I need to test it with solid simple examples.

@ryanmkurtz ryanmkurtz added this to the 9.1 milestone Aug 26, 2019
@ryanmkurtz ryanmkurtz merged commit 1cc1751 into NationalSecurityAgency:master Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.