[PATCH] powerpc: mpc5200: Remove VLA usage

[PATCH] powerpc: mpc5200: Remove VLA usage

[Kees Cook]

In the quest to remove all stack VLA usage from the kernel[1], this
switches to using a stack size large enough for the saved routine and
adds a sanity check.

[1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/powerpc/platforms/52xx/mpc52xx_pm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/52xx/mpc52xx_pm.c b/arch/powerpc/platforms/52xx/mpc52xx_pm.c
index 31d3515672f3..b23da85fa73c 100644
--- a/arch/powerpc/platforms/52xx/mpc52xx_pm.c
+++ b/arch/powerpc/platforms/52xx/mpc52xx_pm.c
@@ -117,7 +117,10 @@ int mpc52xx_pm_enter(suspend_state_t state)
 	u32 intr_main_mask;
 	void __iomem * irq_0x500 = (void __iomem *)CONFIG_KERNEL_START + 0x500;
 	unsigned long irq_0x500_stop = (unsigned long)irq_0x500 + mpc52xx_ds_cached_size;
-	char saved_0x500[mpc52xx_ds_cached_size];
+	char saved_0x500[256];
+
+	if (WARN_ON(mpc52xx_ds_cached_size > sizeof(saved_0x500)))
+		return -ENOMEM;
 
 	/* disable all interrupts in PIC */
 	intr_main_mask = in_be32(&intr->main_mask);
-- 
2.17.1


-- 
Kees Cook
Pixel Security

Re: [PATCH] powerpc: mpc5200: Remove VLA usage

[Arnd Bergmann]

On Fri, Jun 29, 2018 at 8:53 PM, Kees Cook <keescook@chromium.org> wrote:
> In the quest to remove all stack VLA usage from the kernel[1], this
> switches to using a stack size large enough for the saved routine and
> adds a sanity check.
>
> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>
> Signed-off-by: Kees Cook <keescook@chromium.org>

This seems particularly nice, not only avoids it the dynamic stack
allocation, it
also makes sure the new 0x500 handler doesn't overflow into the 0x600
exception handler.

It would help to explain how you arrived at that '256 byte' number in
the changelog though.

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

> ---
>  arch/powerpc/platforms/52xx/mpc52xx_pm.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/platforms/52xx/mpc52xx_pm.c b/arch/powerpc/platforms/52xx/mpc52xx_pm.c
> index 31d3515672f3..b23da85fa73c 100644
> --- a/arch/powerpc/platforms/52xx/mpc52xx_pm.c
> +++ b/arch/powerpc/platforms/52xx/mpc52xx_pm.c
> @@ -117,7 +117,10 @@ int mpc52xx_pm_enter(suspend_state_t state)
>         u32 intr_main_mask;
>         void __iomem * irq_0x500 = (void __iomem *)CONFIG_KERNEL_START + 0x500;
>         unsigned long irq_0x500_stop = (unsigned long)irq_0x500 + mpc52xx_ds_cached_size;
> -       char saved_0x500[mpc52xx_ds_cached_size];
> +       char saved_0x500[256];
> +
> +       if (WARN_ON(mpc52xx_ds_cached_size > sizeof(saved_0x500)))
> +               return -ENOMEM;
>
>         /* disable all interrupts in PIC */
>         intr_main_mask = in_be32(&intr->main_mask);

Re: [PATCH] powerpc: mpc5200: Remove VLA usage

[Kees Cook]

On Fri, Jun 29, 2018 at 2:02 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Jun 29, 2018 at 8:53 PM, Kees Cook <keescook@chromium.org> wrote:
>> In the quest to remove all stack VLA usage from the kernel[1], this
>> switches to using a stack size large enough for the saved routine and
>> adds a sanity check.
>>
>> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>
> This seems particularly nice, not only avoids it the dynamic stack
> allocation, it
> also makes sure the new 0x500 handler doesn't overflow into the 0x600
> exception handler.
>
> It would help to explain how you arrived at that '256 byte' number in
> the changelog though.

Honestly, I just counted instructions, multiplied by 8 and rounded up
to the next nearest power of 2, and the result felt right for giving
some level of flexibility for code growth before tripping the WARN. :P

I'm happy to adjust, of course. :)

-Kees

> Reviewed-by: Arnd Bergmann <arnd@arndb.de>

Thanks!

-Kees

>
>> ---
>>  arch/powerpc/platforms/52xx/mpc52xx_pm.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/platforms/52xx/mpc52xx_pm.c b/arch/powerpc/platforms/52xx/mpc52xx_pm.c
>> index 31d3515672f3..b23da85fa73c 100644
>> --- a/arch/powerpc/platforms/52xx/mpc52xx_pm.c
>> +++ b/arch/powerpc/platforms/52xx/mpc52xx_pm.c
>> @@ -117,7 +117,10 @@ int mpc52xx_pm_enter(suspend_state_t state)
>>         u32 intr_main_mask;
>>         void __iomem * irq_0x500 = (void __iomem *)CONFIG_KERNEL_START + 0x500;
>>         unsigned long irq_0x500_stop = (unsigned long)irq_0x500 + mpc52xx_ds_cached_size;
>> -       char saved_0x500[mpc52xx_ds_cached_size];
>> +       char saved_0x500[256];
>> +
>> +       if (WARN_ON(mpc52xx_ds_cached_size > sizeof(saved_0x500)))
>> +               return -ENOMEM;
>>
>>         /* disable all interrupts in PIC */
>>         intr_main_mask = in_be32(&intr->main_mask);



-- 
Kees Cook
Pixel Security

Re: [PATCH] powerpc: mpc5200: Remove VLA usage

[Michael Ellerman]

Kees Cook <keescook@chromium.org> writes:

> On Fri, Jun 29, 2018 at 2:02 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Fri, Jun 29, 2018 at 8:53 PM, Kees Cook <keescook@chromium.org> wrote:
>>> In the quest to remove all stack VLA usage from the kernel[1], this
>>> switches to using a stack size large enough for the saved routine and
>>> adds a sanity check.
>>>
>>> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>>>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>
>> This seems particularly nice, not only avoids it the dynamic stack
>> allocation, it
>> also makes sure the new 0x500 handler doesn't overflow into the 0x600
>> exception handler.
>>
>> It would help to explain how you arrived at that '256 byte' number in
>> the changelog though.
>
> Honestly, I just counted instructions, multiplied by 8 and rounded up
> to the next nearest power of 2, and the result felt right for giving
> some level of flexibility for code growth before tripping the WARN. :P
>
> I'm happy to adjust, of course. :)

What if we write it:

       char saved_0x500[0x600 - 0x500];

Hopefully the compiler is smart enough not to generate a VLA for that :)

cheers

Re: [PATCH] powerpc: mpc5200: Remove VLA usage

[Kees Cook]

On Sun, Jul 1, 2018 at 6:33 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> Kees Cook <keescook@chromium.org> writes:
>
>> On Fri, Jun 29, 2018 at 2:02 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Fri, Jun 29, 2018 at 8:53 PM, Kees Cook <keescook@chromium.org> wrote:
>>>> In the quest to remove all stack VLA usage from the kernel[1], this
>>>> switches to using a stack size large enough for the saved routine and
>>>> adds a sanity check.
>>>>
>>>> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>>>>
>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>
>>> This seems particularly nice, not only avoids it the dynamic stack
>>> allocation, it
>>> also makes sure the new 0x500 handler doesn't overflow into the 0x600
>>> exception handler.
>>>
>>> It would help to explain how you arrived at that '256 byte' number in
>>> the changelog though.
>>
>> Honestly, I just counted instructions, multiplied by 8 and rounded up
>> to the next nearest power of 2, and the result felt right for giving
>> some level of flexibility for code growth before tripping the WARN. :P
>>
>> I'm happy to adjust, of course. :)
>
> What if we write it:
>
>        char saved_0x500[0x600 - 0x500];
>
> Hopefully the compiler is smart enough not to generate a VLA for that :)

Sure, that's fine. I'll send an updated patch.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] powerpc: mpc5200: Remove VLA usage

[Segher Boessenkool]

On Mon, Jul 02, 2018 at 11:33:32AM +1000, Michael Ellerman wrote:
> What if we write it:
> 
>        char saved_0x500[0x600 - 0x500];
> 
> Hopefully the compiler is smart enough not to generate a VLA for that :)

It is a VLA if the array size is not an integer constant expression.  This
is defined by C; the compiler has nothing to do with it.  0x600-0x500 is
an integer constant expression, so this is not a VLA.

But if you meant if GCC will ever do a dynamic stack allocation for a fixed
size local variable: yes indeed, I hope not!

(Sometimes GCC can avoid this even with VLAs; but in this example we do
not even have a VLA, so it's easier than that :-) )


Segher

Re: [PATCH] powerpc: mpc5200: Remove VLA usage

[Michael Ellerman]

Segher Boessenkool <segher@kernel.crashing.org> writes:
> On Mon, Jul 02, 2018 at 11:33:32AM +1000, Michael Ellerman wrote:
>> What if we write it:
>> 
>>        char saved_0x500[0x600 - 0x500];
>> 
>> Hopefully the compiler is smart enough not to generate a VLA for that :)
>
> It is a VLA if the array size is not an integer constant expression.  This
> is defined by C; the compiler has nothing to do with it.  0x600-0x500 is
> an integer constant expression, so this is not a VLA.

Thanks.

That wasn't meant as a dig at GCC. Kees had an epic struggle with the
kernel's min/max() macros which were causing expressions that looked
like they should be constant to generate VLAs.

> But if you meant if GCC will ever do a dynamic stack allocation for a fixed
> size local variable: yes indeed, I hope not!

Hey that would be cool, just-in-time local variable allocation :)

cheers