| Message ID | 20211117193244.31162-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: socfpga: Fix crash with CONFIG_FORTIRY_SOURCE | expand |
On Wed, Nov 17, 2021 at 08:32:44PM +0100, Takashi Iwai wrote: > When CONFIG_FORTIFY_SOURCE is set, memcpy() checks the potential > buffer overflow and panics. The code in sofcpga bootstrapping > contains the memcpy() calls are mistakenly translated as the shorter > size, hence it triggers a panic as if it were overflowing. > > This patch adds the __NO_FORTIFY define for avoiding the > false-positive crash. > > Buglink: https://bugzilla.suse.com/show_bug.cgi?id=1192473 > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > > I took an easier path for now, as the attempt with a foced cast > failed. If there is a better way to handle, let me know, I'd happily > resubmit. Thanks! > The way these have been fixed in the past is to declare these as char arrays (see include/asm-generic/sections.h). I'd prefer something like this (totally untested): diff --git a/arch/arm/mach-socfpga/core.h b/arch/arm/mach-socfpga/core.h index fc2608b18a0d..18f01190dcfd 100644 --- a/arch/arm/mach-socfpga/core.h +++ b/arch/arm/mach-socfpga/core.h @@ -33,7 +33,7 @@ extern void __iomem *sdr_ctl_base_addr; u32 socfpga_sdram_self_refresh(u32 sdr_base); extern unsigned int socfpga_sdram_self_refresh_sz; -extern char secondary_trampoline, secondary_trampoline_end; +extern char secondary_trampoline[], secondary_trampoline_end[]; extern unsigned long socfpga_cpu1start_addr; diff --git a/arch/arm/mach-socfpga/platsmp.c b/arch/arm/mach-socfpga/platsmp.c index fbb80b883e5d..201191cf68f3 100644 --- a/arch/arm/mach-socfpga/platsmp.c +++ b/arch/arm/mach-socfpga/platsmp.c @@ -20,14 +20,14 @@ static int socfpga_boot_secondary(unsigned int cpu, struct task_struct *idle) { - int trampoline_size = &secondary_trampoline_end - &secondary_trampoline; + int trampoline_size = secondary_trampoline_end - secondary_trampoline; if (socfpga_cpu1start_addr) { /* This will put CPU #1 into reset. */ writel(RSTMGR_MPUMODRST_CPU1, rst_manager_base_addr + SOCFPGA_RSTMGR_MODMPURST); - memcpy(phys_to_virt(0), &secondary_trampoline, trampoline_size); + memcpy(phys_to_virt(0), secondary_trampoline, trampoline_size); writel(__pa_symbol(secondary_startup), sys_manager_base_addr + (socfpga_cpu1start_addr & 0x000000ff)); @@ -45,12 +45,12 @@ static int socfpga_boot_secondary(unsigned int cpu, struct task_struct *idle) static int socfpga_a10_boot_secondary(unsigned int cpu, struct task_struct *idle) { - int trampoline_size = &secondary_trampoline_end - &secondary_trampoline; + int trampoline_size = secondary_trampoline_end - secondary_trampoline; if (socfpga_cpu1start_addr) { writel(RSTMGR_MPUMODRST_CPU1, rst_manager_base_addr + SOCFPGA_A10_RSTMGR_MODMPURST); - memcpy(phys_to_virt(0), &secondary_trampoline, trampoline_size); + memcpy(phys_to_virt(0), secondary_trampoline, trampoline_size); writel(__pa_symbol(secondary_startup), sys_manager_base_addr + (socfpga_cpu1start_addr & 0x00000fff)); > arch/arm/mach-socfpga/platsmp.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/arm/mach-socfpga/platsmp.c b/arch/arm/mach-socfpga/platsmp.c > index fbb80b883e5d..d46b1af96a8a 100644 > --- a/arch/arm/mach-socfpga/platsmp.c > +++ b/arch/arm/mach-socfpga/platsmp.c > @@ -5,6 +5,9 @@ > * Based on platsmp.c, Copyright (C) 2002 ARM Ltd. > * Copyright (C) 2012 Altera Corporation > */ > + > +#define __NO_FORTIFY /* need to avoid the crash with memcpy() calls */ > + > #include <linux/delay.h> > #include <linux/init.h> > #include <linux/smp.h> > -- > 2.26.2 >
On Wed, 17 Nov 2021 23:01:50 +0100, Kees Cook wrote: > > On Wed, Nov 17, 2021 at 08:32:44PM +0100, Takashi Iwai wrote: > > When CONFIG_FORTIFY_SOURCE is set, memcpy() checks the potential > > buffer overflow and panics. The code in sofcpga bootstrapping > > contains the memcpy() calls are mistakenly translated as the shorter > > size, hence it triggers a panic as if it were overflowing. > > > > This patch adds the __NO_FORTIFY define for avoiding the > > false-positive crash. > > > > Buglink: https://bugzilla.suse.com/show_bug.cgi?id=1192473 > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > --- > > > > I took an easier path for now, as the attempt with a foced cast > > failed. If there is a better way to handle, let me know, I'd happily > > resubmit. Thanks! > > > > The way these have been fixed in the past is to declare these as char > arrays (see include/asm-generic/sections.h). I'd prefer something like > this (totally untested): (snip) That looks better, indeed. I'll rebuild the kernel with it and ask the bug reporter for testing it. Thanks! Takashi
On Thu, 18 Nov 2021 08:27:26 +0100, Takashi Iwai wrote: > > On Wed, 17 Nov 2021 23:01:50 +0100, > Kees Cook wrote: > > > > On Wed, Nov 17, 2021 at 08:32:44PM +0100, Takashi Iwai wrote: > > > When CONFIG_FORTIFY_SOURCE is set, memcpy() checks the potential > > > buffer overflow and panics. The code in sofcpga bootstrapping > > > contains the memcpy() calls are mistakenly translated as the shorter > > > size, hence it triggers a panic as if it were overflowing. > > > > > > This patch adds the __NO_FORTIFY define for avoiding the > > > false-positive crash. > > > > > > Buglink: https://bugzilla.suse.com/show_bug.cgi?id=1192473 > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > > --- > > > > > > I took an easier path for now, as the attempt with a foced cast > > > failed. If there is a better way to handle, let me know, I'd happily > > > resubmit. Thanks! > > > > > > > The way these have been fixed in the past is to declare these as char > > arrays (see include/asm-generic/sections.h). I'd prefer something like > > this (totally untested): > (snip) > > That looks better, indeed. I'll rebuild the kernel with it and ask > the bug reporter for testing it. Confirmed to work. Will resubmit the v2 patch. thanks, Takashi
diff --git a/arch/arm/mach-socfpga/platsmp.c b/arch/arm/mach-socfpga/platsmp.c index fbb80b883e5d..d46b1af96a8a 100644 --- a/arch/arm/mach-socfpga/platsmp.c +++ b/arch/arm/mach-socfpga/platsmp.c @@ -5,6 +5,9 @@ * Based on platsmp.c, Copyright (C) 2002 ARM Ltd. * Copyright (C) 2012 Altera Corporation */ + +#define __NO_FORTIFY /* need to avoid the crash with memcpy() calls */ + #include <linux/delay.h> #include <linux/init.h> #include <linux/smp.h>
When CONFIG_FORTIFY_SOURCE is set, memcpy() checks the potential buffer overflow and panics. The code in sofcpga bootstrapping contains the memcpy() calls are mistakenly translated as the shorter size, hence it triggers a panic as if it were overflowing. This patch adds the __NO_FORTIFY define for avoiding the false-positive crash. Buglink: https://bugzilla.suse.com/show_bug.cgi?id=1192473 Signed-off-by: Takashi Iwai <tiwai@suse.de> --- I took an easier path for now, as the attempt with a foced cast failed. If there is a better way to handle, let me know, I'd happily resubmit. Thanks! arch/arm/mach-socfpga/platsmp.c | 3 +++ 1 file changed, 3 insertions(+)