This is an archive of the discontinued LLVM Phabricator instance.

[Sanitizers] Fix read buffer overrun in scanning loader commands

Authored by wrotki on Jan 19 2023, 3:50 PM.



The fix only affects Darwin, but to write the test I had to modify
the MemoryMappingLayout class which is used by all OSes,
to allow for mocking of image header (this change should be NFC). Hence no [Darwin] in the subject
so I can get more eyes on it.

While looking for a memory gap to put the shadow area into, the sanitizer code
scans through the loaded images, and for each image it scans through its
loader command to determine the occupied memory ranges.

While doing so, if the 'segment load' (kLCSegment) loader comand is encountered, the command scanning function
returns success (true), but does not decrement the command list iterator counter.
The result is that the function is called again and again, with the iterator counter
now being too high. The command scanner keeps updating the loader command pointer,
by using the command size field.

If the loop counter is too high, the command pointer
lands into unintended area ( beyond <header addr>+sizeof(mac_header64)+header->sizeofcmds ),
and result depends on the random content found there.

The random content interpreted as loader command might contain a large integer value in the
cmdsize field - this value is added to the current loader command pointer,
which might now point to an inaccessible memory address. It can occasionally result
in a crash if it happens to run beyond the mapped memory segment.

Note that when the area after the loader command list
contains zeros or small integers only, the loop will end normally and the problem
will go unnoticed. So it happened until now since having a some big value
after the header area, falling into command size field is a pretty rare situation.

The fix makes sure that the iterator counter gets updated when the segment load (kLCSegment)
loader command is found too, and in the same code location so the updates will always go together.

Undo the changes in the sanitizer_procmaps_mac.cpp to see the test failing.


Diff Detail

Event Timeline

wrotki created this revision.Jan 19 2023, 3:50 PM
wrotki requested review of this revision.Jan 19 2023, 3:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2023, 3:50 PM
Herald added subscribers: Restricted Project, pcwang-thead. · View Herald Transcript
vitalybuka added inline comments.Jan 19 2023, 9:05 PM

CurrentImageHeader could be protected.

thetruestblue added inline comments.Jan 20 2023, 10:51 AM

When enabling arm64 on Apple devices I added arm64 for Apple only a few lines below this.


If we want to enable unit tests for arm64 universally this is the right place for the change, but I wasn't able to test that. If we want this change here, please remove the line from the apple-specific code:

wrotki updated this revision to Diff 491974.Jan 24 2023, 5:20 PM

Apply code review feedback

  • Removed unnecessary arm64 addition - didn't notice it being added few lines below
  • Made MemoryMappingLayout::CurrentImageHeader method protected
wrotki marked an inline comment as done.Jan 24 2023, 5:24 PM
wrotki added inline comments.

Right, didn't notice this line adding arm64 just few lines below my change. It's right to only do it for Apple builds, at least for now

vitalybuka resigned from this revision.Jan 24 2023, 7:16 PM

LGTM, but as it's mostly Mac, leaving to Apple reviewers.

yln accepted this revision.Jan 27 2023, 1:18 PM

LGTM, with a few small nits. Thanks Mariusz!


I think we can push the definition of this down into MemoryMappingLayout to minimize the diff.


The opaque return type of this and the necessary casts on both sides (implementation and user) is a bit unfortunate, but I don't see a better way to do insert this abstraction here.


Let's inline this into CurrentImageHeader().


Copy & paste leftover? I think we can move #if SANITIZER_APPLE up here and drop this #if about Windows.

This revision is now accepted and ready to land.Jan 27 2023, 1:18 PM
wrotki updated this revision to Diff 494782.Feb 3 2023, 5:57 PM

Addressed the latest feedback from yln

wrotki marked 7 inline comments as done.Feb 3 2023, 6:00 PM
wrotki added inline comments.

Well, yeah, as you say it's unfortunate

This revision was landed with ongoing or failed builds.Feb 3 2023, 6:43 PM
This revision was automatically updated to reflect the committed changes.
wrotki marked an inline comment as done.
thakis added a subscriber: thakis.Feb 4 2023, 4:46 AM

It looks like this might have broken hwasan tests on Linux:

Please take a look and revert for now if it takes a while to fix.

I also bisected this as the cause of a local build failure on Linux (the same error as this buildbot:

yozhu added a subscriber: yozhu.Feb 4 2023, 9:03 AM

Our testing also failed due to the same linker error as seen in the buildbot for this diff:

ld.lld: error: undefined symbol: operator delete(void*)
referenced by sanitizer_procmaps_common.cpp.o:(__sanitizer::MemoryMappingLayout::~MemoryMappingLayout())

yln added a comment.Feb 6 2023, 7:41 PM

Thanks everyone for pointing out the build failures on Linux and reverting the change.

I have a hunch that the failure is related to making the MemoryMappingLayout destructor virtual (and the class non-final).

ld.lld: error: undefined symbol: operator delete(void*)
referenced by sanitizer_procmaps_common.cpp.o:(__sanitizer::MemoryMappingLayout::~MemoryMappingLayout())

Also please make sure you are available to deal with potential fallout when you land changes; especially this part of compiler-rt is very brittle and prone to fallout (so landing changes on Friday afternoon is a risky call).

wrotki reopened this revision.EditedFeb 7 2023, 2:54 PM

Thanks for reverting. Julian is right, the failure was caused by the virtual keyword being added to the destructor.

I'm going to re-land this with the destructor made non-virtual, and the warning shown for it by the macOS build disabled. I tested it by running ninja check-sanitizer on linux , which was failing with the virtual dtor.

This revision is now accepted and ready to land.Feb 7 2023, 2:54 PM