Messages in this thread | | | From | Sami Tolvanen <> | Date | Fri, 15 Oct 2021 09:47:06 -0700 | Subject | Re: [PATCH v5 03/15] linkage: Add DECLARE_NOT_CALLED_FROM_C |
| |
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
| |