Re: [PATCH v5 03/15] linkage: Add DECLARE_NOT_CALLED_FROM_C

From: Sami Tolvanen <samitolvanen@google.com>
To: Andy Lutomirski <luto@kernel.org>, Borislav Petkov <bp@alien8.de>,
	Josh Poimboeuf <jpoimboe@redhat.com>
Cc: "the arch/x86 maintainers" <x86@kernel.org>,
	Kees Cook <keescook@chromium.org>,
	 "Peter Zijlstra (Intel)" <peterz@infradead.org>,
	Nathan Chancellor <nathan@kernel.org>,
	 Nick Desaulniers <ndesaulniers@google.com>,
	Sedat Dilek <sedat.dilek@gmail.com>,
	 Steven Rostedt <rostedt@goodmis.org>,
	linux-hardening@vger.kernel.org,
	 Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	llvm@lists.linux.dev
Subject: Re: [PATCH v5 03/15] linkage: Add DECLARE_NOT_CALLED_FROM_C
Date: Fri, 15 Oct 2021 08:35:32 -0700	[thread overview]
Message-ID: <CABCJKufbRDv1pfr7aPMcPsqDNJHf+_Otc88TMKg+xnLC-01Kzw@mail.gmail.com> (raw)
In-Reply-To: <7377e6b9-7130-4c20-a0c8-16de4620c995@www.fastmail.com>

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