Page MenuHomeFreeBSD

Bug 218861 - libelf elf_update fails when adding sections
Needs ReviewPublic

Authored by eric_metricspace.net on Apr 24 2017, 11:21 PM.

Details

Reviewers
kaiw
Summary

The libelf file in base fails to perform elf_update when growing the file by adding new sections, because elf_getdata erroneously checks section boundaries against the old file size.

This can be fixed by omitting this check when doing elf_update.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

eric_metricspace.net updated this revision to Diff 27706.
emaste added a subscriber: emaste.

Do you have a straightforward testcase that can be used to reproduce the issue (and the one in D10486)?

emaste added a subscriber: cem.Aug 1 2018, 4:41 PM
emaste added inline comments.
contrib/elftoolchain/libelf/elf_data.c
32

alpha order

emaste added a comment.Aug 1 2018, 6:17 PM

Awaiting review; I pointed out a couple of minor nits to address on commit

contrib/elftoolchain/libelf/_libelf.h
234–235

alpha order, no space between * and func

cem added a comment.Aug 1 2018, 7:31 PM

I'm not sure why whatever updates the s_shdr doesn't also update e_rawsize to maintain the invariant.

The rawdata change seems spurious. It isn't used, as far as I can tell.

I don't see any other issues with the implementation here, other than that we're removing an invariant in the one case.

contrib/elftoolchain/libelf/_libelf.h
32–34

Usually sys headers come before standard C ones

234–235

Space after * (return type) does not match existing file style

eric_metricspace.net marked 4 inline comments as done.
cem added a comment.Aug 23 2018, 3:44 PM

The changes from the former revision look good, modulo one nitpick/question below. My top-level questions still stand, though:

  • Why not have updates to s_shdr update e_rawsize to maintain the invariant, instead of this?
  • The change to elf_rawdata() seems spurious and unused. Is it? If you have plans to use it in a later patch, just let me know. But if there aren't plans, I don't see changing it as useful.
contrib/elftoolchain/libelf/_libelf.h
228

Hard to tell from Phab, but it looks like other declarations have a tab between the underlying type and the * or function name, and the new ones may not? If they do, ignore this.

eric_metricspace.net updated this revision to Diff 47197.

OK, I reverted the rawdata change.

I'm not sure what you mean by the invariant. All this patch does is detect a violation and report an error in that case.

From @jkoshy_users.sourceforge.net mail,

Per my understanding, the ELF(3) API requires users to run elf_update(..., ELF_C_NULL) after making structural changes to the ELF object's contents (e.g., after adding a new section, moving sections around, etc).
It isn't clear if the behavior in the bug was caused by incorrect use of the API -- in which case we would need to improve our documentation -- or whether this was a genuine bug in libelf. A small test case would help.

cem added a comment.EditedSep 1 2018, 3:43 AM

I'm not sure what you mean by the invariant. All this patch does is detect a violation and report an error in that case.

This change relaxes the error condition sh_offset > e->e_rawsize || sh_size > e->e_rawsize - sh_offset to only apply if !updating. Could we instead correct e_rawsize when we are updating?

I still don't understand what this change is attempting to do.

The proposed changes seem to handle the case where:

  1. the ELF object had been opened in modes ELF_C_READ or ELF_C_RDWR,
  2. a new section was added, with extents set to fall outside the ELF object,
  3. but no Elf_Data descriptors were associated with this new section (i.e., elf_newdata() was not called).

However, this would be incorrect usage of the ELF(3) API: if a new section is added to an Elf descriptor and if the section falls outside the current object, then elf_newdata() would need to be called on the section to actually associate data with the new byte range.

It could be that I have misunderstood this patch. A short, correct, compilable example (sscce.org) would help me understand the issue better.