Add kcfi_unchecked attribute

[samitolvanen]

Summary

With -fsanitize=kcfi, disabling indirect call checking using the
no_sanitize attribute or the sanitizer special case list is not
fine-grained enough in cases where we want to disable checking
only for specific calls. The kcfi_unchecked attribute applies to
functions and function pointers and allows disabling indirect calls
made using an annotated pointer.

Depends on D119296

[samitolvanen]

Note that this was split from D119296.

In the previous discussion, @joaomoreira pointed out that this is very similar to nocf_check and proposed reusing that attribute. In an offline discussion, @pcc was concerned that an attribute may not be the right approach here and suggested a __builtin_kcfi_unchecked(function(args)) built-in function to avoid changing the type system.

I would appreciate hearing your thoughts.

[aaron.ballman]

In D122673#3414650, @samitolvanen wrote:

Note that this was split from D119296.

In the previous discussion, @joaomoreira pointed out that this is very similar to nocf_check and proposed reusing that attribute. In an offline discussion, @pcc was concerned that an attribute may not be the right approach here and suggested a __builtin_kcfi_unchecked(function(args)) built-in function to avoid changing the type system.

I would appreciate hearing your thoughts.

I tend to be very wary of modifying the type system with attributes -- questions always arise of what the type system effects are of the attribute. e.g., does it impact overload sets or template specialization? Name mangling? If it doesn't have type system impacts... why does it need to be in the type system at all? This also matters because adding more bits to types can have unintended side effects like accidentally reducing the depth of template instantiations we can process (because of the extra memory pressure). So while I'm not certain what you and @pcc talked about, it does seem like an approach at least worth thinking about, especially because this patch needs to bump the size of Type.

clang/include/clang/AST/Type.h

1832

Needing to bump this limit worries me. It's correct for the changes, but it means that Type just because even more expensive and so I wonder how this will impact compile time performance.

[samitolvanen]

In D122673#3443498, @aaron.ballman wrote:

I tend to be very wary of modifying the type system with attributes -- questions always arise of what the type system effects are of the attribute. e.g., does it impact overload sets or template specialization? Name mangling? If it doesn't have type system impacts... why does it need to be in the type system at all? This also matters because adding more bits to types can have unintended side effects like accidentally reducing the depth of template instantiations we can process (because of the extra memory pressure). So while I'm not certain what you and @pcc talked about, it does seem like an approach at least worth thinking about, especially because this patch needs to bump the size of Type.

Sure, I agree. I'll take a look at the built-in approach. Do you have any thoughts about reusing nocf_check, which is essentially identical to this proposed attribute, just currently limited to x86 CET?

[aaron.ballman]

In D122673#3443518, @samitolvanen wrote:

In D122673#3443498, @aaron.ballman wrote:

I tend to be very wary of modifying the type system with attributes -- questions always arise of what the type system effects are of the attribute. e.g., does it impact overload sets or template specialization? Name mangling? If it doesn't have type system impacts... why does it need to be in the type system at all? This also matters because adding more bits to types can have unintended side effects like accidentally reducing the depth of template instantiations we can process (because of the extra memory pressure). So while I'm not certain what you and @pcc talked about, it does seem like an approach at least worth thinking about, especially because this patch needs to bump the size of Type.

Sure, I agree. I'll take a look at the built-in approach. Do you have any thoughts about reusing nocf_check, which is essentially identical to this proposed attribute, just currently limited to x86 CET?

I was thinking about that, but I don't think it's possible to reuse that (however, I must admit that I don't know the semantic details about either of these attributes very well, so I may be missing something). nocf_check is allowed on any X86 target, and kcfi_unchecked is allowed on x86-64. It seems like that target would still need a way to differentiate between the semantics because the attributes do different things. Or have I misunderstood something about the relationship between nocf_check and this attribute? (If the semantics aren't just "essentially identical" but are "actually identical" for x86-64, then this option seems more viable.)

[joaomoreira]

In the previous discussion, @joaomoreira pointed out that this is very similar to nocf_check and proposed reusing that attribute. In an offline discussion, @pcc was concerned that an attribute may not be the right approach here and suggested a __builtin_kcfi_unchecked(function(args)) built-in function to avoid changing the type system.

I'm still thinking a bit about this/needing some time to provide a proper review, but just to not hold the thoughts back since this is moving.

A consideration I can foresee with extending the type system with an attribute is that you then tie the function pointer prototype to the attribute for assignments and this will later require some casting magic if you want to invoke functions without kcfi_unchecked through a kcfi_unchecked pointer (which I assume should be doable). OTOH, it would be nice to get warnings when assigning a kcfi_unchecked function to pointers which will later be used in checked indirect calls (yet, it would be better to have explicit warning about the inconsistency instead of implicit type mismatching ones).

Regarding nocf_check, my understanding is that the kernel IBT support assumes CET/IBT to be orthogonal to KCFI -- with that said, kernel implementation is set to never use or allow no-track prefixes, meaning that the above situation never happens for IBT (since there are no nocf_check function pointers). OTOH, I assume that there could be situations where you want the function pointer call to be relaxed/coarse-grained? If yes, this would need to be done through a different attribute other than nocf_check, since this sets the compiler to emit notrack prefixes that are invalid in kernel.

With all the above, it seems to me that using a kcfi-specific builtin could be the more flexible option.

[samitolvanen]

Alright, thanks for the feedback everyone! I'll abandon this patch and look into adding a built-in function instead.