| 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
| ||
|
Message-ID: <CAHVXubjv7JKyfK4FBkHUGq+rMTiMExznGEiO8btKkAtg75iqYQ@mail.gmail.com>
Date: Wed, 28 Aug 2024 15:32:15 +0200
From: Alexandre Ghiti <alexghiti@...osinc.com>
To: Samuel Holland <samuel.holland@...ive.com>
Cc: Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>, Guo Ren <guoren@...nel.org>,
Heiko Stuebner <heiko@...ech.de>, Philipp Tomsich <philipp.tomsich@...ll.eu>,
linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH -fixes] riscv: Fix RISCV_ALTERNATIVE_EARLY
Hi Samuel,
On Wed, Aug 28, 2024 at 3:08 PM Samuel Holland
<samuel.holland@...ive.com> wrote:
>
> Hi Alex,
>
> On 2024-08-26 5:57 AM, Alexandre Ghiti wrote:
> > RISCV_ALTERNATIVE_EARLY will issue sbi_ecall() very early in the boot
> > process, before the first memory mapping is setup so we can't have any
> > instrumentation happening here.
> >
> > In addition, when the kernel is relocatable, we must also not issue any
> > relocation this early since they would have been patched virtually only.
> >
> > So, instead of disabling instrumentation for the whole kernel/sbi.c file
> > and compiling it with -fno-pie, simply move __sbi_ecall() and
> > __sbi_base_ecall() into their own file where this is fixed.
>
> Looking at the baseline disassembly from both LLVM 19 and GCC 13.2.0 with
> RISCV_ALTERNATIVE_EARLY + KASAN + TRACEPOINTS, all of the instrumentation in
> __sbi_ecall() itself is out of line and only executed when the tracepoint static
> branches are enabled. However, there is instrumentation in sbi_get_m*id() from
> the switch table inlined from sbi_err_map_linux_errno(), and some of those
> memory accesses are done unconditionally.
>
> This change will force sbi_err_map_linux_errno() to be out of line (unless LTO
> is enabled), so it also forces that particular bit of instrumentation to be
> executed only in the error path. But we could still crash in the error path. So
> I think sbi_err_map_linux_errno() needs to be moved to sbi_ecall.c as well.
>
> Alternatively, sbi_get_m*id() do not have any errors defined (and we don't
> really handle that possibility), so we could drop to the call to
> sbi_err_map_linux_errno() from __sbi_base_ecall().
Damn, I should have checked but I was sure it was a static inline in
sbi.h, which would be another solution. I prefer this solution, WDYT?
Thanks,
Alex
>
> Regards,
> Samuel
>
> > Fixes: 1745cfafebdf ("riscv: don't use global static vars to store alternative data")
> > Signed-off-by: Alexandre Ghiti <alexghiti@...osinc.com>
> > ---
> > arch/riscv/include/asm/sbi.h | 2 ++
> > arch/riscv/kernel/Makefile | 6 ++++-
> > arch/riscv/kernel/sbi.c | 44 --------------------------------
> > arch/riscv/kernel/sbi_ecall.c | 48 +++++++++++++++++++++++++++++++++++
> > 4 files changed, 55 insertions(+), 45 deletions(-)
> > create mode 100644 arch/riscv/kernel/sbi_ecall.c
> >
> > diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> > index 7cffd4ffecd0..5843a10b380e 100644
> > --- a/arch/riscv/include/asm/sbi.h
> > +++ b/arch/riscv/include/asm/sbi.h
> > @@ -9,6 +9,7 @@
> >
> > #include <linux/types.h>
> > #include <linux/cpumask.h>
> > +#include <linux/jump_label.h>
> >
> > #ifdef CONFIG_RISCV_SBI
> > enum sbi_ext_id {
> > @@ -304,6 +305,7 @@ struct sbiret {
> > };
> >
> > void sbi_init(void);
> > +long __sbi_base_ecall(int fid);
> > struct sbiret __sbi_ecall(unsigned long arg0, unsigned long arg1,
> > unsigned long arg2, unsigned long arg3,
> > unsigned long arg4, unsigned long arg5,
> > diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
> > index 06d407f1b30b..7f88cc4931f5 100644
> > --- a/arch/riscv/kernel/Makefile
> > +++ b/arch/riscv/kernel/Makefile
> > @@ -20,17 +20,21 @@ endif
> > ifdef CONFIG_RISCV_ALTERNATIVE_EARLY
> > CFLAGS_alternative.o := -mcmodel=medany
> > CFLAGS_cpufeature.o := -mcmodel=medany
> > +CFLAGS_sbi_ecall.o := -mcmodel=medany
> > ifdef CONFIG_FTRACE
> > CFLAGS_REMOVE_alternative.o = $(CC_FLAGS_FTRACE)
> > CFLAGS_REMOVE_cpufeature.o = $(CC_FLAGS_FTRACE)
> > +CFLAGS_REMOVE_sbi_ecall.o = $(CC_FLAGS_FTRACE)
> > endif
> > ifdef CONFIG_RELOCATABLE
> > CFLAGS_alternative.o += -fno-pie
> > CFLAGS_cpufeature.o += -fno-pie
> > +CFLAGS_sbi_ecall.o += -fno-pie
> > endif
> > ifdef CONFIG_KASAN
> > KASAN_SANITIZE_alternative.o := n
> > KASAN_SANITIZE_cpufeature.o := n
> > +KASAN_SANITIZE_sbi_ecall.o := n
> > endif
> > endif
> >
> > @@ -88,7 +92,7 @@ obj-$(CONFIG_DYNAMIC_FTRACE) += mcount-dyn.o
> >
> > obj-$(CONFIG_PERF_EVENTS) += perf_callchain.o
> > obj-$(CONFIG_HAVE_PERF_REGS) += perf_regs.o
> > -obj-$(CONFIG_RISCV_SBI) += sbi.o
> > +obj-$(CONFIG_RISCV_SBI) += sbi.o sbi_ecall.o
> > ifeq ($(CONFIG_RISCV_SBI), y)
> > obj-$(CONFIG_SMP) += sbi-ipi.o
> > obj-$(CONFIG_SMP) += cpu_ops_sbi.o
> > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > index 837bdab2601b..ace9e2f59c41 100644
> > --- a/arch/riscv/kernel/sbi.c
> > +++ b/arch/riscv/kernel/sbi.c
> > @@ -14,9 +14,6 @@
> > #include <asm/smp.h>
> > #include <asm/tlbflush.h>
> >
> > -#define CREATE_TRACE_POINTS
> > -#include <asm/trace.h>
> > -
> > /* default SBI version is 0.1 */
> > unsigned long sbi_spec_version __ro_after_init = SBI_SPEC_VERSION_DEFAULT;
> > EXPORT_SYMBOL(sbi_spec_version);
> > @@ -27,36 +24,6 @@ static int (*__sbi_rfence)(int fid, const struct cpumask *cpu_mask,
> > unsigned long start, unsigned long size,
> > unsigned long arg4, unsigned long arg5) __ro_after_init;
> >
> > -struct sbiret __sbi_ecall(unsigned long arg0, unsigned long arg1,
> > - unsigned long arg2, unsigned long arg3,
> > - unsigned long arg4, unsigned long arg5,
> > - int fid, int ext)
> > -{
> > - struct sbiret ret;
> > -
> > - trace_sbi_call(ext, fid);
> > -
> > - register uintptr_t a0 asm ("a0") = (uintptr_t)(arg0);
> > - register uintptr_t a1 asm ("a1") = (uintptr_t)(arg1);
> > - register uintptr_t a2 asm ("a2") = (uintptr_t)(arg2);
> > - register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3);
> > - register uintptr_t a4 asm ("a4") = (uintptr_t)(arg4);
> > - register uintptr_t a5 asm ("a5") = (uintptr_t)(arg5);
> > - register uintptr_t a6 asm ("a6") = (uintptr_t)(fid);
> > - register uintptr_t a7 asm ("a7") = (uintptr_t)(ext);
> > - asm volatile ("ecall"
> > - : "+r" (a0), "+r" (a1)
> > - : "r" (a2), "r" (a3), "r" (a4), "r" (a5), "r" (a6), "r" (a7)
> > - : "memory");
> > - ret.error = a0;
> > - ret.value = a1;
> > -
> > - trace_sbi_return(ext, ret.error, ret.value);
> > -
> > - return ret;
> > -}
> > -EXPORT_SYMBOL(__sbi_ecall);
> > -
> > int sbi_err_map_linux_errno(int err)
> > {
> > switch (err) {
> > @@ -535,17 +502,6 @@ long sbi_probe_extension(int extid)
> > }
> > EXPORT_SYMBOL(sbi_probe_extension);
> >
> > -static long __sbi_base_ecall(int fid)
> > -{
> > - struct sbiret ret;
> > -
> > - ret = sbi_ecall(SBI_EXT_BASE, fid, 0, 0, 0, 0, 0, 0);
> > - if (!ret.error)
> > - return ret.value;
> > - else
> > - return sbi_err_map_linux_errno(ret.error);
> > -}
> > -
> > static inline long sbi_get_spec_version(void)
> > {
> > return __sbi_base_ecall(SBI_EXT_BASE_GET_SPEC_VERSION);
> > diff --git a/arch/riscv/kernel/sbi_ecall.c b/arch/riscv/kernel/sbi_ecall.c
> > new file mode 100644
> > index 000000000000..24aabb4fbde3
> > --- /dev/null
> > +++ b/arch/riscv/kernel/sbi_ecall.c
> > @@ -0,0 +1,48 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2024 Rivos Inc. */
> > +
> > +#include <asm/sbi.h>
> > +#define CREATE_TRACE_POINTS
> > +#include <asm/trace.h>
> > +
> > +long __sbi_base_ecall(int fid)
> > +{
> > + struct sbiret ret;
> > +
> > + ret = sbi_ecall(SBI_EXT_BASE, fid, 0, 0, 0, 0, 0, 0);
> > + if (!ret.error)
> > + return ret.value;
> > + else
> > + return sbi_err_map_linux_errno(ret.error);
> > +}
> > +EXPORT_SYMBOL(__sbi_base_ecall);
> > +
> > +struct sbiret __sbi_ecall(unsigned long arg0, unsigned long arg1,
> > + unsigned long arg2, unsigned long arg3,
> > + unsigned long arg4, unsigned long arg5,
> > + int fid, int ext)
> > +{
> > + struct sbiret ret;
> > +
> > + trace_sbi_call(ext, fid);
> > +
> > + register uintptr_t a0 asm ("a0") = (uintptr_t)(arg0);
> > + register uintptr_t a1 asm ("a1") = (uintptr_t)(arg1);
> > + register uintptr_t a2 asm ("a2") = (uintptr_t)(arg2);
> > + register uintptr_t a3 asm ("a3") = (uintptr_t)(arg3);
> > + register uintptr_t a4 asm ("a4") = (uintptr_t)(arg4);
> > + register uintptr_t a5 asm ("a5") = (uintptr_t)(arg5);
> > + register uintptr_t a6 asm ("a6") = (uintptr_t)(fid);
> > + register uintptr_t a7 asm ("a7") = (uintptr_t)(ext);
> > + asm volatile ("ecall"
> > + : "+r" (a0), "+r" (a1)
> > + : "r" (a2), "r" (a3), "r" (a4), "r" (a5), "r" (a6), "r" (a7)
> > + : "memory");
> > + ret.error = a0;
> > + ret.value = a1;
> > +
> > + trace_sbi_return(ext, ret.error, ret.value);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(__sbi_ecall);
>
Powered by blists - more mailing lists