Re: [PATCH v5 09/15] x86: Use an opaque type for functions not callable from C

[Sami Tolvanen]

On Thu, Oct 14, 2021 at 10:31 AM Borislav Petkov <bp@xxxxxxxxx> wrote:
>
> On Thu, Oct 14, 2021 at 09:07:57AM -0700, Kees Cook wrote:
> > I don't think it's a super common thing to add, so in this case, yes,
> > I think doing it on a case-by-case basis will be fine.
>
> You don't have a choice - if there's no automation which verifies
> whether all the CFI annotation needed is there, people won't know what
> to wrap in what macro.
>
> > I'd _much_ prefer keeping the macro, as it explains what's going on,
> > which doesn't require a comment at every "extern const u8 foo[]" usage.
>
> You don't have to - it is just an extern.
>
> > It serves as an annotation, etc.
>
> Oh, that I figured.
>
> > And, there's been a lot of discussion on the best way to do this, what
> > to name it, etc.
>
> Looking at the changelog, DECLARE_ASM_FUNC_SYMBOL, makes a lot more
> sense to me even if it doesn't specify the aspect that it is not called
> by C but who cares - it is generic enough.
>
> > This looks to be the best option currently.
>
> Maybe because wrapping some random symbols in a obfuscating macro to
> make the next tool happy, is simply the wrong thing to do. I know, I
> know, clang CFI needs it because of magical reason X but that doesn't
> make it any better. Someday soon we'll have to write a tutorial for
> people submitting kernel patches explaining what annotation to add where
> and why.
>
> Why can't clang be taught to ignore those symbols:
>
> clang -fsanitize=cfi -fsanitize-cfi-ignore-symbols=<list>
>
> ?
>
> Hmm, looking at https://clang.llvm.org/docs/ControlFlowIntegrity.html
>
> there *is* an ignore list:
>
> "Ignorelist
>
> A Sanitizer special case list can be used to relax CFI checks for
> certain source files, functions and types using the src, fun and type
> entity types. Specific CFI modes can be be specified using [section]
> headers.
>
> ...
>
> # Ignore all functions with names containing MyFooBar.
> fun:*MyFooBar*
> ..."
>
>
> So why aren't we doing that instead of those macros?

The ignorelist can be used to disable CFI checking in the listed
functions, but the compiler still checks indirect calls made to these
functions from elsewhere, and therefore, using an opaque type is still
necessary to ensure the symbol address isn't replaced with a jump
table address.

Anyway, I thought using a macro would make the code easier to
understand. I'm happy to rename it to something that makes more sense,
but also fine with switching to a simple extern u8[] if that's
preferable.

Sami