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

fix crash when write GOT if library was built with "-z relro" but not… #10

Merged
merged 1 commit into from
Feb 3, 2019

Conversation

tntljc
Copy link

@tntljc tntljc commented Feb 1, 2019

… "-z now"

  • library could be built in gcc only with "-Wl,-z,relro" without "-Wl,-z,now", so that the library has RELRO enabled but the "BIND_NOW" won't present in the .dyn section. plthook should handle this usage case.

@kubo
Copy link
Owner

kubo commented Feb 2, 2019

@tntljc Thanks!
Could you post more detailed information?

  1. OS name and version
  2. C compiler name and version
  3. Test case

It works without your patch. See this test results about 7 months ago.

@kubo
Copy link
Owner

kubo commented Feb 2, 2019

FYI. The test failure in this pull request will be fixed by this commit.

@tntljc
Copy link
Author

tntljc commented Feb 2, 2019

Hi @kubo, the crash was originally found on Arch Linux but can be reproduced on Ubuntu 16.04 as well. The gcc version is 8.1. I can't write a test for now at home as I don't have Linux environment.

Thanks for pointing out the partial RELRO test case for me, but in my case there is another gcc option "-fno-plt" (sorry I didn't mention it in the first place), so it's actually a real conner case as it's partial RELRO but the function relocation goes through the .got which is read-only.

Put things together:

  • partial RELRO
    function call relocation uses .got.plt (writable), no crash.
  • partial RELRO + -fno-plt
    function call relocation uses .got (read-only), crash.

@kubo
Copy link
Owner

kubo commented Feb 2, 2019

Thanks for the information.

I confirmed that tests fail with -Wl,-z,relro and -fno-plt options on Ubuntu 16.04 and gcc-8 provided by this PPA. However the cause of the failure is different. plthook could not find the specified function. After I fixed it by checking both .rela.plt and .rela.dyn, tests failed with crash as you wrote. I'll merge your pull request after I setup tests on travis-ci.

@tntljc
Copy link
Author

tntljc commented Feb 2, 2019

Thanks, could you descripbe more specifically on the first crash you saw with partial RELRO and no-plt? I’m using plthook in my project and maybe I’ll encounter the same on in the future. Will you also submit the fix?

@kubo
Copy link
Owner

kubo commented Feb 3, 2019

Thanks, could you describe more specifically on the first crash you saw with partial RELRO and no-plt?

When an executable is compiled with partial RELRO and no-plt, __libc_start_main is in .rela.plt and all other relocation symbols are in .rela.dyn. So both .rela.plt and .rela.dyn must be checked to find a relocation entry. However plthook doesn't check .rela.dyn when .rela.plt is found.

plthook/plthook_elf.c

Lines 662 to 668 in 3bbafe3

/* get .rela.plt or .rel.plt section */
dyn = find_dyn_by_tag(lmap->l_ld, DT_JMPREL);
plthook.r_type = R_JUMP_SLOT;
#ifdef PLT_DT_REL
if (dyn == NULL) {
/* get .rela.dyn or .rel.dyn section */
dyn = find_dyn_by_tag(lmap->l_ld, PLT_DT_REL);

When a library is compiled with partial RELRO and no-plt, all relocation symbols are in .rela.dyn. There is no .rela.plt. So you didn't hit the first issue I saw.

I checked above by using readelf --relocs filename.

Will you also submit the fix?

Yes.

@kubo kubo merged commit 6aadee8 into kubo:master Feb 3, 2019
@kubo
Copy link
Owner

kubo commented Feb 3, 2019

@tntljc The pull request was merged. Thanks for your contribution.
Tests passed. See https://travis-ci.org/kubo/plthook/jobs/488108396#L833-L843

@tntljc
Copy link
Author

tntljc commented Feb 4, 2019

Thanks for reviewing and merging my change! BTW, I built my library with -fno-plt and -O2 so the binary has no .rela.plt. that’s why I can’t see your first crash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants