| Message ID | 20241009072749.45006-1-alexghiti@rivosinc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d49320974c4e880c4ae42301a36c474ac946d183 |
| Headers | show |
| Series | [-fixes] riscv: Do not use fortify in early code | expand |
On 10/9/24 10:27, Alexandre Ghiti wrote: > Early code designates the code executed when the MMU is not yet enabled, > and this comes with some limitations (see > Documentation/arch/riscv/boot.rst, section "Pre-MMU execution"). > > FORTIFY_SOURCE must be disabled then since it can trigger kernel panics > as reported in [1]. > > Reported-by: Jason Montleon <jmontleo@redhat.com> > Closes: https://lore.kernel.org/linux-riscv/CAJD_bPJes4QhmXY5f63GHV9B9HFkSCoaZjk-qCT2NGS7Q9HODg@mail.gmail.com/ [1] > Fixes: a35707c3d850 ("riscv: add memory-type errata for T-Head") > Fixes: 26e7aacb83df ("riscv: Allow to downgrade paging mode from the command line") > Cc: stable@vger.kernel.org > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > --- > arch/riscv/errata/Makefile | 6 ++++++ > arch/riscv/kernel/Makefile | 5 +++++ > arch/riscv/kernel/pi/Makefile | 6 +++++- > 3 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/errata/Makefile b/arch/riscv/errata/Makefile > index 8a2739485123..f0da9d7b39c3 100644 > --- a/arch/riscv/errata/Makefile > +++ b/arch/riscv/errata/Makefile > @@ -2,6 +2,12 @@ ifdef CONFIG_RELOCATABLE > KBUILD_CFLAGS += -fno-pie > endif > > +ifdef CONFIG_RISCV_ALTERNATIVE_EARLY > +ifdef CONFIG_FORTIFY_SOURCE > +KBUILD_CFLAGS += -D__NO_FORTIFY > +endif > +endif > + > obj-$(CONFIG_ERRATA_ANDES) += andes/ > obj-$(CONFIG_ERRATA_SIFIVE) += sifive/ > obj-$(CONFIG_ERRATA_THEAD) += thead/ > diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile > index 7f88cc4931f5..69dc8aaab3fb 100644 > --- a/arch/riscv/kernel/Makefile > +++ b/arch/riscv/kernel/Makefile > @@ -36,6 +36,11 @@ KASAN_SANITIZE_alternative.o := n > KASAN_SANITIZE_cpufeature.o := n > KASAN_SANITIZE_sbi_ecall.o := n > endif > +ifdef CONFIG_FORTIFY_SOURCE > +CFLAGS_alternative.o += -D__NO_FORTIFY > +CFLAGS_cpufeature.o += -D__NO_FORTIFY > +CFLAGS_sbi_ecall.o += -D__NO_FORTIFY > +endif > endif > > extra-y += vmlinux.lds > diff --git a/arch/riscv/kernel/pi/Makefile b/arch/riscv/kernel/pi/Makefile > index d5bf1bc7de62..81d69d45c06c 100644 > --- a/arch/riscv/kernel/pi/Makefile > +++ b/arch/riscv/kernel/pi/Makefile > @@ -16,8 +16,12 @@ KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO), $(KBUILD_CFLAGS)) > KBUILD_CFLAGS += -mcmodel=medany > > CFLAGS_cmdline_early.o += -D__NO_FORTIFY > -CFLAGS_lib-fdt_ro.o += -D__NO_FORTIFY > CFLAGS_fdt_early.o += -D__NO_FORTIFY > +# lib/string.c already defines __NO_FORTIFY > +CFLAGS_ctype.o += -D__NO_FORTIFY > +CFLAGS_lib-fdt.o += -D__NO_FORTIFY > +CFLAGS_lib-fdt_ro.o += -D__NO_FORTIFY > +CFLAGS_archrandom_early.o += -D__NO_FORTIFY > > $(obj)/%.pi.o: OBJCOPYFLAGS := --prefix-symbols=__pi_ \ > --remove-section=.note.gnu.property \ I was having similar boot issues with Nezha D1 and this fixes it for me as well. Thanks (and thanks Emil for pointing me to this patch)! Applied in my Arch Linux RISC-V port.
On 9 Oct 2024, at 08:27, Alexandre Ghiti <alexghiti@rivosinc.com> wrote: > > Early code designates the code executed when the MMU is not yet enabled, > and this comes with some limitations (see > Documentation/arch/riscv/boot.rst, section "Pre-MMU execution"). > > FORTIFY_SOURCE must be disabled then since it can trigger kernel panics > as reported in [1]. > > Reported-by: Jason Montleon <jmontleo@redhat.com> > Closes: https://lore.kernel.org/linux-riscv/CAJD_bPJes4QhmXY5f63GHV9B9HFkSCoaZjk-qCT2NGS7Q9HODg@mail.gmail.com/ [1] > Fixes: a35707c3d850 ("riscv: add memory-type errata for T-Head") > Fixes: 26e7aacb83df ("riscv: Allow to downgrade paging mode from the command line") > Cc: stable@vger.kernel.org > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> Is the problem in [1] not just that the early boot path uses memcpy on the result of ALT_OLD_PTR, which is a wildly out-of-bounds pointer from the compiler’s perspective? If so, it would seem better to use unsafe_memcpy for that one call site rather than use the big __NO_FORTIFY hammer, surely? Presumably the non-early path is just as bad to the compiler, but works because patch_text_nosync isn’t instrumented, so that would just align the two. Getting the implementation to not be silent on failure during early boot would also be a good idea, but it’s surely better to have FORTIFY_SOURCE enabled with no output for positives than disable the checking in the first place and risk uncaught corruption. Jess
On Tue, Oct 15, 2024 at 6:05 PM Jessica Clarke <jrtc27@jrtc27.com> wrote: > > On 9 Oct 2024, at 08:27, Alexandre Ghiti <alexghiti@rivosinc.com> wrote: > > > > Early code designates the code executed when the MMU is not yet enabled, > > and this comes with some limitations (see > > Documentation/arch/riscv/boot.rst, section "Pre-MMU execution"). > > > > FORTIFY_SOURCE must be disabled then since it can trigger kernel panics > > as reported in [1]. > > > > Reported-by: Jason Montleon <jmontleo@redhat.com> > > Closes: https://lore.kernel.org/linux-riscv/CAJD_bPJes4QhmXY5f63GHV9B9HFkSCoaZjk-qCT2NGS7Q9HODg@mail.gmail.com/ [1] > > Fixes: a35707c3d850 ("riscv: add memory-type errata for T-Head") > > Fixes: 26e7aacb83df ("riscv: Allow to downgrade paging mode from the command line") > > Cc: stable@vger.kernel.org > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > > Is the problem in [1] not just that the early boot path uses memcpy on > the result of ALT_OLD_PTR, which is a wildly out-of-bounds pointer from > the compiler’s perspective? If so, it would seem better to use > unsafe_memcpy for that one call site rather than use the big > __NO_FORTIFY hammer, surely? > I can add that replacing memcpy with unsafe_memcpy did also work for me. Once it was narrowed down, this is what I originally did in order to boot. Jason > Presumably the non-early path is just as bad to the compiler, but works > because patch_text_nosync isn’t instrumented, so that would just align > the two. > > Getting the implementation to not be silent on failure during early > boot would also be a good idea, but it’s surely better to have > FORTIFY_SOURCE enabled with no output for positives than disable the > checking in the first place and risk uncaught corruption. > > Jess >
Hi Jessica, On 16/10/2024 00:04, Jessica Clarke wrote: > On 9 Oct 2024, at 08:27, Alexandre Ghiti <alexghiti@rivosinc.com> wrote: >> Early code designates the code executed when the MMU is not yet enabled, >> and this comes with some limitations (see >> Documentation/arch/riscv/boot.rst, section "Pre-MMU execution"). >> >> FORTIFY_SOURCE must be disabled then since it can trigger kernel panics >> as reported in [1]. >> >> Reported-by: Jason Montleon <jmontleo@redhat.com> >> Closes: https://lore.kernel.org/linux-riscv/CAJD_bPJes4QhmXY5f63GHV9B9HFkSCoaZjk-qCT2NGS7Q9HODg@mail.gmail.com/ [1] >> Fixes: a35707c3d850 ("riscv: add memory-type errata for T-Head") >> Fixes: 26e7aacb83df ("riscv: Allow to downgrade paging mode from the command line") >> Cc: stable@vger.kernel.org >> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> > Is the problem in [1] not just that the early boot path uses memcpy on > the result of ALT_OLD_PTR, which is a wildly out-of-bounds pointer from > the compiler’s perspective? If so, it would seem better to use > unsafe_memcpy for that one call site rather than use the big > __NO_FORTIFY hammer, surely? Not sure why fortify complains here, and I have just seen that I forgot to cc Kees (done now). > > Presumably the non-early path is just as bad to the compiler, but works > because patch_text_nosync isn’t instrumented, so that would just align > the two. > > Getting the implementation to not be silent on failure during early > boot would also be a good idea, but it’s surely better to have > FORTIFY_SOURCE enabled with no output for positives than disable the > checking in the first place and risk uncaught corruption. I'm not sure to follow: you propose to use unsafe_memcpy() instead of disabling fortify entirely, so we would not get any warning in case of failure anyway right? Or do you propose to modify the fortify code to somehow print a warning? If the latter, it's hard this soon in the boot process (where the mmu is disabled) to make sure that the printing warning path does not try to access any virtual address (which is why the boot failed in the first place) but maybe Kees has an idea. And I believe that enabling fortify and using the unsafe_*() variants is error-prone since we'd have to make sure that all the "fortified" functions used in that code use the unsafe_*() variants. So to me, it's way easier in terms of maintenance to just disabling fortify. Thanks, Alex > Jess > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On 16 Oct 2024, at 12:26, Alexandre Ghiti <alex@ghiti.fr> wrote: > > Hi Jessica, > > On 16/10/2024 00:04, Jessica Clarke wrote: >> On 9 Oct 2024, at 08:27, Alexandre Ghiti <alexghiti@rivosinc.com> wrote: >>> Early code designates the code executed when the MMU is not yet enabled, >>> and this comes with some limitations (see >>> Documentation/arch/riscv/boot.rst, section "Pre-MMU execution"). >>> >>> FORTIFY_SOURCE must be disabled then since it can trigger kernel panics >>> as reported in [1]. >>> >>> Reported-by: Jason Montleon <jmontleo@redhat.com> >>> Closes: https://lore.kernel.org/linux-riscv/CAJD_bPJes4QhmXY5f63GHV9B9HFkSCoaZjk-qCT2NGS7Q9HODg@mail.gmail.com/ [1] >>> Fixes: a35707c3d850 ("riscv: add memory-type errata for T-Head") >>> Fixes: 26e7aacb83df ("riscv: Allow to downgrade paging mode from the command line") >>> Cc: stable@vger.kernel.org >>> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> >> Is the problem in [1] not just that the early boot path uses memcpy on >> the result of ALT_OLD_PTR, which is a wildly out-of-bounds pointer from >> the compiler’s perspective? If so, it would seem better to use >> unsafe_memcpy for that one call site rather than use the big >> __NO_FORTIFY hammer, surely? > > > Not sure why fortify complains here, and I have just seen that I forgot to cc Kees (done now). > > >> >> Presumably the non-early path is just as bad to the compiler, but works >> because patch_text_nosync isn’t instrumented, so that would just align >> the two. >> >> Getting the implementation to not be silent on failure during early >> boot would also be a good idea, but it’s surely better to have >> FORTIFY_SOURCE enabled with no output for positives than disable the >> checking in the first place and risk uncaught corruption. > > > I'm not sure to follow: you propose to use unsafe_memcpy() instead of disabling fortify entirely, so we would not get any warning in case of failure anyway right? Yes, but no. The point is to disable it only for the problematic function call, not the entire file, so any other fortifiable function calls that exist now or in the future in that file don’t get it unnecessarily disabled too. > Or do you propose to modify the fortify code to somehow print a warning? If the latter, it's hard this soon in the boot process (where the mmu is disabled) to make sure that the printing warning path does not try to access any virtual address (which is why the boot failed in the first place) but maybe Kees has an idea. Not for this patch, just observing it would be nice to have. > And I believe that enabling fortify and using the unsafe_*() variants is error-prone since we'd have to make sure that all the "fortified" functions used in that code use the unsafe_*() variants. I mean, that’s how all these things work, normally? Jess > So to me, it's way easier in terms of maintenance to just disabling fortify. > > Thanks, > > Alex > > >> Jess >> >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Wed, Oct 16, 2024 at 01:26:24PM +0200, Alexandre Ghiti wrote: > On 16/10/2024 00:04, Jessica Clarke wrote: > > Is the problem in [1] not just that the early boot path uses memcpy on > > the result of ALT_OLD_PTR, which is a wildly out-of-bounds pointer from > > the compiler’s perspective? If so, it would seem better to use > > unsafe_memcpy for that one call site rather than use the big > > __NO_FORTIFY hammer, surely? > > Not sure why fortify complains here, and I have just seen that I forgot to > cc Kees (done now). I haven't had time to investigate this -- something is confusing the compiler about the object size. It's likely that it has decided that "char *" is literally pointing to a single byte. (Instead of being unable to determine the origin of the pointer and being forced to return SIZE_MAX for the object size -- "unknown" size.) In other cases, we've been able to convert "char *ptr" to "char ptr[]" and that tells the compiler it's an array of unknown size. That didn't look very possible here. > [...] > And I believe that enabling fortify and using the unsafe_*() variants is > error-prone since we'd have to make sure that all the "fortified" functions > used in that code use the unsafe_*() variants. > > So to me, it's way easier in terms of maintenance to just disabling fortify. I would agree: there's no way to report a fortify failure, so best to turn it off here.
Hello: This patch was applied to riscv/linux.git (fixes) by Palmer Dabbelt <palmer@rivosinc.com>: On Wed, 9 Oct 2024 09:27:49 +0200 you wrote: > Early code designates the code executed when the MMU is not yet enabled, > and this comes with some limitations (see > Documentation/arch/riscv/boot.rst, section "Pre-MMU execution"). > > FORTIFY_SOURCE must be disabled then since it can trigger kernel panics > as reported in [1]. > > [...] Here is the summary with links: - [-fixes] riscv: Do not use fortify in early code https://git.kernel.org/riscv/c/d49320974c4e You are awesome, thank you!
diff --git a/arch/riscv/errata/Makefile b/arch/riscv/errata/Makefile index 8a2739485123..f0da9d7b39c3 100644 --- a/arch/riscv/errata/Makefile +++ b/arch/riscv/errata/Makefile @@ -2,6 +2,12 @@ ifdef CONFIG_RELOCATABLE KBUILD_CFLAGS += -fno-pie endif +ifdef CONFIG_RISCV_ALTERNATIVE_EARLY +ifdef CONFIG_FORTIFY_SOURCE +KBUILD_CFLAGS += -D__NO_FORTIFY +endif +endif + obj-$(CONFIG_ERRATA_ANDES) += andes/ obj-$(CONFIG_ERRATA_SIFIVE) += sifive/ obj-$(CONFIG_ERRATA_THEAD) += thead/ diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile index 7f88cc4931f5..69dc8aaab3fb 100644 --- a/arch/riscv/kernel/Makefile +++ b/arch/riscv/kernel/Makefile @@ -36,6 +36,11 @@ KASAN_SANITIZE_alternative.o := n KASAN_SANITIZE_cpufeature.o := n KASAN_SANITIZE_sbi_ecall.o := n endif +ifdef CONFIG_FORTIFY_SOURCE +CFLAGS_alternative.o += -D__NO_FORTIFY +CFLAGS_cpufeature.o += -D__NO_FORTIFY +CFLAGS_sbi_ecall.o += -D__NO_FORTIFY +endif endif extra-y += vmlinux.lds diff --git a/arch/riscv/kernel/pi/Makefile b/arch/riscv/kernel/pi/Makefile index d5bf1bc7de62..81d69d45c06c 100644 --- a/arch/riscv/kernel/pi/Makefile +++ b/arch/riscv/kernel/pi/Makefile @@ -16,8 +16,12 @@ KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_LTO), $(KBUILD_CFLAGS)) KBUILD_CFLAGS += -mcmodel=medany CFLAGS_cmdline_early.o += -D__NO_FORTIFY -CFLAGS_lib-fdt_ro.o += -D__NO_FORTIFY CFLAGS_fdt_early.o += -D__NO_FORTIFY +# lib/string.c already defines __NO_FORTIFY +CFLAGS_ctype.o += -D__NO_FORTIFY +CFLAGS_lib-fdt.o += -D__NO_FORTIFY +CFLAGS_lib-fdt_ro.o += -D__NO_FORTIFY +CFLAGS_archrandom_early.o += -D__NO_FORTIFY $(obj)/%.pi.o: OBJCOPYFLAGS := --prefix-symbols=__pi_ \ --remove-section=.note.gnu.property \
Early code designates the code executed when the MMU is not yet enabled, and this comes with some limitations (see Documentation/arch/riscv/boot.rst, section "Pre-MMU execution"). FORTIFY_SOURCE must be disabled then since it can trigger kernel panics as reported in [1]. Reported-by: Jason Montleon <jmontleo@redhat.com> Closes: https://lore.kernel.org/linux-riscv/CAJD_bPJes4QhmXY5f63GHV9B9HFkSCoaZjk-qCT2NGS7Q9HODg@mail.gmail.com/ [1] Fixes: a35707c3d850 ("riscv: add memory-type errata for T-Head") Fixes: 26e7aacb83df ("riscv: Allow to downgrade paging mode from the command line") Cc: stable@vger.kernel.org Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com> --- arch/riscv/errata/Makefile | 6 ++++++ arch/riscv/kernel/Makefile | 5 +++++ arch/riscv/kernel/pi/Makefile | 6 +++++- 3 files changed, 16 insertions(+), 1 deletion(-)