This is an archive of the discontinued LLVM Phabricator instance.

Fix overstrict validation of Mach-O rebase opcode
AbandonedPublic

Authored by kastiglione on Jun 17 2017, 4:24 PM.

Details

Summary

When running llvm-objdump -macho -rebase on a binary that I presume to be
fully valid (published in the app store), the output was:

llvm-objdump: '/path/to/Some.app/Some': truncated or malformed object (for REBASE_OPCODE_ADD_ADDR_IMM_SCALED bad segOffset, too large for opcode at: 0x123)

The state and sequence is:

  1. Handling REBASE_OPCODE_ADD_ADDR_IMM_SCALED
  2. Precondition: RebaseEntryCheckSegAndOffset
  3. The opcode is applied and SegmentOffset is incremented accordingly
  4. Postcondition RebaseEntryCheckSegAndOffset

The precondition in step 2 fails. The reason is the index,offset pair points
exactly to the end of a section, but the check is called with endInvalid = true
which makes the check reject offsets pointing to the end of a section. Given
that this check happens before applying the opcode, and the opcode
produces a valid offset that passes the check in step 4, it seems erroneous
to fail the precondition.

This fix is to pass false for endInvalid in the precondition in step 2.

Event Timeline

kastiglione created this revision.Jun 17 2017, 4:24 PM
kastiglione retitled this revision from Less strict validation of Mach-O rebase opcode to Fix overstrict validation of Mach-O rebase opcode.Jun 17 2017, 5:15 PM
kastiglione edited the summary of this revision. (Show Details)Jun 18 2017, 6:13 AM
kastiglione planned changes to this revision.Jun 19 2017, 11:12 AM

Will follow @compnerd's feedback and produce some tests.

lib/Object/MachOObjectFile.cpp
2890

Note that this is the only check that with endInvalid = false. Why is it valid for the offset to point to the end of a section here and only here? Could it be that the precondition and postcondition of this opcode are flipped? In other words, why is it valid for SegmentOffset to point to the end of a section after applying the opcode, but not before applying the opcode? Shouldn't it be the other way around?

kastiglione added inline comments.Jun 22 2017, 10:17 PM
lib/Object/MachOObjectFile.cpp
3738–3740

Note this endInvalid = false.

A REBASE_OPCODE_DO_*_TIMES* opcode is processed and leaves SegmentOffset pointing to the end of a section. This call to checkSegAndOffset allows it because of the endInvalid = false.

But, if the subsequent opcode is REBASE_OPCODE_ADD_ADDR_IMM_SCALED, then it will error on its precondition because it calls checkSegAndOffset with endInvalid = true.

kastiglione abandoned this revision.Jun 13 2018, 9:46 PM