| Message ID | 20210816082403.2293846-3-joel@jms.id.au (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc/64s: Fix PPC_BARRIER_NOSPEC=n | expand |
Le 16/08/2021 à 10:24, Joel Stanley a écrit : > When CONFIG_PPC_BARRIER_NOSPEC=n, security.c is not built leading to a > missing definition of uaccess_flush_key. > > LD vmlinux.o > MODPOST vmlinux.symvers > MODINFO modules.builtin.modinfo > GEN modules.builtin > LD .tmp_vmlinux.kallsyms1 > powerpc64le-linux-gnu-ld: arch/powerpc/kernel/align.o:(.toc+0x0): undefined reference to `uaccess_flush_key' > powerpc64le-linux-gnu-ld: arch/powerpc/kernel/signal_64.o:(.toc+0x0): undefined reference to `uaccess_flush_key' > powerpc64le-linux-gnu-ld: arch/powerpc/kernel/process.o:(.toc+0x0): undefined reference to `uaccess_flush_key' > powerpc64le-linux-gnu-ld: arch/powerpc/kernel/traps.o:(.toc+0x0): undefined reference to `uaccess_flush_key' > powerpc64le-linux-gnu-ld: arch/powerpc/kernel/hw_breakpoint_constraints.o:(.toc+0x0): undefined reference to `uaccess_flush_key' > powerpc64le-linux-gnu-ld: arch/powerpc/kernel/ptrace/ptrace.o:(.toc+0x0): more undefined references to `uaccess_flush_key' follow > make[1]: *** [Makefile:1176: vmlinux] Error 1 > > Hack one in to fix the build. > > Signed-off-by: Joel Stanley <joel@jms.id.au> > --- > arch/powerpc/include/asm/security_features.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/powerpc/include/asm/security_features.h b/arch/powerpc/include/asm/security_features.h > index 792eefaf230b..46ade7927a4c 100644 > --- a/arch/powerpc/include/asm/security_features.h > +++ b/arch/powerpc/include/asm/security_features.h > @@ -39,6 +39,9 @@ static inline bool security_ftr_enabled(u64 feature) > return !!(powerpc_security_features & feature); > } > > +#ifndef CONFIG_PPC_BARRIER_NOSPEC > +DEFINE_STATIC_KEY_FALSE(uaccess_flush_key); > +#endif It will then be re-defined by each file that includes asm/security_features.h .... You can't use a DEFINE_ in a .h You have to fix the problem at its source. Cleanest way I see it to modify asm/book3s/64/kup.h and something like if (IS_ENABLED(CONFIG_PPC_BARRIER_NOSPEC) && static_branch_unlikely(&uaccess_flush_key) > > // Features indicating support for Spectre/Meltdown mitigations > >
On Mon, 16 Aug 2021 at 08:39, Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > > Le 16/08/2021 à 10:24, Joel Stanley a écrit : > > When CONFIG_PPC_BARRIER_NOSPEC=n, security.c is not built leading to a > > missing definition of uaccess_flush_key. > > > > LD vmlinux.o > > MODPOST vmlinux.symvers > > MODINFO modules.builtin.modinfo > > GEN modules.builtin > > LD .tmp_vmlinux.kallsyms1 > > powerpc64le-linux-gnu-ld: arch/powerpc/kernel/align.o:(.toc+0x0): undefined reference to `uaccess_flush_key' > > powerpc64le-linux-gnu-ld: arch/powerpc/kernel/signal_64.o:(.toc+0x0): undefined reference to `uaccess_flush_key' > > powerpc64le-linux-gnu-ld: arch/powerpc/kernel/process.o:(.toc+0x0): undefined reference to `uaccess_flush_key' > > powerpc64le-linux-gnu-ld: arch/powerpc/kernel/traps.o:(.toc+0x0): undefined reference to `uaccess_flush_key' > > powerpc64le-linux-gnu-ld: arch/powerpc/kernel/hw_breakpoint_constraints.o:(.toc+0x0): undefined reference to `uaccess_flush_key' > > powerpc64le-linux-gnu-ld: arch/powerpc/kernel/ptrace/ptrace.o:(.toc+0x0): more undefined references to `uaccess_flush_key' follow > > make[1]: *** [Makefile:1176: vmlinux] Error 1 > > > > Hack one in to fix the build. > > > > Signed-off-by: Joel Stanley <joel@jms.id.au> > > --- > > arch/powerpc/include/asm/security_features.h | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/arch/powerpc/include/asm/security_features.h b/arch/powerpc/include/asm/security_features.h > > index 792eefaf230b..46ade7927a4c 100644 > > --- a/arch/powerpc/include/asm/security_features.h > > +++ b/arch/powerpc/include/asm/security_features.h > > @@ -39,6 +39,9 @@ static inline bool security_ftr_enabled(u64 feature) > > return !!(powerpc_security_features & feature); > > } > > > > +#ifndef CONFIG_PPC_BARRIER_NOSPEC > > +DEFINE_STATIC_KEY_FALSE(uaccess_flush_key); > > +#endif > > It will then be re-defined by each file that includes asm/security_features.h .... > > You can't use a DEFINE_ in a .h > > You have to fix the problem at its source. > > Cleanest way I see it to modify asm/book3s/64/kup.h and something like > > if (IS_ENABLED(CONFIG_PPC_BARRIER_NOSPEC) && static_branch_unlikely(&uaccess_flush_key) This won't work either as there's no declaration for uaccess_flush_key: arch/powerpc/include/asm/book3s/64/kup.h:411:78: error: ‘uaccess_flush_key’ undeclared (first use in this function) We could add an extern for it, but that is distasteful as the static key API suggests using the structs directly is deprecated, and the macros we're supposed to use perform initialisation. Could we assume microwatt-like platforms will not gain pkeys support, and have a stubbed out set of prevent/restore_user_access for systems that turn off either pkeys or BARRIER_NOSPEC? Or do we get rid of PPC_BARRIER_NOSPEC as an option, and have machines rely on disabling it at runtime?
Joel Stanley <joel@jms.id.au> writes: > When CONFIG_PPC_BARRIER_NOSPEC=n, security.c is not built leading to a > missing definition of uaccess_flush_key. > > LD vmlinux.o > MODPOST vmlinux.symvers > MODINFO modules.builtin.modinfo > GEN modules.builtin > LD .tmp_vmlinux.kallsyms1 > powerpc64le-linux-gnu-ld: arch/powerpc/kernel/align.o:(.toc+0x0): undefined reference to `uaccess_flush_key' > powerpc64le-linux-gnu-ld: arch/powerpc/kernel/signal_64.o:(.toc+0x0): undefined reference to `uaccess_flush_key' > powerpc64le-linux-gnu-ld: arch/powerpc/kernel/process.o:(.toc+0x0): undefined reference to `uaccess_flush_key' > powerpc64le-linux-gnu-ld: arch/powerpc/kernel/traps.o:(.toc+0x0): undefined reference to `uaccess_flush_key' > powerpc64le-linux-gnu-ld: arch/powerpc/kernel/hw_breakpoint_constraints.o:(.toc+0x0): undefined reference to `uaccess_flush_key' > powerpc64le-linux-gnu-ld: arch/powerpc/kernel/ptrace/ptrace.o:(.toc+0x0): more undefined references to `uaccess_flush_key' follow > make[1]: *** [Makefile:1176: vmlinux] Error 1 > > Hack one in to fix the build. Yeah sorry that is a bit of a hack :) I think the root cause here is that we don't have a CONFIG for "want security workaround stuff", because so far we haven't had a CPU that wants to turn that all off. The generic code uses CONFIG_GENERIC_CPU_VULNERABILITIES, so I guess we should follow that example and add PPC_CPU_VULNERABILITIES. Then we'd use that to disable all the security.c stuff, and PPC_BARRIER_NOSPEC would depend on it also. Then we could allow configuring that off for Microwatt, or possibly for all platforms if people want to do that. cheers
Le 17/08/2021 à 04:44, Joel Stanley a écrit : > On Mon, 16 Aug 2021 at 08:39, Christophe Leroy > <christophe.leroy@csgroup.eu> wrote: >> >> >> >> Le 16/08/2021 à 10:24, Joel Stanley a écrit : >>> When CONFIG_PPC_BARRIER_NOSPEC=n, security.c is not built leading to a >>> missing definition of uaccess_flush_key. >>> >>> LD vmlinux.o >>> MODPOST vmlinux.symvers >>> MODINFO modules.builtin.modinfo >>> GEN modules.builtin >>> LD .tmp_vmlinux.kallsyms1 >>> powerpc64le-linux-gnu-ld: arch/powerpc/kernel/align.o:(.toc+0x0): undefined reference to `uaccess_flush_key' >>> powerpc64le-linux-gnu-ld: arch/powerpc/kernel/signal_64.o:(.toc+0x0): undefined reference to `uaccess_flush_key' >>> powerpc64le-linux-gnu-ld: arch/powerpc/kernel/process.o:(.toc+0x0): undefined reference to `uaccess_flush_key' >>> powerpc64le-linux-gnu-ld: arch/powerpc/kernel/traps.o:(.toc+0x0): undefined reference to `uaccess_flush_key' >>> powerpc64le-linux-gnu-ld: arch/powerpc/kernel/hw_breakpoint_constraints.o:(.toc+0x0): undefined reference to `uaccess_flush_key' >>> powerpc64le-linux-gnu-ld: arch/powerpc/kernel/ptrace/ptrace.o:(.toc+0x0): more undefined references to `uaccess_flush_key' follow >>> make[1]: *** [Makefile:1176: vmlinux] Error 1 >>> >>> Hack one in to fix the build. >>> >>> Signed-off-by: Joel Stanley <joel@jms.id.au> >>> --- >>> arch/powerpc/include/asm/security_features.h | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/arch/powerpc/include/asm/security_features.h b/arch/powerpc/include/asm/security_features.h >>> index 792eefaf230b..46ade7927a4c 100644 >>> --- a/arch/powerpc/include/asm/security_features.h >>> +++ b/arch/powerpc/include/asm/security_features.h >>> @@ -39,6 +39,9 @@ static inline bool security_ftr_enabled(u64 feature) >>> return !!(powerpc_security_features & feature); >>> } >>> >>> +#ifndef CONFIG_PPC_BARRIER_NOSPEC >>> +DEFINE_STATIC_KEY_FALSE(uaccess_flush_key); >>> +#endif >> >> It will then be re-defined by each file that includes asm/security_features.h .... >> >> You can't use a DEFINE_ in a .h >> >> You have to fix the problem at its source. >> >> Cleanest way I see it to modify asm/book3s/64/kup.h and something like >> >> if (IS_ENABLED(CONFIG_PPC_BARRIER_NOSPEC) && static_branch_unlikely(&uaccess_flush_key) > > This won't work either as there's no declaration for uaccess_flush_key: > > arch/powerpc/include/asm/book3s/64/kup.h:411:78: error: > ‘uaccess_flush_key’ undeclared (first use in this function) I can't understand that. According to your commit message, you have reached linking step. It means that the necessary declarations were present. Adding a condition on IS_ENABLED(CONFIG_PPC_BARRIER_NOSPEC) will get you rid of the code past that condition at compile step, then linking will succeed because the code referencing 'uaccess_flush_key' won't be there anymore when you proceed with linking. > > We could add an extern for it, but that is distasteful as the static > key API suggests using the structs directly is deprecated, and the > macros we're supposed to use perform initialisation. You already have DECLARE_STATIC_KEY_FALSE(uaccess_flush_key); in <asm/book3s/64/kup.h> That doesn't initialise anythink but provides the declaration. > > Could we assume microwatt-like platforms will not gain pkeys support, > and have a stubbed out set of prevent/restore_user_access for systems > that turn off either pkeys or BARRIER_NOSPEC? > > Or do we get rid of PPC_BARRIER_NOSPEC as an option, and have machines > rely on disabling it at runtime? >
diff --git a/arch/powerpc/include/asm/security_features.h b/arch/powerpc/include/asm/security_features.h index 792eefaf230b..46ade7927a4c 100644 --- a/arch/powerpc/include/asm/security_features.h +++ b/arch/powerpc/include/asm/security_features.h @@ -39,6 +39,9 @@ static inline bool security_ftr_enabled(u64 feature) return !!(powerpc_security_features & feature); } +#ifndef CONFIG_PPC_BARRIER_NOSPEC +DEFINE_STATIC_KEY_FALSE(uaccess_flush_key); +#endif // Features indicating support for Spectre/Meltdown mitigations
When CONFIG_PPC_BARRIER_NOSPEC=n, security.c is not built leading to a missing definition of uaccess_flush_key. LD vmlinux.o MODPOST vmlinux.symvers MODINFO modules.builtin.modinfo GEN modules.builtin LD .tmp_vmlinux.kallsyms1 powerpc64le-linux-gnu-ld: arch/powerpc/kernel/align.o:(.toc+0x0): undefined reference to `uaccess_flush_key' powerpc64le-linux-gnu-ld: arch/powerpc/kernel/signal_64.o:(.toc+0x0): undefined reference to `uaccess_flush_key' powerpc64le-linux-gnu-ld: arch/powerpc/kernel/process.o:(.toc+0x0): undefined reference to `uaccess_flush_key' powerpc64le-linux-gnu-ld: arch/powerpc/kernel/traps.o:(.toc+0x0): undefined reference to `uaccess_flush_key' powerpc64le-linux-gnu-ld: arch/powerpc/kernel/hw_breakpoint_constraints.o:(.toc+0x0): undefined reference to `uaccess_flush_key' powerpc64le-linux-gnu-ld: arch/powerpc/kernel/ptrace/ptrace.o:(.toc+0x0): more undefined references to `uaccess_flush_key' follow make[1]: *** [Makefile:1176: vmlinux] Error 1 Hack one in to fix the build. Signed-off-by: Joel Stanley <joel@jms.id.au> --- arch/powerpc/include/asm/security_features.h | 3 +++ 1 file changed, 3 insertions(+)