linux-kernel - Re: [PATCH] gendwarfksyms: define __GENKSYMS_

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMj1kXHCX9oinqQ6xSMgKHdVqp16svPrs9bZu0tX_iD6h76NtA@mail.gmail.com>
Date: Thu, 20 Mar 2025 17:41:36 +0100
From: Ard Biesheuvel <ardb@...nel.org>
To: Sami Tolvanen <samitolvanen@...gle.com>, Ingo Molnar <mingo@...nel.org>
Cc: Ard Biesheuvel <ardb+git@...gle.com>, linux-kbuild@...r.kernel.org, brgerst@...il.com, 
	x86@...nel.org, masahiroy@...nel.org, nathan@...nel.org, 
	linux-kernel@...r.kernel.org, nogikh@...gle.com, 
	syzbot+06fd1a3613c50d36129e@...kaller.appspotmail.com
Subject: Re: [PATCH] gendwarfksyms: define __GENKSYMS__ when processing asm-protoypes.h

On Thu, 20 Mar 2025 at 17:22, Sami Tolvanen <samitolvanen@...gle.com> wrote:
>
> On Thu, Mar 20, 2025 at 9:09 AM Ard Biesheuvel <ardb@...nel.org> wrote:
> >
> > On Thu, 20 Mar 2025 at 15:48, Sami Tolvanen <samitolvanen@...gle.com> wrote:
> > >
> > > Hi Ard,
> > >
> > > On Thu, Mar 20, 2025 at 12:07 AM Ard Biesheuvel <ardb+git@...gle.com> wrote:
> > > >
> > > > From: Ard Biesheuvel <ardb@...nel.org>
> > > >
> > > > Ensure that __GENKSYMS__ is #define'd when passing asm/asm-prototypes.h
> > > > through the compiler to capture the exported symbols. This ensures that
> > > > exported symbols such as __ref_stack_chk_guard on x86, which is declared
> > > > conditionally, is visible to the tool.
> > > >
> > > > Otherwise, an error such as the below may be raised, breaking the build
> > > > when CONFIG_GENDWARFKSYMS=y
> > > >
> > > >   <stdin>:4:15: error: use of undeclared identifier '__ref_stack_chk_guard'
> > > >
> > > > Cc: Sami Tolvanen <samitolvanen@...gle.com>
> > > > Reported-by: syzbot+06fd1a3613c50d36129e@...kaller.appspotmail.com
> > > > Signed-off-by: Ard Biesheuvel <ardb@...nel.org>
> > > > ---
> > > >  scripts/Makefile.build | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> > > > index 993708d11874..7855cdc4e763 100644
> > > > --- a/scripts/Makefile.build
> > > > +++ b/scripts/Makefile.build
> > > > @@ -305,6 +305,7 @@ $(obj)/%.rs: $(obj)/%.rs.S FORCE
> > > >  getasmexports =                                                                \
> > > >     { echo "\#include <linux/kernel.h>" ;                               \
> > > >       echo "\#include <linux/string.h>" ;                               \
> > > > +     echo "\#define  __GENKSYMS__" ;                                   \
> > > >       echo "\#include <asm/asm-prototypes.h>" ;                         \
> > > >       $(call getexportsymbols,EXPORT_SYMBOL(\1);) ; }
> > >
> > > This works with gendwarfksyms since __GENKSYMS__ is defined after the
> > > EXPORT_SYMBOL() definition, but I'm now getting warnings with
> > > genksyms:
> > >
> > >   AS      arch/x86/lib/clear_page_64.o
> > > <stdin>:3:10: warning: "__GENKSYMS__" redefined
> > > <command-line>: note: this is the location of the previous definition
> > >
> >
> > Oops.
> >
> > Do you think the fix below should be sufficient?
> >
> > --- a/scripts/Makefile.build
> > +++ b/scripts/Makefile.build
> > @@ -314,7 +314,7 @@
> >  else
> >  cmd_gensymtypes_S =                                                    \
> >         $(getasmexports) |                                              \
> > -       $(CPP) -D__GENKSYMS__ $(c_flags) -xc - | $(genksyms)
> > +       $(CPP) $(c_flags) -xc - | $(genksyms)
> >  endif # CONFIG_GENDWARFKSYMS
> >
> >  quiet_cmd_cpp_s_S = CPP $(quiet_modtag) $@
>
> EXPORT_SYMBOL() has a different definition when __GENKSYMS__ is
> defined, so I think with genksyms we actually do need this on the
> command line. I suppose you could wrap the getasmexports definition in
> #ifndef __GENKSYMS__ to avoid the warning, or just use
> __GENDWARFKSYMS__ like you suggested earlier.
>

Yeah, on second thought, we could just do what Masahiro suggested, and
drop the conditional from asm/asm-prototypes.h

The issue in question only affects definitions, not declarations, and
so having the declaration visible shouldn't be a problem.

Ingo, mind dropping this patch again? We'll do the below instead (I'll
send out the patch in a minute)


--- a/arch/x86/include/asm/asm-prototypes.h
+++ b/arch/x86/include/asm/asm-prototypes.h
@@ -20,6 +20,6 @@
 extern void cmpxchg8b_emu(void);
 #endif

-#if defined(__GENKSYMS__) && defined(CONFIG_STACKPROTECTOR)
+#ifdef CONFIG_STACKPROTECTOR
 extern unsigned long __ref_stack_chk_guard;
 #endif