riscv: Do not use fortify in early code

[PATCH -fixes] riscv: Do not use fortify in early code

Posted by Alexandre Ghiti 1 month, 2 weeks ago

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 \
-- 
2.39.2

Re: [PATCH -fixes] riscv: Do not use fortify in early code

Posted by Jessica Clarke 1 month, 1 week ago

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

Re: [PATCH -fixes] riscv: Do not use fortify in early code

Posted by Alexandre Ghiti 1 month, 1 week ago

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

Re: [PATCH -fixes] riscv: Do not use fortify in early code

Posted by Kees Cook 1 month, 1 week ago

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.

-- 
Kees Cook

Re: [PATCH -fixes] riscv: Do not use fortify in early code

Posted by Jessica Clarke 1 month, 1 week ago

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

Re: [PATCH -fixes] riscv: Do not use fortify in early code

Posted by Jason Montleon 1 month, 1 week ago

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
>

Re: [PATCH -fixes] riscv: Do not use fortify in early code

Posted by Felix Yan 1 month, 1 week ago

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.

-- 
Regards,
Felix Yan