This is an archive of the discontinued LLVM Phabricator instance.

Hwasan InitPrctl check for error using internal_iserror
AcceptedPublic

Authored by mmalcomson on Jan 11 2021, 9:13 AM.

Details

Summary

When adding this function in https://reviews.llvm.org/D68794 I did not
notice that internal_prctl has the API of the syscall to prctl rather
than the API of the glibc (posix) wrapper.

This means that the error return value is not necessarily -1 and that
errno is not set by the call.

For InitPrctl this means that the checks do not catch running on a
kernel *without* the required ABI (not caught since I only tested this
function correctly enables the ABI when it exists).
This commit updates the two calls which check for an error condition to
use internal_iserror. That function sets a provided integer to an
equivalent errno value and returns a boolean to indicate success or not.

Tested by running on a kernel that has this ABI and on one that does
not. Verified that running on the kernel without this ABI the current
code prints the provided error message and does not attempt to run the
program. Verified that running on the kernel with this ABI the current
code does not print an error message and turns on the ABI.
This done on an x86 kernel (where the ABI does not exist), an AArch64
kernel without this ABI, and an AArch64 kernel with this ABI.

In order to keep running the testsuite on kernels that do not provide
this new ABI we add another option to the HWASAN_OPTIONS environment
variable, this option determines whether the library kills the process
if it fails to enable the relaxed syscall ABI or not.
This new flag is fail_without_syscall_abi.
The check-hwasan testsuite results do not change with this patch on
either x86, AArch64 without a kernel supporting this ABI, and AArch64
with a kernel supporting this ABI.

Diff Detail

Event Timeline

mmalcomson created this revision.Jan 11 2021, 9:13 AM
mmalcomson requested review of this revision.Jan 11 2021, 9:13 AM
Herald added a subscriber: Restricted Project. · View Herald TranscriptJan 11 2021, 9:13 AM
eugenis accepted this revision.Jan 11 2021, 1:10 PM

LGTM
Thanks!

This revision is now accepted and ready to land.Jan 11 2021, 1:10 PM
thakis added a subscriber: pcc.Jan 13 2021, 5:28 AM

I'll revert this for now to unbreak the bot since figuring out a way forward will likely take a few hours.

pcc set up the suite on this bot. It looks like the test suite was supposed to be runnable on "normal" machines at some point.

If it's indeed intentional that it now requires special kernels, the normal technique to deal with this is probably to have the lit init check for the feature and to add a required feature to the hwasan lit.site.cfg.py.in.

mmalcomson edited the summary of this revision. (Show Details)

Apologies for the original mistake (which was pretty bad), and the very slow response.

This update has been tested using the hwasan testsuite and manual testing on AArch64 kernels both with and without the syscall ABI, and an x86 kernel.
I did test on an older revision (on top of 302432f) since that testsuite runs on a machine without python3.6, but I believe that shouldn't cause a problem.

The difference with this revision is that I introduce an environment flag to allow running on kernels that do not have the new syscall ABI.
I used fail_without_syscall_abi, but am open to any name people would prefer.

mmalcomson reopened this revision.Feb 18 2021, 8:48 AM

Reopening with new revision since previous version has been reverted due to my mistake.
Apologies again for that lazy mistake.

This revision is now accepted and ready to land.Feb 18 2021, 8:48 AM
pcc accepted this revision.Feb 18 2021, 10:52 AM

LGTM

compiler-rt/lib/hwasan/hwasan_flags.inc
81

Nit: ).