[PATCH] Add comments to i386 assembly source

[PATCH] Add comments to i386 assembly source

[Markus Wichmann]

[-- Attachment #1: Type: text/plain, Size: 1110 bytes --]

Hello everyone,

all the other arch's (I cared to look at) have well-commented or rather
clear assembly sources. Even where it wasn't really needed (PPC's
set_thread_area() would be clear enough without comments, as it's only
two instructions, neither of which complex).

But then there's i386. Without comments, and pulling off some very black
magic, I thought it would be worth commenting the files at least in the
threads directory.

While commenting the files, I noticed something: GS can refer to either
the GDT or the LDT, depending on whether the set_thread_area() syscall
failed or not. If GS refers to the LDT, then the offset portion of it
is 0, so the __clone() function will push an index of 0 on stack. Is
Linux smart enough to notice this, or will the clone() syscall then fail
with EINVAL? It's not documented, but set_thread_area() will fail with
EINVAL if index is out of bounds, and 0 is not a valid value (reason
being that the GDT index 0 is never valid; it's actually the null
selector and causes a GPF if referenced, or, in case of CS, if loaded).

Anyway, enjoy the patch.

Ciao,
Markus

[-- Attachment #2: 0001-Add-comments-to-i386-threading-assembly-files.patch --]
[-- Type: text/x-diff, Size: 4029 bytes --]

From afb818ccc3d565de95903f0b1cde39b6e437f6bf Mon Sep 17 00:00:00 2001
From: Markus Wichmann <nullplan@gmx.net>
Date: Sat, 23 Dec 2017 10:34:31 +0100
Subject: [PATCH] Add comments to i386 threading assembly files.

---
 src/thread/i386/__set_thread_area.s | 22 ++++++++---------
 src/thread/i386/clone.s             | 48 +++++++++++++++++++++----------------
 2 files changed, 39 insertions(+), 31 deletions(-)

diff --git a/src/thread/i386/__set_thread_area.s b/src/thread/i386/__set_thread_area.s
index 3a558fb0..d3eac89e 100644
--- a/src/thread/i386/__set_thread_area.s
+++ b/src/thread/i386/__set_thread_area.s
@@ -6,20 +6,20 @@ __set_thread_area:
 	push $0x51
 	push $0xfffff
 	push 16(%esp)
-	call 1f
-1:	addl $4f-1b,(%esp)
+	call 1f                 /* ebx : 0x51 : 0xfffff : arg : 1f */
+1:	addl $4f-1b,(%esp)      /* ebx : 0x51 : 0xfffff : arg : 4f */
 	pop %ecx
 	mov (%ecx),%edx
-	push %edx
+	push %edx               /* ebx : 0x51 : 0xfffff : arg : [4f] */
 	mov %esp,%ebx
 	xor %eax,%eax
 	mov $243,%al
-	int $128
+	int $128                /* set_thread_area({.index = [4f], .base = arg, .limit=0xfffff, .seg32_bits, .limit_in_pages, .usable}) */
 	testl %eax,%eax
-	jnz 2f
+	jnz 2f                  /* if that failed, go to 2 */
 	movl (%esp),%edx
-	movl %edx,(%ecx)
-	leal 3(,%edx,8),%edx
+	movl %edx,(%ecx)        /* save index in [4f] */
+	leal 3(,%edx,8),%edx    /* multiply index by 8 to get offset for segment selector, add 3 to get CPL 3 */
 3:	movw %dx,%gs
 1:
 	addl $16,%esp
@@ -33,11 +33,11 @@ __set_thread_area:
 	mov $1,%bl
 	mov $16,%dl
 	mov $123,%al
-	int $128
+	int $128                /* modify_ldt(1, {.index = 0, .base = arg, .limit=0xfffff, .seg32_bits, .limit_in_pages, .usable}, 16)*/
 	testl %eax,%eax
-	jnz 1b
-	mov $7,%dl
-	inc %al
+	jnz 1b                  /* if that failed, just clean up and return */
+	mov $7,%dl              /* else the segment selector has offset 0, is in the LDT, and CPL 3 */
+	inc %al                 /* and return 1 */
 	jmp 3b
 
 .data
diff --git a/src/thread/i386/clone.s b/src/thread/i386/clone.s
index 52fe7efb..4e25e52d 100644
--- a/src/thread/i386/clone.s
+++ b/src/thread/i386/clone.s
@@ -1,44 +1,52 @@
 .text
 .global __clone
 .type   __clone,@function
+/* args: 8 - fn
+12 - stack
+16 - flags
+20 - td ptr
+24 - TID ptr
+28 - thread pointer
+32 - TID ptr (again?)
+*/
 __clone:
-	push %ebp
+	push %ebp       /* function intro */
 	mov %esp,%ebp
 	push %ebx
 	push %esi
 	push %edi
 
 	xor %eax,%eax
-	push $0x51
+	push $0x51      /* 0x51 */
 	mov %gs,%ax
-	push $0xfffff
+	push $0xfffff   /* 0x51 : 0xfffff */
 	shr $3,%eax
-	push 28(%ebp)
-	push %eax
-	mov $120,%al
+	push 28(%ebp)   /* 0x51 : 0xfffff : thread pointer */
+	push %eax       /* 0x51 : 0xfffff : thread pointer : current gs index */
+	mov $120,%al    /* __NR_clone */
 
-	mov 12(%ebp),%ecx
-	mov 16(%ebp),%ebx
+	mov 12(%ebp),%ecx   /* ecx = stack */
+	mov 16(%ebp),%ebx   /* ebx = flags */
 	and $-16,%ecx
-	sub $16,%ecx
-	mov 20(%ebp),%edi
-	mov %edi,(%ecx)
-	mov 24(%ebp),%edx
-	mov %esp,%esi
-	mov 32(%ebp),%edi
-	mov 8(%ebp),%ebp
-	int $128
+	sub $16,%ecx        /* align stack */
+	mov 20(%ebp),%edi   /* edi = td pointer */
+	mov %edi,(%ecx)     /* push td pointer to new stack */
+	mov 24(%ebp),%edx   /* edx = TID pointer */
+	mov %esp,%esi       /* esi = esp */
+	mov 32(%ebp),%edi   /* edi = TID pointer */
+	mov 8(%ebp),%ebp    /* ebp = start function (for safe-keeping) */
+	int $128            /* clone(flags, stack, TID pointer, {.index = current gs index, .base = thread pointer, .limit=0xfffff, .seg32_bit, .limit_in_pages, .usable}, td pointer) */
 	test %eax,%eax
-	jnz 1f
+	jnz 1f              /* if that's not 0, just return the return value */
 
-	mov %ebp,%eax
+	mov %ebp,%eax       /* in child: zero out ebp, then call function */
 	xor %ebp,%ebp
 	call *%eax
-	mov %eax,%ebx
+	mov %eax,%ebx       /* exit(rv from function) */
 	xor %eax,%eax
 	inc %eax
 	int $128
-	hlt
+	hlt                 /* exit didn't exit? Crash! */
 
 1:	add $16,%esp
 	pop %edi
-- 
2.14.2

Re: [PATCH] Add comments to i386 assembly source

[John Reiser]

On 12/23/2017 09:45 UTC, Markus Wichmann wrote:

> But then there's i386. Without comments, and pulling off some very black
> magic, I thought it would be worth commenting the files at least in the
> threads directory.

 > -	mov $120,%al
 > +	mov $120,%al    /* __NR_clone */

Using an actual symbol is clearer and easier to maintain or modify:
+__NR_clone = 120
+	mov $__NR_clone,%al

Constant arguments to system calls (including the system call number)
should be loaded last in order to provide the least constraints for computing
non-constant arguments.  Also, it is not obvious that as values (%eax == %al).
The value in %eax was set by "xor %eax,%eax; ...; mov %gs,%ax; ...; shr $3,%eax";
what guarantees that (%gs <= (255 << 3)) ?  %gs could be as high as (8191 << 3).
So _that_ deserves a comment; else for safety all of %eax should be set:
+	push $__NR_clone; pop %eax   /* 3 bytes; __NR_clone < 128 */
+	int $128            /* clone(flags, stack, TID pointer, {.index = current gs index, .base = thread pointer, .limit=0xfffff, .seg32_bit, .limit_in_pages, .usable}, td pointer) */

Clarity can be improved by using a symbol:
NBPW = 4  /* Number of Bytes Per Word */
	mov 3*NBPW(%ebp),%ecx  /* ecx = stack */
	mov 4*NBPW(%ebp),%ebx  /* ebx = flags */
etc.

Incorrect comment:
> +	sub $16,%ecx        /* align stack */
Perhaps you meant "/* allocate space for returned segment descriptor */"?
The alignment is performed by:
  	and $-4*NBPW,%ecx  /* align for stack */

If you are aiming for small space then
+	mov %eax,%ebx       /* exit(rv from function) */
can be implemented one byte smaller as:
+	xchg %eax,%ebx  /* syscall arg %ebx = rv from function; %eax = do not care */

-- 

Re: [PATCH] Add comments to i386 assembly source

[Markus Wichmann]

On Sat, Dec 30, 2017 at 08:15:41PM -0800, John Reiser wrote:
> On 12/23/2017 09:45 UTC, Markus Wichmann wrote:
> 
> > But then there's i386. Without comments, and pulling off some very black
> > magic, I thought it would be worth commenting the files at least in the
> > threads directory.
> 
> > -	mov $120,%al
> > +	mov $120,%al    /* __NR_clone */
> 
> Using an actual symbol is clearer and easier to maintain or modify:
> +__NR_clone = 120
> +	mov $__NR_clone,%al
> 

Modify? Syscall numbers on i386? That's not happening while Linus still
draws breath.

> Constant arguments to system calls (including the system call number)
> should be loaded last in order to provide the least constraints for computing
> non-constant arguments.  Also, it is not obvious that as values (%eax == %al).
> The value in %eax was set by "xor %eax,%eax; ...; mov %gs,%ax; ...; shr $3,%eax";
> what guarantees that (%gs <= (255 << 3)) ?  %gs could be as high as (8191 << 3).

Ooh, boy, I hadn't considered that. GS is set in __set_thread_area. As
far as I know, it will usually be rather small, but it might not be. The
ABI certainly doesn't enforce it, set_thread_area() returns an int,
which is a GDT index. The platform also doesn't enforce it: The selector
has 13 bits of space for an index, and the GDTR limit portion can store
16 bits of space in bytes, where each GDT member is at least 8 bytes
large, so the GDT can have at most 2^13 entries.

I mean, alright, on Linux, a GDT with 20 elements is large, and musl
only uses a single TLS segment (Linux treats the GDT as thread-local),
but an implementation with one GDT to rule them all is certainly
thinkable. And then 255 segments will be gone in the blink of an eye, as
every process needs at least one TLS segment.

> So _that_ deserves a comment; else for safety all of %eax should be set:
> +	push $__NR_clone; pop %eax   /* 3 bytes; __NR_clone < 128 */

Wouldn't a register move avoid memory references and therefore be
faster? And clearer, since not everyone knows that "push byte" will
sign-extend the value to 32 bits ("push word" will not).

Unless you are at it with Fefe's mindset (not a single byte wasted!), I
don't see a reason for such a micro-op.

> +	int $128            /* clone(flags, stack, TID pointer, {.index = current gs index, .base = thread pointer, .limit=0xfffff, .seg32_bit, .limit_in_pages, .usable}, td pointer) */
> 
> Clarity can be improved by using a symbol:
> NBPW = 4  /* Number of Bytes Per Word */
> 	mov 3*NBPW(%ebp),%ecx  /* ecx = stack */
> 	mov 4*NBPW(%ebp),%ebx  /* ebx = flags */
> etc.

Most people can multiply and divide by four in their heads. At least
such small numbers. Don't know about the rest of the world, but in my
locality, we learn the small multiplication table in third grade.

My biggest problem with this stuff was actually to figure out where the
arguments started, and what they were.

> 
> Incorrect comment:
> > +	sub $16,%ecx        /* align stack */
> Perhaps you meant "/* allocate space for returned segment descriptor */"?
> The alignment is performed by:
>  	and $-4*NBPW,%ecx  /* align for stack */
> 

Ah, it's one line of difference. At the time I still thought that I was
right, though, because at the very start, ecx points to the first byte
behind the stack. Even after alignment, that might still be the case.

Of course, that isn't a problem, since any time the processor pushes
something on stack, it's

*--sp = argument;

So sp itself need not point to a stack, so long as "--sp" will point to
one.

Besides, the lines directly below the one you quoted already push the
thread descriptor pointer to the new thread's stack.

> If you are aiming for small space then
> +	mov %eax,%ebx       /* exit(rv from function) */
> can be implemented one byte smaller as:
> +	xchg %eax,%ebx  /* syscall arg %ebx = rv from function; %eax = do not care */
> 

Another micro-op. Personally, I don't see the need, but I do see the
confusion using an xchg in place of a mov might cause. But alright,
that's what comments are for.

And another thing: I merely commented the files as they were. I
explicitly did not want to change the source code itself, for fear of
getting something wrong.

That's why I only voiced my concern with using GS in the manner of this
function, but did not actually patch it. Would be easy enough, just look
if the L bit is set in GS, and if so, don't provide the CLONE_SETTLS bit
in flags. But then the child would have to call __set_thread_area again,
and I did not research if that happens. We might have had to
special-case it for i386, and that means, we'd have to call
__set_thread_area from the child in __clone. Possible, but ugly.

Ciao,
Markus

Re: [PATCH] Add comments to i386 assembly source

[Rich Felker]

The context of your reply was a bit confusing. Markus's patch is about
adding comments to code which he did not write but is trying to make
it easier to understand. Changes to the asm being commented are mostly
off-topic. If there are clear errors or inefficiencies found as part
of the commenting process, they may require additional patches, but
including any such changes (things that alter the machine code) in a
cosmetic patch would be reason for rejection. Cosmetic patches always
need to be machine-verifiable as cosmetic-only.

Now on to the actual review:

On Sat, Dec 30, 2017 at 08:15:41PM -0800, John Reiser wrote:
> On 12/23/2017 09:45 UTC, Markus Wichmann wrote:
> 
> >But then there's i386. Without comments, and pulling off some very black
> >magic, I thought it would be worth commenting the files at least in the
> >threads directory.
> 
> > -	mov $120,%al
> > +	mov $120,%al    /* __NR_clone */
> 
> Using an actual symbol is clearer and easier to maintain or modify:
> +__NR_clone = 120
> +	mov $__NR_clone,%al

The style in Markus's patch is what's preferred in musl. At first I
thought you were suggesting preprocessed asm, but now I see you're
using asm symbols/labels, which I suppose works but needs to be
considered for how it affects symbol tables (local only, I think, but
does affect output .o files) and whether it makes a compatibility
difference for existing or hypothetical new assemblers. In general I
don't like to enlarge the set of features we're relying on
unnecessarily.

The main time when I think symbolic constants have a strong benefit
over comments is when their value can change, which is not the case
here.

> Constant arguments to system calls (including the system call number)
> should be loaded last in order to provide the least constraints for computing
> non-constant arguments.  Also, it is not obvious that as values (%eax == %al).
> The value in %eax was set by "xor %eax,%eax; ...; mov %gs,%ax; ...; shr $3,%eax";
> what guarantees that (%gs <= (255 << 3)) ?  %gs could be as high as (8191 << 3).
> So _that_ deserves a comment; else for safety all of %eax should be set:
> +	push $__NR_clone; pop %eax   /* 3 bytes; __NR_clone < 128 */
> +	int $128            /* clone(flags, stack, TID pointer, {.index = current gs index, .base = thread pointer, .limit=0xfffff, .seg32_bit, .limit_in_pages, .usable}, td pointer) */

I don't follow your reasoning here. Where are you getting the possible
range of %gs from? If __clone is called with flags relevant to thread
creation, %gs is necessarily a GDT entry. The LDT stuff in i386's
__set_thread_area is only used to provide a working %gs for
single-threaded processes on ancient kernels that lack thread support;
in this case pthread_create always fails without calling __clone.

> Clarity can be improved by using a symbol:
> NBPW = 4  /* Number of Bytes Per Word */
> 	mov 3*NBPW(%ebp),%ecx  /* ecx = stack */
> 	mov 4*NBPW(%ebp),%ebx  /* ebx = flags */
> etc.

While there's an argument to be made that 3*4 and 4*4 should be used
here (I believe there are some files written that way) it's not done
consistently that way now, and it's readable either way.

> Incorrect comment:
> >+	sub $16,%ecx        /* align stack */
> Perhaps you meant "/* allocate space for returned segment descriptor */"?
> The alignment is performed by:
>  	and $-4*NBPW,%ecx  /* align for stack */

On a quick re-reading, the allocation seems to be to make space for
the argument to the start function in the new thread/process. It's
loaded from 20(%ebp) and stored at (%ecx) (the new stack) so that it
will get passed when the new thread/process executes the call insn
below.

Thank you and Markus for reminding me why some comments would help
here. :-)

> If you are aiming for small space then
> +	mov %eax,%ebx       /* exit(rv from function) */
> can be implemented one byte smaller as:
> +	xchg %eax,%ebx  /* syscall arg %ebx = rv from function; %eax = do not care */

I think this kind of change is just confusing and contrary to the
intent to make the code easier to understand. Saving a few bytes/insns
(bytes probably only if it reduced cache lines) in memset or memcpy
might be worthwhile but in a syscall it's pointless.

Rich

Re: [PATCH] Add comments to i386 assembly source

[Markus Wichmann]

On Sun, Dec 31, 2017 at 10:49:26AM -0500, Rich Felker wrote:
> I don't follow your reasoning here. Where are you getting the possible
> range of %gs from? If __clone is called with flags relevant to thread
> creation, %gs is necessarily a GDT entry. The LDT stuff in i386's
> __set_thread_area is only used to provide a working %gs for
> single-threaded processes on ancient kernels that lack thread support;
> in this case pthread_create always fails without calling __clone.
> 

First of all, happy new year everybody.

The range of GS is rather simple: GS is a word in Intel-lingo, i.e. 16
bits, so that sets its range. Nothing else limits it down any way
further. Not the interpretation of it (there are 13 bits for selector
index, so enough for 8192 selector entries), not GDT size (the GDTR can
hold a size of at most 65535 bytes, which is enough for 8192 selector
entries if each entry is 8 bytes), not the ABI (kernel returns GDT index
as int), nor the API (set_thread_area(2) is underdocumented, anyway).

Now, __set_thread_area() for i386 only makes sense if Linux has a
different GDT for each thread: In the entire code, you make sure to only
allocate a single TLS array index for the whole process runtime, which
would be nonsense if the GDT were distributed any further. In that case,
exceeding twenty segments would be quite a feat, never mind two hundred.

Also, the thing about the LDT not being used except on kernels that
don't have threading, that should be documented as well. Solves the
question I had in the OP of this thread nicely.

> Thank you and Markus for reminding me why some comments would help
> here. :-)
> 

You're welcome. :-)

Ciao,
Markus

Re: [PATCH] Add comments to i386 assembly source

[John Reiser]

There's a bug.  clone() is a user-level function that can be used
independently of the musl internal implementation of threads.
Thus when clone() in musl/src/linux/clone.c calls
         return __syscall_ret(__clone(func, stack, flags, arg, ptid, tls, ctid));
then the i386 implementation of __clone has no guarantee about
the value in %gs, and it is a bug to assume that (%gs >> 3)
fits in 8 bits.

The code in musl/src/thread/i386/clone.s wastes up to 12 bytes
when aligning the new stack, by aligning before [pre-]allocating
space for the one argument to the thread function.

This code fixes the %gs bug, wastes no stack space in the new thread,
and is 6 bytes smaller (83 ==> 77; -7.2%):

===== musl/src/thread/i386/clone.s
__NR_clone = 120
NBPW = 4  /* Number of Bytes Per Word */

.text
.global __clone
.type   __clone,@function

__clone:  /* clone(func, stack, flags, arg, ptid, tls, ctid) */
	push %esi  /* non-standard; save .text space */
	lea 2*NBPW(%esp),%esi  /* &func */
	push %ebx
	push %ebp
	push %edi

/* 'cld' must be in effect upon entry to a .globl function */
	lodsl; xchg %eax,%ebp  /* func (save) */
	lodsl; lea -NBPW(%eax),%ecx  /* stack; pre-allocate space for 1 arg */
	lodsl; xchg %eax,%ebx  /* flags */
		and $-16,%ecx  /* 16-byte align new stack */
	lodsl; mov %eax,(%ecx)  /* arg to new thread */
	lodsl; xchg %eax,%edx  /* ptid */

	push $0x51  /* flags */
	push $0xffff  /* limit */
	lodsl; push %eax  /* tls */
	xor %eax,%eax; mov %gs,%ax; shr $3,%eax; push %eax  /* segment # */

	mov (%esi),%edi  /* ctid */
	mov %esp,%esi  /* &segment_descriptor on current stack */
	push $__NR_clone; pop %eax
	int $128
	test %eax,%eax
	jnz 1f

	mov %ebp,%eax  /* func */
	xor %ebp,%ebp  /* end chain of stack frames */
	call *%eax  /* func(arg) */
	mov %eax,%ebx  /* rv is arg1 to syscall */
	xor %eax,%eax
	inc %eax  /* __NR_exit */
	int $128
	hlt

1:	add $16,%esp
	pop %edi
	pop %ebp
	pop %ebx
	pop %esi
	ret
=====

-- 

Re: [PATCH] Add comments to i386 assembly source

[Rich Felker]

On Mon, Jan 01, 2018 at 02:57:02PM -0800, John Reiser wrote:
> There's a bug.  clone() is a user-level function that can be used
> independently of the musl internal implementation of threads.
> Thus when clone() in musl/src/linux/clone.c calls
>         return __syscall_ret(__clone(func, stack, flags, arg, ptid, tls, ctid));
> then the i386 implementation of __clone has no guarantee about
> the value in %gs, and it is a bug to assume that (%gs >> 3)
> fits in 8 bits.

The ABI is that at function call or any time a signal could be
received, %gs must always be a valid segment register value reflecting
the current thread's thread pointer. If this is violated, the program
has undefined behavior.

> The code in musl/src/thread/i386/clone.s wastes up to 12 bytes
> when aligning the new stack, by aligning before [pre-]allocating
> space for the one argument to the thread function.

I suspect the initial value happens to be aligned anyway in which case
reserving 16 bytes and aligning to 16 is the same as reserving 4 and
aligning to 16. If you think it's not, I don't mind changing if you
can do careful testing to make sure it doesn't introduce any bugs.

Rich

Re: [PATCH] Add comments to i386 assembly source

[John Reiser]

On 01/01/2018 13:49 UTC, Rich Felker wrote:
> On Mon, Jan 01, 2018 at 02:57:02PM -0800, John Reiser wrote:
>> There's a bug.  clone() is a user-level function that can be used
>> independently of the musl internal implementation of threads.
>> Thus when clone() in musl/src/linux/clone.c calls
>>          return __syscall_ret(__clone(func, stack, flags, arg, ptid, tls, ctid));
>> then the i386 implementation of __clone has no guarantee about
>> the value in %gs, and it is a bug to assume that (%gs >> 3)
>> fits in 8 bits.
> 
> The ABI is that at function call or any time a signal could be
> received, %gs must always be a valid segment register value reflecting
> the current thread's thread pointer. If this is violated, the program
> has undefined behavior.

More than one segment descriptor can designate the same subset
of the linear address space.  Duplicate the segment descriptor
to a target selector that is >= 256, and load %gs with the
duplicate selector before calling clone().

> 
>> The code in musl/src/thread/i386/clone.s wastes up to 12 bytes
>> when aligning the new stack, by aligning before [pre-]allocating
>> space for the one argument to the thread function.
> 
> I suspect the initial value happens to be aligned anyway in which case
> reserving 16 bytes and aligning to 16 is the same as reserving 4 and
> aligning to 16. If you think it's not, I don't mind changing if you
> can do careful testing to make sure it doesn't introduce any bugs.

This is another bug!  Consider the valid code:
	void **lo_stack = malloc(5 * sizeof(void *));
	/* malloc() guarantees 16-byte alignment of lo_stack */
	clone(func, &lo_stack[5], ...);

then __clone() does:
	and $-16,%ecx  /* &lo_stack[4] */
	sub $ 16,%ecx  /* &lo_stack[0] */
	  ...
	mov %ecx,%esp  /* new thread: implicit action of ___NR_clone system call */
	call *%eax  /* OUT-OF-BOUNDS:  lo_stack[-1] = return address */

Thus, starting the thread function has scribbled outside the allocated area,
even though the lo_stack[] array can accommodate the call by the code I showed:
	lea -NBPW(arg2),%ecx  /* &lo_stack[4] */
	and $-16,%ecx  /* still &lo_stack[4] */
	  ...
	mov %ecx,%esp  /* new thread: implicit action of __NR_clone system call */
	call *%eax  /* lo_stack[3] = return address */

The danger is not "new bugs", but rather revealing latent bugs that were
obscured by the less-strict old code.  For instance, if the thread
function actually has two formal parameters, or if it uses va_arg()
to reference beyond the first actual argument, then running the optimal
code is more likely to notice.

-- 

Re: [PATCH] Add comments to i386 assembly source

[Rich Felker]

On Mon, Jan 01, 2018 at 07:15:50PM -0800, John Reiser wrote:
> On 01/01/2018 13:49 UTC, Rich Felker wrote:
> >On Mon, Jan 01, 2018 at 02:57:02PM -0800, John Reiser wrote:
> >>There's a bug.  clone() is a user-level function that can be used
> >>independently of the musl internal implementation of threads.
> >>Thus when clone() in musl/src/linux/clone.c calls
> >>         return __syscall_ret(__clone(func, stack, flags, arg, ptid, tls, ctid));
> >>then the i386 implementation of __clone has no guarantee about
> >>the value in %gs, and it is a bug to assume that (%gs >> 3)
> >>fits in 8 bits.
> >
> >The ABI is that at function call or any time a signal could be
> >received, %gs must always be a valid segment register value reflecting
> >the current thread's thread pointer. If this is violated, the program
> >has undefined behavior.
> 
> More than one segment descriptor can designate the same subset
> of the linear address space.  Duplicate the segment descriptor
> to a target selector that is >= 256, and load %gs with the
> duplicate selector before calling clone().

It's not clear to me that such a substition is valid; as far as I can
tell no explicit effort to ensure that it works is made, and it would
not happen without writing asm to do specifically that.

> >>The code in musl/src/thread/i386/clone.s wastes up to 12 bytes
> >>when aligning the new stack, by aligning before [pre-]allocating
> >>space for the one argument to the thread function.
> >
> >I suspect the initial value happens to be aligned anyway in which case
> >reserving 16 bytes and aligning to 16 is the same as reserving 4 and
> >aligning to 16. If you think it's not, I don't mind changing if you
> >can do careful testing to make sure it doesn't introduce any bugs.
> 
> This is another bug!  Consider the valid code:
> 	void **lo_stack = malloc(5 * sizeof(void *));
> 	/* malloc() guarantees 16-byte alignment of lo_stack */
> 	clone(func, &lo_stack[5], ...);

You can't run code on a 20-byte stack. This is not a surprise. In
theory it might be possible if the callee is only asm, but you can't
make C function calls since each call frame will consume at least 16
bytes (return address and alignment). I also disagree with considering
it valid to assume clone invokes the provided callback function
directly with no intervening functions; this is incorrect on SH right
now since we use a C function to smooth over the difference between
plain and fdpic calling conventions. ARM will probably do the same
once fdpic for cortex-M is added.

> then __clone() does:
> 	and $-16,%ecx  /* &lo_stack[4] */
> 	sub $ 16,%ecx  /* &lo_stack[0] */
> 	  ...
> 	mov %ecx,%esp  /* new thread: implicit action of ___NR_clone system call */
> 	call *%eax  /* OUT-OF-BOUNDS:  lo_stack[-1] = return address */
> 
> Thus, starting the thread function has scribbled outside the allocated area,
> even though the lo_stack[] array can accommodate the call by the code I showed:
> 	lea -NBPW(arg2),%ecx  /* &lo_stack[4] */
> 	and $-16,%ecx  /* still &lo_stack[4] */
> 	  ...
> 	mov %ecx,%esp  /* new thread: implicit action of __NR_clone system call */
> 	call *%eax  /* lo_stack[3] = return address */
> 
> The danger is not "new bugs", but rather revealing latent bugs that were
> obscured by the less-strict old code.  For instance, if the thread
> function actually has two formal parameters, or if it uses va_arg()
> to reference beyond the first actual argument, then running the optimal
> code is more likely to notice.

I agree with your analysis of what happens but I don't think it's
particularly interesting or a bug.

Rich

a third bug in musl clone()

[John Reiser]

In addition to the bugs in __clone for i386 with %gs and stack alignment
in the new thread, there is a third bug in musl's implementation of clone().
clone() takes optional arguments that need not be present,
yet musl/src/linux/clone.c fetches them anyway.  This can cause SIGSEGV.

===== musl/src/linux/clone.c excerpts
int clone(int (*func)(void *), void *stack, int flags, void *arg, ...)
{
    [[snip]]
         va_start(ap, arg);
         ptid = va_arg(ap, pid_t *);
         tls  = va_arg(ap, void *);
         ctid = va_arg(ap, pid_t *);
         va_end(ap);

         return __syscall_ret(__clone(func, stack, flags, arg, ptid, tls, ctid));
}
=====
The presence of ptid, tls, and ctid is indicated by bits in 'flags':
CLONE_PARENT_SETTID, CLONE_SETTLS, CLONE_CHILD_SETTID/CLONE_CHILD_CLEARTID.
If none of those bits are set, then it could be that none of the variable
arguments are present; therefore none of them should be fetched, and 0 (NULL)
should be passed to __clone() for each of ptid, tls, ctid.
[The meaning is unclear if any omitted argument is followed by an argument
that is flagged as present.  Should the implementation call the corresponding
va_arg(), or skip over it?]

How SIGSEGV can be generated: It is valid for &arg to be the address of
the last word on a hardware page: 0x...ffc on a 32-bit CPU with 4KiB pages,
with the following page unmapped.  &func would be 16-byte aligned at 0x...ff0.
Any one of the va_arg() calls would attempt to fetch from the next
page at address 0x...000 or greater, which will generate SIGSEGV.

If the implementer of clone() actually thought about this then it
is MANDATORY to insert a code comment about the situation.  Such as:
    /* Always fetch ptid,tls,ctid even though the official standard
     * says that they are va_arg.  The standard says that only
     * to allow compilation of [old] calls that omit un-flagged
     * trailing arguments.  In practice it is highly unreasonable
     * to require conditional fetching; even the meaning is unclear.
     * In the extremely unlikely case that &arg is near a "hole"
     * in the address space, then we will suffer SIGSEGV.
     */

-- 

Re: a third bug in musl clone()

[Rich Felker]

On Tue, Jan 02, 2018 at 10:24:42AM -0800, John Reiser wrote:
> In addition to the bugs in __clone for i386 with %gs and stack alignment
> in the new thread, there is a third bug in musl's implementation of clone().
> clone() takes optional arguments that need not be present,
> yet musl/src/linux/clone.c fetches them anyway.  This can cause SIGSEGV.
> 
> ===== musl/src/linux/clone.c excerpts
> int clone(int (*func)(void *), void *stack, int flags, void *arg, ...)
> {
>    [[snip]]
>         va_start(ap, arg);
>         ptid = va_arg(ap, pid_t *);
>         tls  = va_arg(ap, void *);
>         ctid = va_arg(ap, pid_t *);
>         va_end(ap);
> 
>         return __syscall_ret(__clone(func, stack, flags, arg, ptid, tls, ctid));
> }
> =====
> The presence of ptid, tls, and ctid is indicated by bits in 'flags':
> CLONE_PARENT_SETTID, CLONE_SETTLS, CLONE_CHILD_SETTID/CLONE_CHILD_CLEARTID.
> If none of those bits are set, then it could be that none of the variable
> arguments are present; therefore none of them should be fetched, and 0 (NULL)
> should be passed to __clone() for each of ptid, tls, ctid.
> [The meaning is unclear if any omitted argument is followed by an argument
> that is flagged as present.  Should the implementation call the corresponding
> va_arg(), or skip over it?]
> 
> How SIGSEGV can be generated: It is valid for &arg to be the address of
> the last word on a hardware page: 0x...ffc on a 32-bit CPU with 4KiB pages,
> with the following page unmapped.  &func would be 16-byte aligned at 0x...ff0.
> Any one of the va_arg() calls would attempt to fetch from the next
> page at address 0x...000 or greater, which will generate SIGSEGV.

This is undefined behavior and should be corrected, but I disagree
with your claimed mechanism of failure. clone() is necessarily called
by some other function, which has its own call frame, yielding a worst
case of:

0. clone's return address (aligned -4 mod 16)
4. func
8. stack
12. flags
16. arg
20. ??
24. ??
28. ??
32. caller's return address (aligned -4 mod 16)

In any case it should be fixed by checking flags.

As an aside, I don't think there are any valid (supportable) ways to
use the variadic args anyway (at least the tls slot is not usable;
maybe the tids are?), so it's possible that we should just remove
support for them instead. Code that wants to do really wacky things
here needs to be written in 100% asm anyway since calls to C from a
pseudo-thread created by manual calls to clone will violate the ABI
(invalid or duplicate thread pointer) and if you're writing asm for
something special you should just use SYS_clone yourself...

Rich

Re: a third bug in musl clone()

[Florian Weimer]

On 01/02/2018 08:58 PM, Rich Felker wrote:
> In any case it should be fixed by checking flags.

I think this would be worse than the cure because it results in subtle 
bugs if the kernel adds more flags which require different argument 
counts.  We saw that with O_TMPFILE and open/openat.

Thanks,
Florian

Re: a third bug in musl clone()

[Rich Felker]

On Tue, Jan 02, 2018 at 11:09:24PM +0100, Florian Weimer wrote:
> On 01/02/2018 08:58 PM, Rich Felker wrote:
> >In any case it should be fixed by checking flags.
> 
> I think this would be worse than the cure because it results in
> subtle bugs if the kernel adds more flags which require different
> argument counts.  We saw that with O_TMPFILE and open/openat.

Wasn't that just a bug with O_TMPFILE having implicit O_CREAT but not
having the actual O_CREAT bit set in its value? I understand the
sentiment here but I think if we're really worried about that we could
just fail with EINVAL for unknown flags (requiring a sufficiently new
libc.so to be aware of the flags) rather than leaving the UB in place.

Rich