powerpc: mpc5200: Remove VLA usage
Subsystems (2)
PATCH 1/1 powerpc: mpc5200: Remove VLA usage
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
--- arch/powerpc/platforms/52xx/mpc52xx_pm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
arch/powerpc/platforms/52xx/mpc52xx_pm.c +4 -1
@@ -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);
Signature · 6 lines
2.17.1 -- Kees Cook Pixel Security
Quoted 7 lines · 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 <[email protected]>
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.
Quoted 20 lines
--- arch/powerpc/platforms/52xx/mpc52xx_pm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)arch/powerpc/platforms/52xx/mpc52xx_pm.c +4
-1@@ -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);
Quoted 9 lines · Arnd Bergmann
Quoted 7 lines · 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 <[email protected]>
Quoted 7 lines
This seems particularly nice, not only avoids it the dynamic stack allocation, it
also makes sure the new0x500handler doesn't overflow into the0x600exception 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
Quoted 1 line
Reviewed-by Arnd Bergmann <[email protected]>
Thanks!
-Kees
Quoted 21 lines
Quoted 21 lines
--- arch/powerpc/platforms/52xx/mpc52xx_pm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)arch/powerpc/platforms/52xx/mpc52xx_pm.c +4
-1@@ -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);
Signature · 2 lines
Kees Cook Pixel Security
Michael Ellerman
reviewerQuoted 15 lines · Kees Cook
Quoted 14 lines · Arnd Bergmann
Quoted 7 lines · 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 <[email protected]>This seems particularly nice, not only avoids it the dynamic stack allocation, it
also makes sure the new0x500handler doesn't overflow into the0x600exception handler.
Quoted 8 lines
Quoted 2 lines
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
Quoted 24 lines · Michael Ellerman
Quoted 22 lines · Kees Cook
Quoted 9 lines · Arnd Bergmann
Quoted 7 lines · 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 <[email protected]>Quoted 7 lines
This seems particularly nice, not only avoids it the dynamic stack allocation, it
also makes sure the new0x500handler doesn't overflow into the0x600exception 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
Quoted 7 lines
Quoted 1 line
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
Signature · 2 lines
Kees Cook Pixel Security
Quoted 5 lines · Michael Ellerman
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
Michael Ellerman
reviewerQuoted 10 lines · Segher Boessenkool
Quoted 5 lines · Michael Ellerman
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.
Quoted 2 lines
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