Page MenuHomePhabricator

asan: kernel: make no_sanitize("address") attribute work with -fsanitize=kernel-address
ClosedPublic

Authored by andreyknvl on Mar 28 2018, 8:57 AM.

Details

Summary

Right now to disable -fsanitize=kernel-address instrumentation, one needs to use no_sanitize("kernel-address"). Make either no_sanitize("address") or no_sanitize("kernel-address") disable both ASan and KASan instrumentation. Also remove redundant test.

Patch by Andrey Konovalov

Diff Detail

Repository
rL LLVM

Event Timeline

andreyknvl created this revision.Mar 28 2018, 8:57 AM
andreyknvl edited the summary of this revision. (Show Details)
glider added inline comments.Mar 28 2018, 9:34 AM
clang/lib/CodeGen/CodeGenFunction.cpp
853 ↗(On Diff #140085)

I think this should work the other way round as well, i.e. `attribute((no_sanitize("kernel-address"))) should also disable ASan.
I hope nobody is gonna use that, but it's easier to treat the attributes as synonyms than to guess which of them implies the other one.
What do others think?

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

I think it makes sense to make both no_sanitize("address") and no_sanitize("kernel-address") work. Done that.

eugenis added inline comments.Mar 29 2018, 11:36 AM
clang/lib/CodeGen/CodeGenFunction.cpp
853 ↗(On Diff #140085)

It sounds like you want to treat kernel_address as an alias for address in no_sanitize("") attribute. I don't have a strong opinion about that, but the implementation could be moved to SemaDeclAttr.cpp.

I don't see an easy way to fix this in SemaDeclAttr.cpp. The way that I see is to change std::vector<StringRef> Sanitizers to something like std::set, then .insert("kernel-address") when we see "address" and vice versa, and then convert back to vector before passing to D->addAttr. What do you think?

You are right, it's not the best idea.
Is it too late to get rid of C/C++-level "kernel-address" attribute entirely? For the source code perspective, it's just ASan, but for the kernel. If that's an important distinction, one can always do #ifdef KERNEL or something.

You are right, it's not the best idea.
Is it too late to get rid of C/C++-level "kernel-address" attribute entirely? For the source code perspective, it's just ASan, but for the kernel. If that's an important distinction, one can always do #ifdef KERNEL or something.

I don't these attributes are used in kernel.
If this does not affect gcc, then this is even safer because there are 2 or 3 people who use KASAN with clang.

The -fsanitize=kernel-address flag is used by the kernel and we should leave it. The no_sanitize("kernel-address") attribute is not used in the kernel, so it's totally OK to get rid of it. What kind of changes would that require? AFAIU clang/include/clang/Basic/Sanitizers.def is used to generate both, the flag and the no_sanitize attribute.

I take that back, it's natural to expect there to be a no_sanitize("") attribute matching each possible -fsanitize= value.

I'm worried about the spread of kernel-* sanitizers. "kernel" is an orthogonal property, plus Linux is only one of the many existing kernels. Ideally the sanitizers should be named the same as in userspace, and kernel-specific behaviour would be triggered by the target triple or -mkernel flag or something like that.

I don't have any preference between -fsanitize=kernel-address and -fsanitize=address -mllvm asan-kernel=1. GCC uses -fsanitize=kernel-address and I think it looked quite logical to use the same in Clang.

I doubt there'll be any difference in the instrumentation pass between the Linux kernel and say the FreeBSD kernel (I'm not sure though), so kernel in the name makes sense.

So what should I do here?

eugenis accepted this revision.Apr 6 2018, 9:39 AM

Let's keep adding kernel- sanitizers for now.

This revision is now accepted and ready to land.Apr 6 2018, 9:39 AM

Thanks! Could you commit this?

vitalybuka accepted this revision.Apr 9 2018, 1:01 PM
vitalybuka edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.