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

[andreyknvl]

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

[glider]

clang/lib/CodeGen/CodeGenFunction.cpp

853

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]

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

[eugenis]

clang/lib/CodeGen/CodeGenFunction.cpp

853

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.

[andreyknvl]

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?

[eugenis]

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.

[dvyukov]

In D44981#1056081, @eugenis wrote:

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.

[andreyknvl]

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.

[eugenis]

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.

[andreyknvl]

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]

Let's keep adding kernel- sanitizers for now.

[andreyknvl]

Thanks! Could you commit this?

clang/lib/CodeGen/CodeGenFunction.cpp

if (CGM.isInSanitizerBlacklist(SanitizerKind::ID, Fn, Loc))                \
       SanOpts.set(SanitizerKind::ID, false);
 
 #include "clang/Basic/Sanitizers.def"
 #undef SANITIZER
   } while (0);
 
   if (D) {
     // Apply the no_sanitize* attributes to SanOpts.
-    for (auto Attr : D->specific_attrs<NoSanitizeAttr>())
-      SanOpts.Mask &= ~Attr->getMask();
+    for (auto Attr : D->specific_attrs<NoSanitizeAttr>()) {
+      SanitizerMask mask = Attr->getMask();
+      SanOpts.Mask &= ~mask;
+      if (mask & SanitizerKind::Address)
+        SanOpts.set(SanitizerKind::KernelAddress, false);
+    }
   }
 
   // Apply sanitizer attributes to the function.
   if (SanOpts.hasOneOf(SanitizerKind::Address | SanitizerKind::KernelAddress))
     Fn->addFnAttr(llvm::Attribute::SanitizeAddress);
   if (SanOpts.hasOneOf(SanitizerKind::HWAddress))
     Fn->addFnAttr(llvm::Attribute::SanitizeHWAddress);
   if (SanOpts.has(SanitizerKind::Thread))