lkml.org 
[lkml]   [2021]   [Oct]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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

    \
     
     \ /
      Last update: 2021-10-15 18:48    [W:2.516 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site