Before this patch, the _start function looks like this:
0000000000001170 <_start>:
1170: pop %rdi
1171: mov %rsp,%rsi
1174: lea 0x8(%rsi,%rdi,8),%rdx
1179: and $0xfffffffffffffff0,%rsp
117d: sub $0x8,%rsp
1181: call 1000 <main>
1186: movzbq %al,%rdi
118a: mov $0x3c,%rax
1191: syscall
1193: hlt
1194: data16 cs nopw 0x0(%rax,%rax,1)
119f: nop
Note the "and" to %rsp, it makes the %rsp be 16-byte aligned, but then
there is a "sub" with $0x8 which makes the %rsp no longer 16-byte
aligned, then it calls main. That's the bug!
Right before "call", the %rsp must be 16-byte aligned. So the "sub"
here breaks the alignment. Remove it.
Also the content of %rbp may be unspecified at process initialization
time. For example, if the _start gets called by an interpreter, the
interpreter may not zero the %rbp, so we should zero the %rbp on _start.
In this patch, we recode the _start function, and now it looks like
this:
0000000000001170 <_start>:
1170: pop %rdi # argc
1171: mov %rsp,%rsi # argv
1174: lea 0x8(%rsi,%rdi,8),%rdx # envp
1179: xor %ebp,%ebp # %rbp = 0
117b: and $0xfffffffffffffff0,%rsp # %rsp &= -16
117f: call 1000 <main>
1184: mov %eax,%edi
1186: mov $0xe7,%eax
118b: syscall # sys_exit_group(%rdi)
118d: hlt
118e: xchg %ax,%ax
Extra fixes:
- Use NR_exit_group instead of NR_exit.
- Use `mov %eax,%edi` instead of `movzbq %al,%rdi`. This makes the
exit code more observable from strace. While the exit code is
only 8-bit, the kernel has taken care of that, so no need to
worry about it.
Cc: Bedirhan KURT <[email protected]>
Cc: Louvian Lyndal <[email protected]>
Reported-by: Peter Cordes <[email protected]>
Signed-off-by: Ammar Faizi <[email protected]>
---
tools/include/nolibc/nolibc.h | 61 +++++++++++++++++++++++++++--------
1 file changed, 47 insertions(+), 14 deletions(-)
diff --git a/tools/include/nolibc/nolibc.h b/tools/include/nolibc/nolibc.h
index 1483d95c8330..f502b645d363 100644
--- a/tools/include/nolibc/nolibc.h
+++ b/tools/include/nolibc/nolibc.h
@@ -403,21 +403,54 @@ struct stat {
_ret; \
})
+
/* startup code */
-asm(".section .text\n"
- ".global _start\n"
- "_start:\n"
- "pop %rdi\n" // argc (first arg, %rdi)
- "mov %rsp, %rsi\n" // argv[] (second arg, %rsi)
- "lea 8(%rsi,%rdi,8),%rdx\n" // then a NULL then envp (third arg, %rdx)
- "and $-16, %rsp\n" // x86 ABI : esp must be 16-byte aligned when
- "sub $8, %rsp\n" // entering the callee
- "call main\n" // main() returns the status code, we'll exit with it.
- "movzb %al, %rdi\n" // retrieve exit code from 8 lower bits
- "mov $60, %rax\n" // NR_exit == 60
- "syscall\n" // really exit
- "hlt\n" // ensure it does not return
- "");
+asm(
+ ".section .text\n"
+ ".global _start\n"
+
+ "_start:\n\t"
+ "popq %rdi\n\t" // argc (first arg, %rdi)
+ "movq %rsp, %rsi\n\t" // argv[] (second arg, %rsi)
+ "leaq 8(%rsi,%rdi,8), %rdx\n\t" // then a NULL, then envp (third arg, %rdx)
+
+ /*
+ * The System V ABI mandates the deepest stack frame should be zero.
+ * Thus we zero the %rbp here.
+ */
+ "xorl %ebp, %ebp\n\t"
+
+ /*
+ * The System V ABI mandates the %rsp needs to be aligned at 16-byte
+ * before performing a function call.
+ */
+ "andq $-16, %rsp\n\t"
+
+ /*
+ * main() returns the status code, we will exit with it.
+ */
+ "callq main\n\t"
+
+ /*
+ * Move the return value to the first argument of exit_group.
+ */
+ "movl %eax, %edi\n\t"
+
+ /*
+ * NR_exit_group == 231
+ */
+ "movl $231, %eax\n\t"
+
+ /*
+ * Really exit.
+ */
+ "syscall\n\t"
+
+ /*
+ * Ensure it does not return.
+ */
+ "hlt\n\t"
+);
/* fcntl / open */
#define O_RDONLY 0
--
2.30.2
Hi,
This is a code to test.
Compile with:
gcc -O3 -ggdb3 -nostdlib -o test test.c
Technical explanation:
The System V ABI mandates the %rsp must be 16-byte aligned before
performing a function call, but the current nolibc.h violates it.
This %rsp alignment violation makes the callee can't align its stack
properly. Note that the callee may have a situation where it requires
vector aligned move. For example, `movaps` with memory operand w.r.t.
xmm registers, it requires the src/dst address be 16-byte aligned.
Since the callee can't align its stack properly, it will segfault when
executing `movaps`. The following C code is the reproducer and test
to ensure the bug really exists and this patch fixes it.
```
/* SPDX-License-Identifier: LGPL-2.1 OR MIT */
/*
* Bug reproducer and test for Willy's nolibc (x86-64) by Ammar.
*
* Compile with:
* gcc -O3 -ggdb3 -nostdlib -o test test.c
*/
#include "tools/include/nolibc/nolibc.h"
static void dump_argv(int argc, char *argv[])
{
int i;
const char str[] = "\nDumping argv...\n";
write(1, str, strlen(str));
for (i = 0; i < argc; i++) {
write(1, argv[i], strlen(argv[i]));
write(1, "\n", 1);
}
}
static void dump_envp(char *envp[])
{
int i = 0;
const char str[] = "\nDumping envp...\n";
write(1, str, strlen(str));
while (envp[i] != NULL) {
write(1, envp[i], strlen(envp[i]));
write(1, "\n", 1);
i++;
}
}
#define read_barrier(PTR) __asm__ volatile(""::"r"(PTR):"memory")
__attribute__((__target__("sse2")))
static void test_sse_move_aligned(void)
{
int i;
int arr[32] __attribute__((__aligned__(16)));
/*
* The assignment inside this loop is very likely
* performed with aligned move, thus if we don't
* align the %rsp properly, it will fault!
*
* If we fault due to misaligned here, then there
* must be a caller below us that violates SysV
* ABI w.r.t. to %rsp alignment before func call.
*/
for (i = 0; i < 32; i++)
arr[i] = 1;
read_barrier(arr);
}
int main(int argc, char *argv[], char *envp[])
{
dump_argv(argc, argv);
dump_envp(envp);
test_sse_move_aligned();
return 0;
}
```
--
Ammar Faizi
(Sending again as previous email had my Ubuntu username as sender and
Thunderbird attached my GPG key on it. Hope I cancelled on time.)
Hi Ammar,
I've tested your patchset on my local clone of Linux kernel with up to
date fetch of master branch and this is the output I've gotten after
executing the test binary compiled;
```
# I have replaced Powerline-specific characters to avoid font rendering
# issue.
windowzytch@WindowZUbuntu > ~/linux > master > ./test
Dumping argv...
./test
Dumping envp...
SYSTEMD_EXEC_PID=6168
SSH_AUTH_SOCK=/run/user/1001/keyring/ssh
LANGUAGE=en_US:en
LANG=en_US.UTF-8
PAPERSIZE=letter
SESSION_MANAGER=local/WindowZUbuntu:@/tmp/.ICE-unix/3472,unix/WindowZUbuntu:/tmp/.ICE-unix/3472
XDG_CURRENT_DESKTOP=ubuntu:GNOME
LC_IDENTIFICATION=en_US.UTF-8
DEFAULTS_PATH=/usr/share/gconf/ubuntu-xorg.default.path
XDG_SESSION_CLASS=user
_=/home/windowzytch/linux/./test
COLORTERM=truecolor
GPG_AGENT_INFO=/run/user/1001/gnupg/S.gpg-agent:0:1
QT_IM_MODULE=ibus
DESKTOP_SESSION=ubuntu-xorg
USER=windowzytch
XDG_MENU_PREFIX=gnome-
LC_MEASUREMENT=en_US.UTF-8
G_ENABLE_DIAGNOSTIC=0
HOME=/home/windowzytch
GNOME_TERMINAL_SCREEN=/org/gnome/Terminal/screen/f89f0e46_b4c6_4c0a_8434_4fd73845d5aa
JOURNAL_STREAM=8:92304
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1001/bus
XMODIFIERS=@im=ibus
LC_NUMERIC=en_US.UTF-8
SSH_AGENT_LAUNCHER=gnome-keyring
GTK_MODULES=gail:atk-bridge
XDG_DATA_DIRS=/usr/share/ubuntu-xorg:/home/windowzytch/.local/share/flatpak/exports/share:/var/lib/flatpak/exports/share:/usr/local/share/:/usr/share/:/var/lib/snapd/desktop
WINDOWPATH=2
XDG_SESSION_DESKTOP=ubuntu-xorg
XDG_CONFIG_DIRS=/etc/xdg/xdg-ubuntu-xorg:/etc/xdg
QT_ACCESSIBILITY=1
GNOME_DESKTOP_SESSION_ID=this-is-deprecated
MANAGERPID=3189
LC_TIME=en_US.UTF-8
MANDATORY_PATH=/usr/share/gconf/ubuntu-xorg.mandatory.path
LOGNAME=windowzytch
GNOME_TERMINAL_SERVICE=:1.115
LC_PAPER=en_US.UTF-8
GNOME_SHELL_SESSION_MODE=ubuntu
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/snap/bin
XDG_RUNTIME_DIR=/run/user/1001
SHELL=/bin/zsh
XDG_SESSION_TYPE=x11
LC_MONETARY=en_US.UTF-8
LC_TELEPHONE=en_US.UTF-8
USERNAME=windowzytch
VTE_VERSION=6402
INVOCATION_ID=6052906e58884b5487d3c88314961722
PWD=/home/windowzytch/linux
SHLVL=1
XAUTHORITY=/run/user/1001/gdm/Xauthority
LC_NAME=en_US.UTF-8
IM_CONFIG_PHASE=1
GDMSESSION=ubuntu-xorg
LC_ADDRESS=en_US.UTF-8
DISPLAY=:0
TERM=xterm-256color
OLDPWD=/home/windowzytch/twrprebase/recovery_device_vestel_teos
ZSH=/home/windowzytch/.oh-my-zsh
PAGER=less
LESS=-R
LSCOLORS=Gxfxcxdxbxegedabagacad
LS_COLORS=rs=0:di=01;34:ln=01;36:mh=00:pi=40;33:so=01;35:do=01;35:bd=40;33;01:cd=40;33;01:or=40;31;01:mi=00:su=37;41:sg=30;43:ca=30;41:tw=30;42:ow=34;42:st=37;44:ex=01;32:*.tar=01;31:*.tgz=01;31:*.arc=01;31:*.arj=01;31:*.taz=01;31:*.lha=01;31:*.lz4=01;31:*.lzh=01;31:*.lzma=01;31:*.tlz=01;31:*.txz=01;31:*.tzo=01;31:*.t7z=01;31:*.zip=01;31:*.z=01;31:*.dz=01;31:*.gz=01;31:*.lrz=01;31:*.lz=01;31:*.lzo=01;31:*.xz=01;31:*.zst=01;31:*.tzst=01;31:*.bz2=01;31:*.bz=01;31:*.tbz=01;31:*.tbz2=01;31:*.tz=01;31:*.deb=01;31:*.rpm=01;31:*.jar=01;31:*.war=01;31:*.ear=01;31:*.sar=01;31:*.rar=01;31:*.alz=01;31:*.ace=01;31:*.zoo=01;31:*.cpio=01;31:*.7z=01;31:*.rz=01;31:*.cab=01;31:*.wim=01;31:*.swm=01;31:*.dwm=01;31:*.esd=01;31:*.jpg=01;35:*.jpeg=01;35:*.mjpg=01;35:*.mjpeg=01;35:*.gif=01;35:*.bmp=01;35:*.pbm=01;35:*.pgm=01;35:*.ppm=01;35:*.tga=01;35:*.xbm=01;35:*.xpm=01;35:*.tif=01;35:*.tiff=01;35:*.png=01;35:*.svg=01;35:*.svgz=01;35:*.mng=01;35:*.pcx=01;35:*.mov=01;35:*.mpg=01;35:*.mpeg=01;35:*.m2v=01;35:*.mkv=01;35:*.webm=01;35:*.webp=01;35:*.ogm=01;35:*.mp4=01;35:*.m4v=01;35:*.mp4v=01;35:*.vob=01;35:*.qt=01;35:*.nuv=01;35:*.wmv=01;35:*.asf=01;35:*.rm=01;35:*.rmvb=01;35:*.flc=01;35:*.avi=01;35:*.fli=01;35:*.flv=01;35:*.gl=01;35:*.dl=01;35:*.xcf=01;35:*.xwd=01;35:*.yuv=01;35:*.cgm=01;35:*.emf=01;35:*.ogv=01;35:*.ogx=01;35:*.aac=00;36:*.au=00;36:*.flac=00;36:*.m4a=00;36:*.mid=00;36:*.midi=00;36:*.mka=00;36:*.mp3=00;36:*.mpc=00;36:*.ogg=00;36:*.ra=00;36:*.wav=00;36:*.oga=00;36:*.opus=00;36:*.spx=00;36:*.xspf=00;36:
```
I hope these are helpful and I could help throughout this patchset. I
didn't get any SegFaults compared to my tests with same code on pure
state either so I think everything works just fine. You can append me in
Tested-by tag if you want.
--
Bedirhan KURT
On 10/15/21 11:57, Ammar Faizi wrote:
> Hi,
>
> This is a code to test.
>
> Compile with:
> gcc -O3 -ggdb3 -nostdlib -o test test.c
>
> Technical explanation:
> The System V ABI mandates the %rsp must be 16-byte aligned before
> performing a function call, but the current nolibc.h violates it.
>
> This %rsp alignment violation makes the callee can't align its stack
> properly. Note that the callee may have a situation where it requires
> vector aligned move. For example, `movaps` with memory operand w.r.t.
> xmm registers, it requires the src/dst address be 16-byte aligned.
>
> Since the callee can't align its stack properly, it will segfault when
> executing `movaps`. The following C code is the reproducer and test
> to ensure the bug really exists and this patch fixes it.
>
> ```
> /* SPDX-License-Identifier: LGPL-2.1 OR MIT */
>
> /*
> * Bug reproducer and test for Willy's nolibc (x86-64) by Ammar.
> *
> * Compile with:
> * gcc -O3 -ggdb3 -nostdlib -o test test.c
> */
>
> #include "tools/include/nolibc/nolibc.h"
>
> static void dump_argv(int argc, char *argv[])
> {
> int i;
> const char str[] = "\nDumping argv...\n";
>
> write(1, str, strlen(str));
> for (i = 0; i < argc; i++) {
> write(1, argv[i], strlen(argv[i]));
> write(1, "\n", 1);
> }
> }
>
> static void dump_envp(char *envp[])
> {
> int i = 0;
> const char str[] = "\nDumping envp...\n";
>
> write(1, str, strlen(str));
> while (envp[i] != NULL) {
> write(1, envp[i], strlen(envp[i]));
> write(1, "\n", 1);
> i++;
> }
> }
>
> #define read_barrier(PTR) __asm__ volatile(""::"r"(PTR):"memory")
>
> __attribute__((__target__("sse2")))
> static void test_sse_move_aligned(void)
> {
> int i;
> int arr[32] __attribute__((__aligned__(16)));
>
> /*
> * The assignment inside this loop is very likely
> * performed with aligned move, thus if we don't
> * align the %rsp properly, it will fault!
> *
> * If we fault due to misaligned here, then there
> * must be a caller below us that violates SysV
> * ABI w.r.t. to %rsp alignment before func call.
> */
> for (i = 0; i < 32; i++)
> arr[i] = 1;
>
> read_barrier(arr);
> }
>
> int main(int argc, char *argv[], char *envp[])
> {
> dump_argv(argc, argv);
> dump_envp(envp);
> test_sse_move_aligned();
> return 0;
> }
> ```
>
On Fri, Oct 15, 2021 at 3:57 PM Ammar Faizi wrote:
>
> Hi,
>
> This is a code to test.
>
> Compile with:
> gcc -O3 -ggdb3 -nostdlib -o test test.c
>
> Technical explanation:
> The System V ABI mandates the %rsp must be 16-byte aligned before
> performing a function call, but the current nolibc.h violates it.
>
> This %rsp alignment violation makes the callee can't align its stack
> properly. Note that the callee may have a situation where it requires
> vector aligned move. For example, `movaps` with memory operand w.r.t.
> xmm registers, it requires the src/dst address be 16-byte aligned.
>
> Since the callee can't align its stack properly, it will segfault when
> executing `movaps`. The following C code is the reproducer and test
> to ensure the bug really exists and this patch fixes it.
Hello,
With the current nolibc.h, the program segfault on movaps:
Program received signal SIGSEGV, Segmentation fault.
0x0000555555555032 in dump_argv (argv=0x7fffffffe288, argc=1) at test.c:15
15 const char str[] = "\nDumping argv...\n";
(gdb) x/20i main
0x555555555000 <main>: endbr64
0x555555555004 <main+4>: push %r14
0x555555555006 <main+6>: push %r13
0x555555555008 <main+8>: mov %edi,%r13d
0x55555555500b <main+11>: push %r12
0x55555555500d <main+13>: push %rbp
0x55555555500e <main+14>: mov %rdx,%rbp
0x555555555011 <main+17>: mov $0xa,%edx
0x555555555016 <main+22>: push %rbx
0x555555555017 <main+23>: mov %rsi,%rbx
0x55555555501a <main+26>: sub $0x8,%rsp
0x55555555501e <main+30>: movdqa 0xffa(%rip),%xmm0 # 0x555555556020
0x555555555026 <main+38>: mov %dx,-0x68(%rsp)
0x55555555502b <main+43>: lea -0x78(%rsp),%r12
0x555555555030 <main+48>: xor %edx,%edx
=> 0x555555555032 <main+50>: movaps %xmm0,-0x78(%rsp)
0x555555555037 <main+55>: nopw 0x0(%rax,%rax,1)
0x555555555040 <main+64>: add $0x1,%rdx
0x555555555044 <main+68>: cmpb $0x0,(%r12,%rdx,1)
0x555555555049 <main+73>: jne 0x555555555040 <main+64>
(gdb) p $rsp-0x78
$1 = (void *) 0x7fffffffe1c8
(gdb)
Apparently it's because $rsp-0x78 is not multiple of 16. After this
patchset, it works fine. gcc version 11.1.0
Tested-by: Louvian Lyndal <[email protected]>
On Fri, Oct 15, 2021 at 4:26 PM Bedirhan KURT <[email protected]> wrote:
>
> (Sending again as previous email had my Ubuntu username as sender and
> Thunderbird attached my GPG key on it. Hope I cancelled on time.)
>
> Hi Ammar,
>
> I've tested your patchset on my local clone of Linux kernel with up to
> date fetch of master branch and this is the output I've gotten after
> executing the test binary compiled;
>
[...]
>
> I hope these are helpful and I could help throughout this patchset. I
> didn't get any SegFaults compared to my tests with same code on pure
> state either so I think everything works just fine. You can append me in
> Tested-by tag if you want.
>
Thanks for testing!
Tested-by: Bedirhan KURT <[email protected]>
--
Ammar Faizi
Hi Ammar,
sorry for the delay, I needed to check a few things first.
On Fri, Oct 15, 2021 at 03:25:07PM +0700, Ammar Faizi wrote:
> Before this patch, the _start function looks like this:
>
> 0000000000001170 <_start>:
> 1170: pop %rdi
> 1171: mov %rsp,%rsi
> 1174: lea 0x8(%rsi,%rdi,8),%rdx
> 1179: and $0xfffffffffffffff0,%rsp
> 117d: sub $0x8,%rsp
> 1181: call 1000 <main>
> 1186: movzbq %al,%rdi
> 118a: mov $0x3c,%rax
> 1191: syscall
> 1193: hlt
> 1194: data16 cs nopw 0x0(%rax,%rax,1)
> 119f: nop
>
> Note the "and" to %rsp, it makes the %rsp be 16-byte aligned, but then
> there is a "sub" with $0x8 which makes the %rsp no longer 16-byte
> aligned, then it calls main. That's the bug!
>
> Right before "call", the %rsp must be 16-byte aligned. So the "sub"
> here breaks the alignment. Remove it.
That's very interesting because my understanding till now was that
the stack had to be 16-aligned in the callee, not the caller. But I've
checked the psABI doc, and it indeed says in section 3.2.2 that it's
rsp+8 which is 16-aligned. Of course, when pushing a frame pointer
onto the stack, it becomes the same. Thanks for spotting this one!
> Also the content of %rbp may be unspecified at process initialization
> time. For example, if the _start gets called by an interpreter, the
> interpreter may not zero the %rbp, so we should zero the %rbp on _start.
OK.
> Extra fixes:
> - Use NR_exit_group instead of NR_exit.
Please, this is independent on the fix above so it must be subject
of a different patch with its own justification. Also it should cover
all supported architectures, otherwise programs will start to behave
differently on different targets.
> - Use `mov %eax,%edi` instead of `movzbq %al,%rdi`. This makes the
> exit code more observable from strace. While the exit code is
> only 8-bit, the kernel has taken care of that, so no need to
> worry about it.
I'm fine with this one as well, but similarly, it should be in its own
patch and applied to all architectures.
> /* startup code */
> -asm(".section .text\n"
> - ".global _start\n"
> - "_start:\n"
> - "pop %rdi\n" // argc (first arg, %rdi)
> - "mov %rsp, %rsi\n" // argv[] (second arg, %rsi)
> - "lea 8(%rsi,%rdi,8),%rdx\n" // then a NULL then envp (third arg, %rdx)
> - "and $-16, %rsp\n" // x86 ABI : esp must be 16-byte aligned when
> - "sub $8, %rsp\n" // entering the callee
> - "call main\n" // main() returns the status code, we'll exit with it.
> - "movzb %al, %rdi\n" // retrieve exit code from 8 lower bits
> - "mov $60, %rax\n" // NR_exit == 60
> - "syscall\n" // really exit
> - "hlt\n" // ensure it does not return
> - "");
> +asm(
> + ".section .text\n"
> + ".global _start\n"
> +
> + "_start:\n\t"
> + "popq %rdi\n\t" // argc (first arg, %rdi)
> + "movq %rsp, %rsi\n\t" // argv[] (second arg, %rsi)
> + "leaq 8(%rsi,%rdi,8), %rdx\n\t" // then a NULL, then envp (third arg, %rdx)
> +
> + /*
> + * The System V ABI mandates the deepest stack frame should be zero.
> + * Thus we zero the %rbp here.
> + */
> + "xorl %ebp, %ebp\n\t"
> +
> + /*
> + * The System V ABI mandates the %rsp needs to be aligned at 16-byte
> + * before performing a function call.
> + */
> + "andq $-16, %rsp\n\t"
> +
> + /*
> + * main() returns the status code, we will exit with it.
> + */
> + "callq main\n\t"
> +
> + /*
> + * Move the return value to the first argument of exit_group.
> + */
> + "movl %eax, %edi\n\t"
> +
> + /*
> + * NR_exit_group == 231
> + */
> + "movl $231, %eax\n\t"
> +
> + /*
> + * Really exit.
> + */
> + "syscall\n\t"
> +
> + /*
> + * Ensure it does not return.
> + */
> + "hlt\n\t"
> +);
I find the whole thing much less readable here, as asm code tends to
be read as visual groups of patterns. I'm suggesting that you place a
multi-line comment before the asm statement indicating the general
rules (e.g. lowest stack frame must be zero, rsp+8 must be multiple of
16 etc), then only comment each instruction on the same line as it was
before.
Thank you!
Willy
On Mon, Oct 18, 2021 at 11:59 AM Willy Tarreau <[email protected]> wrote:
> > Extra fixes:
> > - Use NR_exit_group instead of NR_exit.
>
> Please, this is independent on the fix above so it must be subject
> of a different patch with its own justification. Also it should cover
> all supported architectures, otherwise programs will start to behave
> differently on different targets.
>
> > - Use `mov %eax,%edi` instead of `movzbq %al,%rdi`. This makes the
> > exit code more observable from strace. While the exit code is
> > only 8-bit, the kernel has taken care of that, so no need to
> > worry about it.
>
> I'm fine with this one as well, but similarly, it should be in its own
> patch and applied to all architectures.
>
[...]
>
> I find the whole thing much less readable here, as asm code tends to
> be read as visual groups of patterns. I'm suggesting that you place a
> multi-line comment before the asm statement indicating the general
> rules (e.g. lowest stack frame must be zero, rsp+8 must be multiple of
> 16 etc), then only comment each instruction on the same line as it was
> before.
Got it, agree with that. I will address your review and resend this as a
patchset v2 soon.
--
Ammar Faizi
On Sat, Oct 23, 2021 at 08:27:15PM +0700, Ammar Faizi wrote:
> Sorry for the delay, I got extra activities this week. Sorry for not
> giving any update lately.
No, really, don't be sorry, I'm myself quite busy, so I understnad, I
was just inquiring to arrange my time, nothing more.
> 1) I can send the %rsp alignment fix patch. I will send it today or
> tomorrow (GMT+07 time).
OK, no rush anyway. Even early next week is okay for me.
> 2) I can't send the syscall change used for exit. Because I only
> have x86 machine. So I can't apply the changes to other arch(s).
I see. I can do it for the various archs then, as the ones that are
supported are essentially the ones I can test.
> For (2), basically sys_exit doesn't close the entire process. Instead
> it only closes specific thread that calls that syscall. The libc uses
> sys_exit_group to close the process and its threads.
>
> ^ It's not really an urgent thing, because the nolibc.h may not be
> used for multithreaded app. Even so, I don't see something dangerous.
Yep that's what I understood as well, so we may easily postpone this.
> For (1), it's urgent, because the alignment violation causes segfault
> if the compiler generates aligned move, often when we compile it
> with -O3, usually that happens with SSE instructions, like `movdqa`,
> `movaps`.
Yes, that's what I saw from the other reports, I didn't notice it myself
but I probably faced it and attributed it to anything else.
> Preparing the patch...
Great, thank you!
Willy
On Sat, Oct 23, 2021 at 4:02 PM Willy Tarreau <[email protected]> wrote:
>
> Hi Ammar,
>
> On Mon, Oct 18, 2021 at 01:53:29PM +0700, Ammar Faizi wrote:
> > Got it, agree with that. I will address your review and resend this as a
> > patchset v2 soon.
>
> Just checking if you have anything about this or if you're busy. No
> stress, it's just that I prefer to send batches to Paul since he
> rebuilds and retests everything each time, so I'm keeping your first
> patch and another one on hold for now.
>
> Do not hesitate to let me know if you don't have time and if you want
> me to rework your patches myself.
>
> Thanks!
> Willy
Hi Willy,
Sorry for the delay, I got extra activities this week. Sorry for not
giving any update lately.
1) I can send the %rsp alignment fix patch. I will send it today or
tomorrow (GMT+07 time).
2) I can't send the syscall change used for exit. Because I only
have x86 machine. So I can't apply the changes to other arch(s).
For (2), basically sys_exit doesn't close the entire process. Instead
it only closes specific thread that calls that syscall. The libc uses
sys_exit_group to close the process and its threads.
^ It's not really an urgent thing, because the nolibc.h may not be
used for multithreaded app. Even so, I don't see something dangerous.
For (1), it's urgent, because the alignment violation causes segfault
if the compiler generates aligned move, often when we compile it
with -O3, usually that happens with SSE instructions, like `movdqa`,
`movaps`.
Preparing the patch...
--
Ammar Faizi
Hi Willy,
This is a patchset v2, there are 2 patches in this series.
[PATCH 1/2] is a bug fix. Thanks to Peter who reported the bug, fixed
in [PATCH 1/2] by me.
[PATCH 2/2] is just a small improvement to minimize code size, no
functional changes.
Detailed explanation in the commit message.
Please review!
Link v1: https://lore.kernel.org/lkml/[email protected]/
----------------------------------------------------------------
Ammar Faizi (2):
tools/nolibc: x86-64: Fix startup code bug
tools/nolibc: x86-64: Use `mov $60,%eax` instead of `mov $60,%rax`
tools/include/nolibc/nolibc.h | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
Thanks!
--
Ammar Faizi
Before this patch, the `_start` function looks like this:
```
0000000000001170 <_start>:
1170: pop %rdi
1171: mov %rsp,%rsi
1174: lea 0x8(%rsi,%rdi,8),%rdx
1179: and $0xfffffffffffffff0,%rsp
117d: sub $0x8,%rsp
1181: call 1000 <main>
1186: movzbq %al,%rdi
118a: mov $0x3c,%rax
1191: syscall
1193: hlt
1194: data16 cs nopw 0x0(%rax,%rax,1)
119f: nop
```
Note the "and" to %rsp with $-16, it makes the %rsp be 16-byte aligned,
but then there is a "sub" with $0x8 which makes the %rsp no longer
16-byte aligned, then it calls main. That's the bug!
What actually the x86-64 System V ABI mandates is that right before the
"call", the %rsp must be 16-byte aligned, not after the "call". So the
"sub" with $0x8 here breaks the alignment. Remove it.
An example where this rule matters is when the callee needs to align
its stack at 16-byte for aligned move instruction, like `movdqa` and
`movaps`. If the callee can't align its stack properly, it will result
in segmentation fault.
x86-64 System V ABI also mandates the deepest stack frame should be
zero. Just to be safe, let's zero the %rbp on startup as the content
of %rbp may be unspecified when the program starts. Now it looks like
this:
```
0000000000001170 <_start>:
1170: pop %rdi
1171: mov %rsp,%rsi
1174: lea 0x8(%rsi,%rdi,8),%rdx
1179: xor %ebp,%ebp # zero the %rbp
117b: and $0xfffffffffffffff0,%rsp # align the %rsp
117f: call 1000 <main>
1184: movzbq %al,%rdi
1188: mov $0x3c,%rax
118f: syscall
1191: hlt
1192: data16 cs nopw 0x0(%rax,%rax,1)
119d: nopl (%rax)
```
Cc: Bedirhan KURT <[email protected]>
Cc: Louvian Lyndal <[email protected]>
Reported-by: Peter Cordes <[email protected]>
Signed-off-by: Ammar Faizi <[email protected]>
---
tools/include/nolibc/nolibc.h | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/tools/include/nolibc/nolibc.h b/tools/include/nolibc/nolibc.h
index 1483d95c8330..ea38d6f356a1 100644
--- a/tools/include/nolibc/nolibc.h
+++ b/tools/include/nolibc/nolibc.h
@@ -404,14 +404,20 @@ struct stat {
})
/* startup code */
+/*
+ * x86-64 System V ABI mandates:
+ * 1) %rsp must be 16-byte aligned right before the function call.
+ * 2) The deepest stack frame should be zero (the %rbp).
+ *
+ */
asm(".section .text\n"
".global _start\n"
"_start:\n"
"pop %rdi\n" // argc (first arg, %rdi)
"mov %rsp, %rsi\n" // argv[] (second arg, %rsi)
"lea 8(%rsi,%rdi,8),%rdx\n" // then a NULL then envp (third arg, %rdx)
- "and $-16, %rsp\n" // x86 ABI : esp must be 16-byte aligned when
- "sub $8, %rsp\n" // entering the callee
+ "xor %ebp, %ebp\n" // zero the stack frame
+ "and $-16, %rsp\n" // x86 ABI : esp must be 16-byte aligned before call
"call main\n" // main() returns the status code, we'll exit with it.
"movzb %al, %rdi\n" // retrieve exit code from 8 lower bits
"mov $60, %rax\n" // NR_exit == 60
--
2.30.2
Note that mov to 32-bit register will zero extend to 64-bit register.
Thus `mov $60,%eax` has the same effect with `mov $60,%rax`. Use the
shorter opcode to achieve the same thing.
```
b8 3c 00 00 00 mov $60,%eax (5 bytes) [1]
48 c7 c0 3c 00 00 00 mov $60,%rax (7 bytes) [2]
```
Currently, we use [2]. Change it to [1] for shorter code.
Signed-off-by: Ammar Faizi <[email protected]>
---
tools/include/nolibc/nolibc.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/include/nolibc/nolibc.h b/tools/include/nolibc/nolibc.h
index ea38d6f356a1..b1a81f711327 100644
--- a/tools/include/nolibc/nolibc.h
+++ b/tools/include/nolibc/nolibc.h
@@ -420,7 +420,7 @@ asm(".section .text\n"
"and $-16, %rsp\n" // x86 ABI : esp must be 16-byte aligned before call
"call main\n" // main() returns the status code, we'll exit with it.
"movzb %al, %rdi\n" // retrieve exit code from 8 lower bits
- "mov $60, %rax\n" // NR_exit == 60
+ "mov $60, %eax\n" // NR_exit == 60
"syscall\n" // really exit
"hlt\n" // ensure it does not return
"");
--
2.30.2
On Sun, Oct 24, 2021 at 09:11:30AM +0700, Ammar Faizi wrote:
> Hi Willy,
> This is a patchset v2, there are 2 patches in this series.
>
> [PATCH 1/2] is a bug fix. Thanks to Peter who reported the bug, fixed
> in [PATCH 1/2] by me.
>
> [PATCH 2/2] is just a small improvement to minimize code size, no
> functional changes.
>
> Detailed explanation in the commit message.
> Please review!
Many thanks Ammar, both look good, I've queued them and am now reviewing
the ABI and code for other archs in case I did the same mistake for the
alignment at other places (i386 comes to mind).
I'll also have a look at the exit calls your mentioned.
Thanks!
Willy