[RFC PATCH] ARM: zynq: Use memcpy_toio instead of memcpy on smp bring-up

[RFC PATCH] ARM: zynq: Use memcpy_toio instead of memcpy on smp bring-up

[Luis Araneda]

This fixes a kernel panic (read overflow) on memcpy when
FORTIFY_SOURCE is enabled.

The computed size of memcpy args are:
- p_size (dst): 4294967295 = (size_t) -1
- q_size (src): 1
- size (len): 8

Additionally, the memory is marked as __iomem, so one of
the memcpy_* functions should be used for read/write

Signed-off-by: Luis Araneda <luaraneda@gmail.com>
---

For anyone trying to reproduce / debug this, it panics
before the console has any output.
I used JTAG to find the panic, but I had to comment-out
the call to "zynq_slcr_cpu_stop" as it stops the JTAG
interface and the connection is dropped, at least with OpenOCD.

I run-tested this on a Digilent Zybo Z7 board
---
 arch/arm/mach-zynq/platsmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-zynq/platsmp.c b/arch/arm/mach-zynq/platsmp.c
index a7cfe07156f4..407abade7336 100644
--- a/arch/arm/mach-zynq/platsmp.c
+++ b/arch/arm/mach-zynq/platsmp.c
@@ -57,7 +57,7 @@ int zynq_cpun_start(u32 address, int cpu)
 			* 0x4: Jump by mov instruction
 			* 0x8: Jumping address
 			*/
-			memcpy((__force void *)zero, &zynq_secondary_trampoline,
+			memcpy_toio(zero, &zynq_secondary_trampoline,
 							trampoline_size);
 			writel(address, zero + trampoline_size);
 
-- 
2.22.0

Re: [RFC PATCH] ARM: zynq: Use memcpy_toio instead of memcpy on smp bring-up

[Russell King - ARM Linux admin]

On Tue, Jul 30, 2019 at 12:43:26AM -0400, Luis Araneda wrote:
> This fixes a kernel panic (read overflow) on memcpy when
> FORTIFY_SOURCE is enabled.
> 
> The computed size of memcpy args are:
> - p_size (dst): 4294967295 = (size_t) -1
> - q_size (src): 1
> - size (len): 8
> 
> Additionally, the memory is marked as __iomem, so one of
> the memcpy_* functions should be used for read/write
> 
> Signed-off-by: Luis Araneda <luaraneda@gmail.com>
> ---
> 
> For anyone trying to reproduce / debug this, it panics
> before the console has any output.
> I used JTAG to find the panic, but I had to comment-out
> the call to "zynq_slcr_cpu_stop" as it stops the JTAG
> interface and the connection is dropped, at least with OpenOCD.
> 
> I run-tested this on a Digilent Zybo Z7 board
> ---
>  arch/arm/mach-zynq/platsmp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-zynq/platsmp.c b/arch/arm/mach-zynq/platsmp.c
> index a7cfe07156f4..407abade7336 100644
> --- a/arch/arm/mach-zynq/platsmp.c
> +++ b/arch/arm/mach-zynq/platsmp.c
> @@ -57,7 +57,7 @@ int zynq_cpun_start(u32 address, int cpu)
>  			* 0x4: Jump by mov instruction
>  			* 0x8: Jumping address
>  			*/
> -			memcpy((__force void *)zero, &zynq_secondary_trampoline,
> +			memcpy_toio(zero, &zynq_secondary_trampoline,
>  							trampoline_size);
>  			writel(address, zero + trampoline_size);

I'm not convinced that this is correct.  It looks like
zynq_secondary_trampoline could be either ARM or Thumb code - there is
no .arm directive before it.  If it's ARM code, then this is fine.  If
Thumb code, then zynq_secondary_trampoline will be offset by one, and
we will miss copying the first byte of code.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [RFC PATCH] ARM: zynq: Use memcpy_toio instead of memcpy on smp bring-up

[Luis Araneda]

Hi Russell,

Thanks for reviewing.

On Tue, Jul 30, 2019 at 6:47 AM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Tue, Jul 30, 2019 at 12:43:26AM -0400, Luis Araneda wrote:
> > This fixes a kernel panic (read overflow) on memcpy when
> > FORTIFY_SOURCE is enabled.
[...]
>
> I'm not convinced that this is correct.  It looks like
> zynq_secondary_trampoline could be either ARM or Thumb code - there is
> no .arm directive before it.  If it's ARM code, then this is fine.  If
> Thumb code, then zynq_secondary_trampoline will be offset by one, and
> we will miss copying the first byte of code.

You're right, I tested what happens if the zynq_secondary_trampoline
is ARM or Thumb by editing the file where it's defined, headsmp.S

When the .arm directive is used, the CPU is brought-up correctly,
but if I use .thumb, I get the following message (no panic):
> CPU1: failed to come online

This seems unrelated to solving the panic, as the message
even appears with memcpy and FORTIFY_SOURCE disabled.

I could add the .arm directive to headsmp.S
Is that your expected solution?
Should that change be on a separate commit?

I'd like to know Michal's opinion, as he wrote the code.

Re: [RFC PATCH] ARM: zynq: Use memcpy_toio instead of memcpy on smp bring-up

[Michal Simek]

On 31. 07. 19 6:12, Luis Araneda wrote:
> Hi Russell,
> 
> Thanks for reviewing.
> 
> On Tue, Jul 30, 2019 at 6:47 AM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
>>
>> On Tue, Jul 30, 2019 at 12:43:26AM -0400, Luis Araneda wrote:
>>> This fixes a kernel panic (read overflow) on memcpy when
>>> FORTIFY_SOURCE is enabled.
> [...]
>>
>> I'm not convinced that this is correct.  It looks like
>> zynq_secondary_trampoline could be either ARM or Thumb code - there is
>> no .arm directive before it.  If it's ARM code, then this is fine.  If
>> Thumb code, then zynq_secondary_trampoline will be offset by one, and
>> we will miss copying the first byte of code.
> 
> You're right, I tested what happens if the zynq_secondary_trampoline
> is ARM or Thumb by editing the file where it's defined, headsmp.S
> 
> When the .arm directive is used, the CPU is brought-up correctly,
> but if I use .thumb, I get the following message (no panic):
>> CPU1: failed to come online
> 
> This seems unrelated to solving the panic, as the message
> even appears with memcpy and FORTIFY_SOURCE disabled.
> 
> I could add the .arm directive to headsmp.S
> Is that your expected solution?
> Should that change be on a separate commit?
> 
> I'd like to know Michal's opinion, as he wrote the code.
> 

There are two things together. Thanks Russel to pointing to it.
1. How to support SMP in thumb2 mode?
Adding .arm mode to headsmp.S which ensure that cpu starts in proper
mode and correct code size is copied.
And also point to secondary_startup_arm in zynq_boot_secondary to switch
cpu from arm to thumb mode.

2. And the second is this patch to fix FORTIFY_SOURCE.

Feel free to create the first patch too or I will do it myself.

Thanks,
Michal

Re: [RFC PATCH] ARM: zynq: Use memcpy_toio instead of memcpy on smp bring-up

[Luis Araneda]

Hi Michal,

Thanks for the review.

On Mon, Aug 5, 2019 at 5:53 AM Michal Simek <michal.simek@xilinx.com> wrote:
>
> On 31. 07. 19 6:12, Luis Araneda wrote:
> > Hi Russell,
> >
> > Thanks for reviewing.
> >
> > On Tue, Jul 30, 2019 at 6:47 AM Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> >>
> >> On Tue, Jul 30, 2019 at 12:43:26AM -0400, Luis Araneda wrote:
> >>> This fixes a kernel panic (read overflow) on memcpy when
> >>> FORTIFY_SOURCE is enabled.
> > [...]
> >>
> >> I'm not convinced that this is correct.  It looks like
> >> zynq_secondary_trampoline could be either ARM or Thumb code - there is
> >> no .arm directive before it.  If it's ARM code, then this is fine.  If
> >> Thumb code, then zynq_secondary_trampoline will be offset by one, and
> >> we will miss copying the first byte of code.
> >
> > You're right, I tested what happens if the zynq_secondary_trampoline
> > is ARM or Thumb by editing the file where it's defined, headsmp.S
> >
> > When the .arm directive is used, the CPU is brought-up correctly,
> > but if I use .thumb, I get the following message (no panic):
> >> CPU1: failed to come online
> >
> > This seems unrelated to solving the panic, as the message
> > even appears with memcpy and FORTIFY_SOURCE disabled.
> >
> > I could add the .arm directive to headsmp.S
> > Is that your expected solution?
> > Should that change be on a separate commit?
> >
> > I'd like to know Michal's opinion, as he wrote the code.
> >
>
> There are two things together. Thanks Russel to pointing to it.
> 1. How to support SMP in thumb2 mode?
> Adding .arm mode to headsmp.S which ensure that cpu starts in proper
> mode and correct code size is copied.
> And also point to secondary_startup_arm in zynq_boot_secondary to switch
> cpu from arm to thumb mode.
>
> 2. And the second is this patch to fix FORTIFY_SOURCE.
>
> Feel free to create the first patch too or I will do it myself.

I'll be sending the two patches as a series (I already tested that
they work), so they can be picked by the stable trees.

Thanks,
Luis Araneda.