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