Re: v2 [PATCH 0/X] Introduce HWASAN sanitizer to GCC

[Matthew Malcomson <Matthew dot Malcomson at arm dot com>]

On 20/11/2019 14:29, Martin Liška wrote:
> On 11/7/19 7:37 PM, Matthew Malcomson wrote:
>> I have rebased this series onto Martin Liska's patches that take the most
>> recent libhwasan from upstream LLVM.
>> https://gcc.gnu.org/ml/gcc-patches/2019-11/msg00340.html
>>
>> I've also cleared up some nomenclature (I had previously used the word 
>> 'colour'
>> a few times instead of the word 'tag' and that clashes with other 
>> descriptions)
>> and based the patch series off a more recent GCC revision (r277678).
>>
>> There's an ongoing discussion on whether to have 
>> __SANITIZER_ADDRESS__, or
>> __SANITIZER_HWADDRESS__, but I'm keeping that discussion to the existing
>> thread.
>>
>> Similarly there's still the question around C++ exceptions that I'm 
>> keeping to
>> the existing thread (on the first patch series).
>>
>>
>> NOTE:
>>    Unfortunately, there's a bug in the more recent version of GCC I 
>> rebased
>>    onto.
>>    Hwasan catches this when bootstrapping, which means bootstrapping 
>> with hwasan
>>    fails.
>>    I'm working on tracking the bug down now, but sending this series 
>> upstream
>>    for visibility while that happens.
>>
>>    Bugzilla link:
>>    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92410
>>
>> Entire patch series attached to cover letter.=
>>
> 
> Hello.
> 
> Thank you very much for the patch set, I've just wrote some inline replies
> and I have also some questions/suggestions that I'll summarize here. One 
> caveat,
> I'm not maintainer in any of the ideas and I must confirm that I haven't 
> made
> a deep review of the part which relates to new RTL hooks and stack 
> expansion.
> I expect Jakub can help us here.
> 
> Questions/notes:
> a) For ASAN we do have bunch of parameters:
> 
> gcc --help=param | grep asan
>    asan-stack                  Enable asan stack protection.
>    asan-instrument-allocas     Enable asan allocas/VLAs protection.
>    asan-globals                Enable asan globals protection.
>    asan-instrument-writes      Enable asan store operations protection.
>    asan-instrument-reads       Enable asan load operations protection.
>    asan-memintrin              Enable asan builtin functions protection.
>    asan-use-after-return       Enable asan detection of use-after-return 
> bugs.
>    asan-instrumentation-with-call-threshold Use callbacks instead of 
> inline code if number of accesses in function becomes greater or equal 
> to this number.
> 
> We can probably use some of these in order to drive hwaddress in a 
> similar way. Most of them makes sense for hwaddress as well


I would prefer to keep the options different but would be happy to share 
them if others want.

I figure that there will be some parameters that make sense for hwasan 
but not for asan.
For example hwasan-random-frame-tag doesn't have any analogue for asan.
Re-using `asan-stack` for hwasan would mean we would then need to decide 
between having options with either `hwasan` or `asan` prefix being able 
to affect hwasan, or introducing a parameter `asan-random-frame-tag` 
that has no meaning on asan.


> 
> b) Apparently clang prints also value of all registers. Do we want to do 
> the same?
> 

I believe this only happens on clang for inline checking (try testing 
clang using the parameter `-mllvm --hwasan-instrument-with-calls` to see 
without).

This would be nice to have, but I'm not planning on looking at it any 
time soon.
This is currently implemented in clang on top of two not-yet implemented 
features: Inline instrumentation, and generated checking functions with 
special calling conventions.

It's certainly not something I'm aiming to get into GCC 10.


> 
> c) I'm a bit confused by the pointer tags, where clang does:
> 

Yeah -- I left out a description of short-tags.
Hopefully my explanation in the below email helped.
https://gcc.gnu.org/ml/gcc-patches/2019-11/msg01968.html

> 
> d) Are you planning to come up with inline stack variable 
> tagging/untagging for GCC 10?

To make sure I understand the question correctly:
I think you're asking about writing tags into the shadow memory.

I wasn't planning on this.

What I've posted has all the functionality I'm aiming to get in.
My stretch goal at the moment is to handle exceptions (where I currently 
have a fundamental problem I'm trying to resolve).


> e) In hwasan_increment_tag, shouldn't you skip HWASAN_STACK_BACKGROUND 
> color?

Hopefully answered in 
https://gcc.gnu.org/ml/gcc-patches/2019-11/msg01968.html

> f) I would rename ASAN_MARK in tree dumps to HWASAN_MARK, similarly to 
> what you did
>     for HWASAN_CHECK?

This is an artifact of a shortcut I made (and tried but probably failed 
to explain well in 
https://gcc.gnu.org/ml/gcc-patches/2019-09/msg00392.html).

For HWASAN_CHECK I introduced a new internal function that takes the 
same arguments as ASAN_CHECK, hence this new function is printed 
differently.


For HWASAN_MARK, I used a trick to avoid adding "almost duplicate" code 
everywhere in the mid-end that checked for ASAN_MARK.
Then, in the sanopt pass (i.e. later on in compilation) I turn that 
ASAN_MARK internal function into a HWASAN_MARK internal function.
This happens after all passes that check for ASAN_MARK so it should be a 
problem .


Otherwise, everywhere that a function handles the case of coming across 
ASAN_MARK, I would need to account for HWASAN_MARK by doing essentially 
the same thing and I thought that would be a bit of a pain.

I would be happy to use the more explicit way of having two seperate 
functions -- not certain which I'd prefer myself.


> g) I noticed quite some GNU coding style violations: 'if (! my_cond)', 
> should be 'if (!my_cond)',
>     but it's a nit.

Will take a look.

> h) I maybe found a bug for:

Hopefully answered in 
https://gcc.gnu.org/ml/gcc-patches/2019-11/msg01968.html

> i) As mentioned, we would appreciate a comment before each newly added 
> function :)

Will do ;-)

> 
> As said, thanks for working on that.
> Martin