linux-kernel - Re: [PATCH 2/2] tools/nolibc: x86-64: Fix startup code bug

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 18 Oct 2021 06:58:29 +0200
From:   Willy Tarreau <w@....eu>
To:     Ammar Faizi <ammar.faizi@...dents.amikom.ac.id>
Cc:     Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andy Lutomirski <luto@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>,
        David Laight <David.Laight@...LAB.COM>,
        Peter Cordes <peter@...des.ca>,
        Bedirhan KURT <windowz414@...weeb.org>,
        Louvian Lyndal <louvianlyndal@...il.com>
Subject: Re: [PATCH 2/2] tools/nolibc: x86-64: Fix startup code bug

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