Re: Possible incorrect usage of STACKALIGN in kern_exec

[christos%astron.com@localhost (Christos Zoulas)]

In article 
<CAB0bowc+LnNhojsszfZgixcL73UDThOD2fqZP-GBy+mPuUz5LA%mail.gmail.com@localhost>,
Paul Fleischer  <paul%xpg.dk@localhost> wrote:
>Hello there,
>
>I am currently working on a NetBSD port for the FriendlyARM MINI2440,
>and have run into a situation where the arguments to user space
>programs is garbled.
>I've noticed it for the init-process and for the getty process.
>Booting with rc_configured=NO in /etc/rc.conf, mounting procfs, and
>issuing a cat /proc/1/cmdline will yields "init" followed by
>"garbage". Always 0x5C 0xEF
>0xFF 0xBF 0x02
>
>Digging a bit around the kernel, I have noticed that line 796 in
>kern_exec.c (execve1) has a special case for ARM only (as far as I can
>tell):
>
>len = STACKALIGN(len); /* make the stack "safely" aligned */
>
>STACKALIGN aligns is implemented in arch/arm/include/param.h:
>
>   96 /* ARM-specific macro to align a stack pointer (downwards). */
>   97 #define STACKALIGNBYTES         (8 - 1)
>   98 #define STACKALIGN(p)           ((u_int)(p) &~ STACKALIGNBYTES)
>
>So it's basically truncates to proper alignment, rounding down to an
>aligned number. Rounding down lengths seems like a dangerous thing to
>do.
>
>Removing the usage of STACKALIGN and always using ALIGN, solves the
>problem for the MINI2440-port.
>
>Looking a the history of kern_exec.c, the following code was commited
>in 1.243 in March 2007:
>
>+#ifdef STACKLALIGN     /* arm, etc. */
>+       len = STACKALIGN(len);  /* make the stack "safely" aligned */
>+#else
>       len = ALIGN(len);       /* make the stack "safely" aligned */
>+#endif
>
>Notice that the #ifdef checks for STACK*L*ALIGN, which is not defined
>anywhere in the kernel.
>In August 2011 the typo was fixed in r.1.323, meaning that the
>STACKALIGN macro has not been used until 4 months ago. This indicates
>to me that STACKALIGN is not really supposed to be used in this
>situation (and neither OpenBSD nor FreeBSD have it, although much of
>the surrounding code is similar).
>
>Is the usage of STACKALIGN indeed incorrect in this situation, or am I
>missing the big picture?

Yes it is. I will commit a fix shortly.

christos