| Message ID | 20211013181658.1020262-4-samitolvanen@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | x86: Add support for Clang CFI | expand |
On Wed, Oct 13, 2021 at 11:16:46AM -0700, Sami Tolvanen wrote: > The kernel has several assembly functions, which are not directly > callable from C but need to be referred to from C code. This change adds > the DECLARE_NOT_CALLED_FROM_C macro, which allows us to declare these > symbols using an opaque type, which makes misuse harder, and avoids the > need to annotate references to the functions for Clang's Control-Flow > Integrity (CFI). > > Suggested-by: Andy Lutomirski <luto@amacapital.net> > Suggested-by: Steven Rostedt <rostedt@goodmis.org> > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> I like this; I have a sense CFI won't stay the only user of this annotation. Reviewed-by: Kees Cook <keescook@chromium.org>
On Wed, Oct 13, 2021, at 11:16 AM, Sami Tolvanen wrote: > The kernel has several assembly functions, which are not directly > callable from C but need to be referred to from C code. This change adds > the DECLARE_NOT_CALLED_FROM_C macro, which allows us to declare these > symbols using an opaque type, which makes misuse harder, and avoids the > need to annotate references to the functions for Clang's Control-Flow > Integrity (CFI). > > Suggested-by: Andy Lutomirski <luto@amacapital.net> > Suggested-by: Steven Rostedt <rostedt@goodmis.org> > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > Tested-by: Nick Desaulniers <ndesaulniers@google.com> > Tested-by: Sedat Dilek <sedat.dilek@gmail.com> > --- > include/linux/linkage.h | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/include/linux/linkage.h b/include/linux/linkage.h > index dbf8506decca..f982d5f550ac 100644 > --- a/include/linux/linkage.h > +++ b/include/linux/linkage.h > @@ -48,6 +48,19 @@ > #define __PAGE_ALIGNED_DATA .section ".data..page_aligned", "aw" > #define __PAGE_ALIGNED_BSS .section ".bss..page_aligned", "aw" > > +/* > + * Declares a function not callable from C using an opaque type. Defined as > + * an array to allow the address of the symbol to be taken without '&'. > + */ I’m not convinced that taking the address without using & is a laudable goal. The magical arrays-are-pointers-too behavior of C is a mistake, not a delightful simplification. > +#ifndef DECLARE_NOT_CALLED_FROM_C > +#define DECLARE_NOT_CALLED_FROM_C(sym) \ > + extern const u8 sym[] > +#endif The relevant property of these symbols isn’t that they’re not called from C. The relevant thing is that they are just and not objects of a type that the programmer cares to tell the compiler about. (Or that the compiler understands, for that matter. On a system with XO memory or if they’re in a funny section, dereferencing them may fail.) So I think we should use incomplete structs, which can’t be dereferenced and will therefore be less error prone.
On Thu, Oct 14, 2021 at 7:51 PM Andy Lutomirski <luto@kernel.org> wrote: > > > > On Wed, Oct 13, 2021, at 11:16 AM, Sami Tolvanen wrote: > > The kernel has several assembly functions, which are not directly > > callable from C but need to be referred to from C code. This change adds > > the DECLARE_NOT_CALLED_FROM_C macro, which allows us to declare these > > symbols using an opaque type, which makes misuse harder, and avoids the > > need to annotate references to the functions for Clang's Control-Flow > > Integrity (CFI). > > > > Suggested-by: Andy Lutomirski <luto@amacapital.net> > > Suggested-by: Steven Rostedt <rostedt@goodmis.org> > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > Tested-by: Nick Desaulniers <ndesaulniers@google.com> > > Tested-by: Sedat Dilek <sedat.dilek@gmail.com> > > --- > > include/linux/linkage.h | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/include/linux/linkage.h b/include/linux/linkage.h > > index dbf8506decca..f982d5f550ac 100644 > > --- a/include/linux/linkage.h > > +++ b/include/linux/linkage.h > > @@ -48,6 +48,19 @@ > > #define __PAGE_ALIGNED_DATA .section ".data..page_aligned", "aw" > > #define __PAGE_ALIGNED_BSS .section ".bss..page_aligned", "aw" > > > > +/* > > + * Declares a function not callable from C using an opaque type. Defined as > > + * an array to allow the address of the symbol to be taken without '&'. > > + */ > > I’m not convinced that taking the address without using & is a laudable goal. The magical arrays-are-pointers-too behavior of C is a mistake, not a delightful simplification. Sure, if everyone is fine with the extra churn of adding & to the symbol references, I can certainly switch to an incomplete struct here instead. I stayed with extern const u8[] because you didn't comment on the potential churn previously: https://lore.kernel.org/lkml/CABCJKud4auwY50rO0UzH721eRyyvJ8+43+Xt9vcLgw-SMYtHEw@mail.gmail.com/ Boris, any thoughts about using an incomplete struct instead of extern u8[]? > > +#ifndef DECLARE_NOT_CALLED_FROM_C > > +#define DECLARE_NOT_CALLED_FROM_C(sym) \ > > + extern const u8 sym[] > > +#endif > > The relevant property of these symbols isn’t that they’re not called from C. The relevant thing is that they are just and not objects of a type that the programmer cares to tell the compiler about. (Or that the compiler understands, for that matter. On a system with XO memory or if they’re in a funny section, dereferencing them may fail.) Makes sense. It sounds like the macro should be renamed then. Josh wasn't a fan of DECLARE_ASM_FUNC_SYMBOL() and if I understand correctly, you and Boris feel like DECLARE_NOT_CALLED_FROM_C() is not quite right either. Suggestions? > So I think we should use incomplete structs, which can’t be dereferenced and will therefore be less error prone. Another option is to just drop the macro and use something like struct opaque_symbol instead, as you suggested earlier: https://lore.kernel.org/lkml/CALCETrVEhL9N_DEM8=rbAzp8Nb2pDitRCZGRAVcE288MBrJ4ug@mail.gmail.com/ Any preferences? I wouldn't mind having a consensus on the approach and naming before sending v6. :) Sami
On Thu, Oct 14 2021 at 19:51, Andy Lutomirski wrote: > On Wed, Oct 13, 2021, at 11:16 AM, Sami Tolvanen wrote: >> >> +/* >> + * Declares a function not callable from C using an opaque type. Defined as >> + * an array to allow the address of the symbol to be taken without '&'. >> + */ > I’m not convinced that taking the address without using & is a > laudable goal. The magical arrays-are-pointers-too behavior of C is a > mistake, not a delightful simplification. >> +#ifndef DECLARE_NOT_CALLED_FROM_C >> +#define DECLARE_NOT_CALLED_FROM_C(sym) \ >> + extern const u8 sym[] >> +#endif > > The relevant property of these symbols isn’t that they’re not called > from C. The relevant thing is that they are just and not objects of a > type that the programmer cares to tell the compiler about. (Or that > the compiler understands, for that matter. On a system with XO memory > or if they’re in a funny section, dereferencing them may fail.) I agree. > So I think we should use incomplete structs, which can’t be > dereferenced and will therefore be less error prone. While being late to that bike shed painting party, I really have to ask the question _why_ can't the compiler provide an annotation for these kind of things which: 1) Make the build fail when invoked directly 2) Tell CFI that this is _NOT_ something it can understand -void clear_page_erms(void *page); +void __bikeshedme clear_page_erms(void *page); That still tells me: 1) This is a function 2) It has a regular argument which is expected to be in RDI which even allows to do analyis of e.g. the alternative call which invokes that function. DECLARE_NOT_CALLED_FROM_C(clear_page_erms); loses these properties and IMO it's a tasteless hack. Thanks, tglx
On Fri, Oct 15, 2021, at 8:55 AM, Thomas Gleixner wrote: > On Thu, Oct 14 2021 at 19:51, Andy Lutomirski wrote: >> On Wed, Oct 13, 2021, at 11:16 AM, Sami Tolvanen wrote: >>> >>> +/* >>> + * Declares a function not callable from C using an opaque type. Defined as >>> + * an array to allow the address of the symbol to be taken without '&'. >>> + */ >> I’m not convinced that taking the address without using & is a >> laudable goal. The magical arrays-are-pointers-too behavior of C is a >> mistake, not a delightful simplification. > >>> +#ifndef DECLARE_NOT_CALLED_FROM_C >>> +#define DECLARE_NOT_CALLED_FROM_C(sym) \ >>> + extern const u8 sym[] >>> +#endif >> > >> The relevant property of these symbols isn’t that they’re not called >> from C. The relevant thing is that they are just and not objects of a >> type that the programmer cares to tell the compiler about. (Or that >> the compiler understands, for that matter. On a system with XO memory >> or if they’re in a funny section, dereferencing them may fail.) > > I agree. > >> So I think we should use incomplete structs, which can’t be >> dereferenced and will therefore be less error prone. > > While being late to that bike shed painting party, I really have to ask > the question _why_ can't the compiler provide an annotation for these > kind of things which: > > 1) Make the build fail when invoked directly > > 2) Tell CFI that this is _NOT_ something it can understand > > -void clear_page_erms(void *page); > +void __bikeshedme clear_page_erms(void *page); > > That still tells me: > > 1) This is a function > > 2) It has a regular argument which is expected to be in RDI > > which even allows to do analyis of e.g. the alternative call which > invokes that function. > > DECLARE_NOT_CALLED_FROM_C(clear_page_erms); > > loses these properties and IMO it's a tasteless hack. > Ah, but clear_page_erms is a different beast entirely as compared to, say, the syscall entry. It *is* a C function. So I see two ways to handle it: 1. Make it completely opaque. Tglx doesn’t like it, and I agree, but it would *work*. 2. Make it a correctly typed function. In clang CFI land, this may or may not be “canonical” (or non canonical?). I think #2 is far better. I complained about this quite a few versions ago, and, sorry, the word “canonical” is pretty much a non-starter. There needs to be a way to annotate a function pointer type and an extern function declaration that says “the callee follows the ABI *without CFI*” and the compiler needs to do the right thing. And whatever attribute or keyword gets used needs to give the reader at least some chance of understanding. (If there is a technical reason why function *pointers* of this type can’t be called, perhaps involving IBT, so be it. But the type system should really be aware of C-ABI functions that come from outside the CFI world.) It looks like clear_page might be improved by using static_call some day, and then proper typing will be a requirement. Would it help if I file a clang bug about this?
On Fri, Oct 15, 2021 at 9:22 AM Andy Lutomirski <luto@kernel.org> wrote: > > > > On Fri, Oct 15, 2021, at 8:55 AM, Thomas Gleixner wrote: > > On Thu, Oct 14 2021 at 19:51, Andy Lutomirski wrote: > >> On Wed, Oct 13, 2021, at 11:16 AM, Sami Tolvanen wrote: > >>> > >>> +/* > >>> + * Declares a function not callable from C using an opaque type. Defined as > >>> + * an array to allow the address of the symbol to be taken without '&'. > >>> + */ > >> I’m not convinced that taking the address without using & is a > >> laudable goal. The magical arrays-are-pointers-too behavior of C is a > >> mistake, not a delightful simplification. > > > >>> +#ifndef DECLARE_NOT_CALLED_FROM_C > >>> +#define DECLARE_NOT_CALLED_FROM_C(sym) \ > >>> + extern const u8 sym[] > >>> +#endif > >> > > > >> The relevant property of these symbols isn’t that they’re not called > >> from C. The relevant thing is that they are just and not objects of a > >> type that the programmer cares to tell the compiler about. (Or that > >> the compiler understands, for that matter. On a system with XO memory > >> or if they’re in a funny section, dereferencing them may fail.) > > > > I agree. > > > >> So I think we should use incomplete structs, which can’t be > >> dereferenced and will therefore be less error prone. > > > > While being late to that bike shed painting party, I really have to ask > > the question _why_ can't the compiler provide an annotation for these > > kind of things which: > > > > 1) Make the build fail when invoked directly > > > > 2) Tell CFI that this is _NOT_ something it can understand > > > > -void clear_page_erms(void *page); > > +void __bikeshedme clear_page_erms(void *page); > > > > That still tells me: > > > > 1) This is a function > > > > 2) It has a regular argument which is expected to be in RDI > > > > which even allows to do analyis of e.g. the alternative call which > > invokes that function. > > > > DECLARE_NOT_CALLED_FROM_C(clear_page_erms); > > > > loses these properties and IMO it's a tasteless hack. > > > > > Ah, but clear_page_erms is a different beast entirely as compared to, say, the syscall entry. It *is* a C function. So I see two ways to handle it: > > 1. Make it completely opaque. Tglx doesn’t like it, and I agree, but it would *work*. > > 2. Make it a correctly typed function. In clang CFI land, this may or may not be “canonical” (or non canonical?). Technically speaking the clear_page_* declarations don't need to be changed for CFI, they do work fine as is, but I included them in the patch as they're not actually called from C code right now. But you're right, we should use a proper function declarations for these. I'll drop the changes to this file in the next version. I wouldn't mind having a consensus on how to deal with exception handlers etc. though. Should I still use opaque types for those? Sami
On Fri, Oct 15, 2021, at 9:47 AM, Sami Tolvanen wrote: > On Fri, Oct 15, 2021 at 9:22 AM Andy Lutomirski <luto@kernel.org> wrote: >> >> >> >> On Fri, Oct 15, 2021, at 8:55 AM, Thomas Gleixner wrote: >> > On Thu, Oct 14 2021 at 19:51, Andy Lutomirski wrote: >> >> On Wed, Oct 13, 2021, at 11:16 AM, Sami Tolvanen wrote: >> >>> >> >>> +/* >> >>> + * Declares a function not callable from C using an opaque type. Defined as >> >>> + * an array to allow the address of the symbol to be taken without '&'. >> >>> + */ >> >> I’m not convinced that taking the address without using & is a >> >> laudable goal. The magical arrays-are-pointers-too behavior of C is a >> >> mistake, not a delightful simplification. >> > >> >>> +#ifndef DECLARE_NOT_CALLED_FROM_C >> >>> +#define DECLARE_NOT_CALLED_FROM_C(sym) \ >> >>> + extern const u8 sym[] >> >>> +#endif >> >> >> > >> >> The relevant property of these symbols isn’t that they’re not called >> >> from C. The relevant thing is that they are just and not objects of a >> >> type that the programmer cares to tell the compiler about. (Or that >> >> the compiler understands, for that matter. On a system with XO memory >> >> or if they’re in a funny section, dereferencing them may fail.) >> > >> > I agree. >> > >> >> So I think we should use incomplete structs, which can’t be >> >> dereferenced and will therefore be less error prone. >> > >> > While being late to that bike shed painting party, I really have to ask >> > the question _why_ can't the compiler provide an annotation for these >> > kind of things which: >> > >> > 1) Make the build fail when invoked directly >> > >> > 2) Tell CFI that this is _NOT_ something it can understand >> > >> > -void clear_page_erms(void *page); >> > +void __bikeshedme clear_page_erms(void *page); >> > >> > That still tells me: >> > >> > 1) This is a function >> > >> > 2) It has a regular argument which is expected to be in RDI >> > >> > which even allows to do analyis of e.g. the alternative call which >> > invokes that function. >> > >> > DECLARE_NOT_CALLED_FROM_C(clear_page_erms); >> > >> > loses these properties and IMO it's a tasteless hack. >> > >> >> >> Ah, but clear_page_erms is a different beast entirely as compared to, say, the syscall entry. It *is* a C function. So I see two ways to handle it: >> >> 1. Make it completely opaque. Tglx doesn’t like it, and I agree, but it would *work*. >> >> 2. Make it a correctly typed function. In clang CFI land, this may or may not be “canonical” (or non canonical?). > > Technically speaking the clear_page_* declarations don't need to be > changed for CFI, they do work fine as is, but I included them in the > patch as they're not actually called from C code right now. But you're > right, we should use a proper function declarations for these. I'll > drop the changes to this file in the next version. If you were to call (with a regular C function call using ()) clear_page_erms, what happens? IMO it should either work or fail to compile. Crashing is no good. > > I wouldn't mind having a consensus on how to deal with exception > handlers etc. though. Should I still use opaque types for those? > Yes, as they are not C functions. > Sami
On Fri, Oct 15 2021 at 17:55, Thomas Gleixner wrote: > On Thu, Oct 14 2021 at 19:51, Andy Lutomirski wrote: >> On Wed, Oct 13, 2021, at 11:16 AM, Sami Tolvanen wrote: > That still tells me: > > 1) This is a function > > 2) It has a regular argument which is expected to be in RDI > > which even allows to do analyis of e.g. the alternative call which > invokes that function. > > DECLARE_NOT_CALLED_FROM_C(clear_page_erms); > > loses these properties and IMO it's a tasteless hack. Look: SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head, size_t, len) Not beautiful, but it gives the information which is needed and it tells me clearly what this is about. While the above lumps everything together whatever it is. Having __bikeshedme would allow to do: __hardware_call __xenhv_call __inline_asm_call or such, which clearly tells how the function should be used and it can even be validated by tooling. You could to that with macros as well, but thats not what you offered. Seriously, if you want to sell me that stuff, then you really should offer me something which has a value on its own and makes it palatable to me. That's not something new. See: https://lore.kernel.org/lkml/alpine.LFD.2.00.1001251002430.3574@localhost.localdomain/ That said, I still want to have a coherent technical explanation why the compiler people cannot come up with a sensible annotation for these things. I'm tired of this attitude, that they add features and then ask us to make our code more ugly. It's not a well hidden secret that the kernel uses these kind of constructs. So why on earth can't they be bothered to think about these things upfront and offer us something nice and useful? Thanks, tglx
On Fri, Oct 15, 2021 at 10:57 AM Thomas Gleixner <tglx@linutronix.de> wrote: > > On Fri, Oct 15 2021 at 17:55, Thomas Gleixner wrote: > > On Thu, Oct 14 2021 at 19:51, Andy Lutomirski wrote: > >> On Wed, Oct 13, 2021, at 11:16 AM, Sami Tolvanen wrote: > > That still tells me: > > > > 1) This is a function > > > > 2) It has a regular argument which is expected to be in RDI > > > > which even allows to do analyis of e.g. the alternative call which > > invokes that function. > > > > DECLARE_NOT_CALLED_FROM_C(clear_page_erms); > > > > loses these properties and IMO it's a tasteless hack. > > Look: > > SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head, > size_t, len) > > Not beautiful, but it gives the information which is needed and it tells > me clearly what this is about. While the above lumps everything together > whatever it is. Sure, that makes sense. Ignoring the macro for a moment, how do you feel about using incomplete structs for the non-C functions as Andy suggested? > Having __bikeshedme would allow to do: > > __hardware_call > __xenhv_call > __inline_asm_call > > or such, which clearly tells how the function should be used and it can > even be validated by tooling. Previously you suggested adding a built-in function to the compiler: https://lore.kernel.org/lkml/877dl0sc2m.ffs@nanos.tec.linutronix.de/ I actually did implement this in Clang, but the feature wasn't necessary with opaque types, so I never moved forward with those patches. A built-in also won't make the code any cleaner, which was a concern last time. I do agree that a function attribute would look cleaner, but it won't stop anyone from mistakenly calling these functions from C code, which was something Andy wanted to address at the same time. Do you still prefer a function attribute over using an opaque type nevertheless? > You could to that with macros as well, but thats not what you offered. > > Seriously, if you want to sell me that stuff, then you really should > offer me something which has a value on its own and makes it palatable > to me. That's not something new. See: > > https://lore.kernel.org/lkml/alpine.LFD.2.00.1001251002430.3574@localhost.localdomain/ > > That said, I still want to have a coherent technical explanation why the > compiler people cannot come up with a sensible annotation for these > things. I can only assume they didn't think about this specific use case. Sami
On Fri, Oct 15, 2021, at 11:42 AM, Sami Tolvanen wrote: >> https://lore.kernel.org/lkml/alpine.LFD.2.00.1001251002430.3574@localhost.localdomain/ >> >> That said, I still want to have a coherent technical explanation why the >> compiler people cannot come up with a sensible annotation for these >> things. > > I can only assume they didn't think about this specific use case. I must be missing something here. Linux is full of C-ABI functions implemented in asm. Just off of a quick grep: asm_load_gs_index, memset, memmove, basically everything in arch/x86/lib/*.S If they're just declared and called directly from C, it should just work. But an *indirect* call needs some sort of special handling. How does this work in your patchset? Then we get to these nasty cases where, for some reason, we need to explicitly grab the actual entry point or we need to grab the actual literal address that we can call indirectly. This might be alternative_call, where we're trying to be fast and we want to bypass the CFI magic because, despite what the compiler might try to infer, we are doing a direct call (so it can't be the wrong address due a runtime attack, ENDBR isn't needed, etc). And I can easily believe that the opposite comes to mind. And there are things like exception entries, where C calls make no sense, CFI makes no sense, and they should be totally opaque. So I tend to think that tglx is right *and* we need an attribute, because there really are multiple things going on here. SYM_FUNC_START(c_callable_func) ... ret SYM_FUNC_END extern __magic int c_callable_func(whatever); Surely *something* needs to go where __magic is to tell the compiler that we have a function that wasn't generated by a CFI-aware compiler and that it's just a C ABI function. (Or maybe this is completely implicit? I can't keep track of exactly which thing generates which code in clang CFI.) But we *also* have the read-the-address thing: void something(void) { /* actual C body */ } alternative_call(something, someotherthing, ...); That wants to expand to assembly code that does: CALL [target] where [target] is the actual first instruction of real code and not a CFI prologue. Or, inversely, we want: void (*ptr)(void) = something; which (I presume -- correct me if I'm wrong) wants the CFI landing pad. It's not the same address. And this all wants to work both for asm-defined functions and C-defined functions. This really is orthogonal to the is-it-asm-or-is-it-C things. All four combinations are possible. Does this make any sense? I kind of thing we want the attributes and the builtin, along the lines of: asm("call %m", function_nocfi_address(something)); or however else we wire it up. (And, of course, the things that aren't C functions at all, like exception entries, should be opaque.)
On Fri, Oct 15, 2021 at 12:36 PM Andy Lutomirski <luto@kernel.org> wrote: > > On Fri, Oct 15, 2021, at 11:42 AM, Sami Tolvanen wrote: > >> https://lore.kernel.org/lkml/alpine.LFD.2.00.1001251002430.3574@localhost.localdomain/ > >> > >> That said, I still want to have a coherent technical explanation why the > >> compiler people cannot come up with a sensible annotation for these > >> things. > > > > I can only assume they didn't think about this specific use case. > > I must be missing something here. Linux is full of C-ABI functions implemented in asm. Just off of a quick grep: > > asm_load_gs_index, memset, memmove, basically everything in arch/x86/lib/*.S > > If they're just declared and called directly from C, it should just work. But an *indirect* call needs some sort of special handling. How does this work in your patchset? Making indirect calls to functions implemented in assembly doesn't require special handling. The type is inferred from the C function declaration. > Then we get to these nasty cases where, for some reason, we need to explicitly grab the actual entry point or we need to grab the actual literal address that we can call indirectly. This might be alternative_call, where we're trying to be fast and we want to bypass the CFI magic because, despite what the compiler might try to infer, we are doing a direct call (so it can't be the wrong address due a runtime attack, ENDBR isn't needed, etc). And I can easily believe that the opposite comes to mind. And there are things like exception entries, where C calls make no sense, CFI makes no sense, and they should be totally opaque. Correct, this is the main issue we're trying to solve. For low-level entry points, we want the actual symbol address instead of the CFI magic, both because of performance and because CFI jump tables may not be mapped into memory with KPTI. Using an opaque type cleanly accomplishes the goal for symbols that are not callable from C code. > So I tend to think that tglx is right *and* we need an attribute, because there really are multiple things going on here. > > SYM_FUNC_START(c_callable_func) > ... > ret > SYM_FUNC_END > > extern __magic int c_callable_func(whatever); > > Surely *something* needs to go where __magic is to tell the compiler that we have a function that wasn't generated by a CFI-aware compiler and that it's just a C ABI function. (Or maybe this is completely implicit? I can't keep track of exactly which thing generates which code in clang CFI.) This is implicit. Nothing is needed for this case. > But we *also* have the read-the-address thing: > > void something(void) > { > /* actual C body */ > } > alternative_call(something, someotherthing, ...); > > That wants to expand to assembly code that does: > > CALL [target] > > where [target] is the actual first instruction of real code and not a CFI prologue. Yes, here we would ideally want to avoid the CFI stub for better performance, but nothing actually breaks even if we don't. > Or, inversely, we want: > > void (*ptr)(void) = something; > > which (I presume -- correct me if I'm wrong) wants the CFI landing pad. It's not the same address. Correct. > And this all wants to work both for asm-defined functions and C-defined functions. This really is orthogonal to the is-it-asm-or-is-it-C things. All four combinations are possible. > > Does this make any sense? I kind of thing we want the attributes and the builtin, along the lines of: > > asm("call %m", function_nocfi_address(something)); > > or however else we wire it up. > > (And, of course, the things that aren't C functions at all, like exception entries, should be opaque.) I agree, there are cases where having a function attribute and/or a built-in to stop the compiler from interfering would be useful. I'll dust off my patch series and see how the LLVM folks feel about it. Sami
On Fri, Oct 15 2021 at 11:42, Sami Tolvanen wrote: > On Fri, Oct 15, 2021 at 10:57 AM Thomas Gleixner <tglx@linutronix.de> wrote: >> Not beautiful, but it gives the information which is needed and it tells >> me clearly what this is about. While the above lumps everything together >> whatever it is. > > Sure, that makes sense. Ignoring the macro for a moment, how do you > feel about using incomplete structs for the non-C functions as Andy > suggested? I think I agreed with that back then when he suggested it the first time. That still allows me to do a classification: struct asm_exception struct asm_xen_hv_call .... >> Having __bikeshedme would allow to do: >> >> __hardware_call >> __xenhv_call >> __inline_asm_call >> >> or such, which clearly tells how the function should be used and it can >> even be validated by tooling. > > Previously you suggested adding a built-in function to the compiler: > > https://lore.kernel.org/lkml/877dl0sc2m.ffs@nanos.tec.linutronix.de/ > > I actually did implement this in Clang, but the feature wasn't > necessary with opaque types, so I never moved forward with those > patches. A built-in also won't make the code any cleaner, which was a > concern last time. > > I do agree that a function attribute would look cleaner, but it won't > stop anyone from mistakenly calling these functions from C code, which > was something Andy wanted to address at the same time. Do you still > prefer a function attribute over using an opaque type nevertheless? For actually callable functions, by some definition of callable, e.g. the clear_page_*() variants a proper attribute would be definitely preferred. That attribute should tell the compiler that the function is using the register arguments correctly but is not suitable for direct invocation because it clobbers registers. So the compiler can just refuse to call such a function if used directly without an inline asm wrapper which describes the clobbers, right? But thinking more about clobbers. The only "annotation" of clobbers we have today are the clobbers in the inline asm, which is fragile too. Something like __attribute__ ((clobbers ("rcx", "rax"))) might be useful by itself because it allows validation of the clobbers in the inline asm wrappers and also allows a analysis tool to look at the ASM code and check whether the above list is correct. Hmm? Thanks, tglx
On Fri, Oct 15, 2021 at 01:37:02PM -0700, Sami Tolvanen wrote: > > But we *also* have the read-the-address thing: > > > > void something(void) > > { > > /* actual C body */ > > } > > alternative_call(something, someotherthing, ...); > > > > That wants to expand to assembly code that does: > > > > CALL [target] > > > > where [target] is the actual first instruction of real code and not > > a CFI prologue. > > Yes, here we would ideally want to avoid the CFI stub for better > performance, but nothing actually breaks even if we don't. That's because there's no CFI involved in alternative_call(). It doesn't use function pointers. It uses direct calls. So all the discussion about clear_page_*() is completely irrelevant. It has nothing to do with CFI. Same for copy_user_enhanced_fast_string() and friends. > > And this all wants to work both for asm-defined functions and > > C-defined functions. This really is orthogonal to the > > is-it-asm-or-is-it-C things. All four combinations are possible. > > > > Does this make any sense? Not really, I think Sami debunked most of your theories :-) I think you're misunderstanding how Clang CFI works. It doesn't instrument the target function. It hooks into the caller's function pointer relocation, so when I try to call func_ptr(), the compiler hijacks the function pointer and forces me to call into a func_ptr.cfi_jt() checking function instead. > > I kind of thing we want the attributes and the builtin, along the lines of: > > > > asm("call %m", function_nocfi_address(something)); > > > > or however else we wire it up. > > > > (And, of course, the things that aren't C functions at all, like > > exception entries, should be opaque.) > > I agree, there are cases where having a function attribute and/or a > built-in to stop the compiler from interfering would be useful. I'll > dust off my patch series and see how the LLVM folks feel about it. With all the talk about function attributes I still haven't heard a clear and specific case where one is needed. If you take out the things that don't actually need the DEFINE_NOT_CALLED_FROM_C() annotation then all you have left is the need for opaque structs as far as I can tell.
On Sat, Oct 16, 2021 at 12:17:40AM +0200, Thomas Gleixner wrote: > For actually callable functions, by some definition of callable, > e.g. the clear_page_*() variants a proper attribute would be definitely > preferred. See my last email, clear_page_*() has nothing to do with CFI in the first place. > That attribute should tell the compiler that the function is using the > register arguments correctly but is not suitable for direct invocation > because it clobbers registers. > > So the compiler can just refuse to call such a function if used directly > without an inline asm wrapper which describes the clobbers, right? > > But thinking more about clobbers. The only "annotation" of clobbers we > have today are the clobbers in the inline asm, which is fragile too. > > Something like > > __attribute__ ((clobbers ("rcx", "rax"))) > > might be useful by itself because it allows validation of the clobbers > in the inline asm wrappers and also allows a analysis tool to look at > the ASM code and check whether the above list is correct. > > Hmm? Functions are allowed to clobber rcx and rax anyway. The clear_page_*() functions follow the C ABI, like (almost) every other asm function in the kernel. I think there's a misunderstanding here, as most of this doesn't have anything to do with CFI anyway.
On Sat, Oct 16, 2021 at 2:12 PM Josh Poimboeuf <jpoimboe@redhat.com> wrote: > > On Fri, Oct 15, 2021 at 01:37:02PM -0700, Sami Tolvanen wrote: > > > But we *also* have the read-the-address thing: > > > > > > void something(void) > > > { > > > /* actual C body */ > > > } > > > alternative_call(something, someotherthing, ...); > > > > > > That wants to expand to assembly code that does: > > > > > > CALL [target] > > > > > > where [target] is the actual first instruction of real code and not > > > a CFI prologue. > > > > Yes, here we would ideally want to avoid the CFI stub for better > > performance, but nothing actually breaks even if we don't. > > That's because there's no CFI involved in alternative_call(). It > doesn't use function pointers. It uses direct calls. Ah, you're right. alternative_call() uses the function name instead of the address, so it's not actually affected by CFI. > > > I kind of thing we want the attributes and the builtin, along the lines of: > > > > > > asm("call %m", function_nocfi_address(something)); > > > > > > or however else we wire it up. > > > > > > (And, of course, the things that aren't C functions at all, like > > > exception entries, should be opaque.) > > > > I agree, there are cases where having a function attribute and/or a > > built-in to stop the compiler from interfering would be useful. I'll > > dust off my patch series and see how the LLVM folks feel about it. > > With all the talk about function attributes I still haven't heard a > clear and specific case where one is needed. > > If you take out the things that don't actually need the > DEFINE_NOT_CALLED_FROM_C() annotation then all you have left is the need > for opaque structs as far as I can tell. Correct. Sami
diff --git a/include/linux/linkage.h b/include/linux/linkage.h index dbf8506decca..f982d5f550ac 100644 --- a/include/linux/linkage.h +++ b/include/linux/linkage.h @@ -48,6 +48,19 @@ #define __PAGE_ALIGNED_DATA .section ".data..page_aligned", "aw" #define __PAGE_ALIGNED_BSS .section ".bss..page_aligned", "aw" +/* + * Declares a function not callable from C using an opaque type. Defined as + * an array to allow the address of the symbol to be taken without '&'. + */ +#ifndef DECLARE_NOT_CALLED_FROM_C +#define DECLARE_NOT_CALLED_FROM_C(sym) \ + extern const u8 sym[] +#endif + +#ifndef __ASSEMBLY__ +typedef const u8 *asm_func_ptr; +#endif + /* * This is used by architectures to keep arguments on the stack * untouched by the compiler by keeping them live until the end.