Messages in this thread | | | Date | Fri, 15 Oct 2021 10:34:16 -0700 | From | "Andy Lutomirski" <> | Subject | Re: [PATCH v5 03/15] linkage: Add DECLARE_NOT_CALLED_FROM_C |
| |
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
| |