Patch: Negative stack pointer references

Patch: Negative stack pointer references

[Markus Wichmann]

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

Hi all,

I found a few instances in musl where negative stack pointer offsets
were used in the handwritten assembly. That is problematic, because if a
signal arrives and is handled during the time that scratch space is in
use (unlikely but possible), and sigaltstack() is not used, then that
scratch space will be overwritten.

This was just something I saw while randomly reading the code. Also, I
only searched for the error pattern using a regex, so the problem may
persist with (possibly) negative nonconstant offsets to the stack
pointer, use of the same space with another register as base, or use of
a negative constant offset my regex failed to match. I searched for

-\d\+(%[er]sp)
-0[xX]\x\+(%[er]sp)

in all .s, .c, and .h files.

Also, the problem may exist in architectures other than AMD64 or x32.

I'm not a subscriber, so please CC me in this thread.

Ciao,
Markus

[-- Attachment #2: 0033-Remove-negative-constant-stack-pointer-offsets.patch --]
[-- Type: text/x-diff, Size: 3845 bytes --]

From d06bafc3d6654b9b26387b66d0dab4c5d21d02ea Mon Sep 17 00:00:00 2001
From: Markus Wichmann <nullplan@gmx.net>
Date: Fri, 25 Dec 2015 21:37:00 +0100
Subject: [PATCH 33/33] Remove negative constant stack pointer offsets.

Some places used constant negative stack pointer offsets for scratch
space. In theory sound, but if a signal arrives during such usage and it
is handled and sigaltstack() is not in use, the stack gets clobbered and
the scratch space overwritten. Since that can happen at any time,
negative stack pointer offsets should be avoided at all cost.

Note that I only used a regex to find these instances. There might be
negative nonconstant offsets used somewhere, negative constant offsets
used in a way my regex didn't find, and the same pattern in any
architecture other than x32 and x86_64.
---
 src/fenv/x32/fenv.s     | 22 +++++++++++++---------
 src/fenv/x86_64/fenv.s  | 22 +++++++++++++---------
 src/math/x32/exp2l.s    |  6 ++++--
 src/math/x86_64/exp2l.s |  6 ++++--
 4 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/src/fenv/x32/fenv.s b/src/fenv/x32/fenv.s
index 4531046..d0de0a1 100644
--- a/src/fenv/x32/fenv.s
+++ b/src/fenv/x32/fenv.s
@@ -1,32 +1,36 @@
 .global feclearexcept
 .type feclearexcept,@function
 feclearexcept:
-		# maintain exceptions in the sse mxcsr, clear x87 exceptions
+	# maintain exceptions in the sse mxcsr, clear x87 exceptions
+        subl $8, %esp
 	mov %edi,%ecx
 	and $0x3f,%ecx
 	fnstsw %ax
 	test %eax,%ecx
 	jz 1f
 	fnclex
-1:	stmxcsr -8(%esp)
+1:	stmxcsr (%esp)
 	and $0x3f,%eax
-	or %eax,-8(%esp)
-	test %ecx,-8(%esp)
+	or %eax,(%esp)
+	test %ecx,(%esp)
 	jz 1f
 	not %ecx
-	and %ecx,-8(%esp)
-	ldmxcsr -8(%esp)
+	and %ecx,(%esp)
+	ldmxcsr (%esp)
 1:	xor %eax,%eax
+        addl $8, %esp
 	ret
 
 .global feraiseexcept
 .type feraiseexcept,@function
 feraiseexcept:
+        subl $8, %esp
 	and $0x3f,%edi
-	stmxcsr -8(%esp)
-	or %edi,-8(%esp)
-	ldmxcsr -8(%esp)
+	stmxcsr (%esp)
+	or %edi,(%esp)
+	ldmxcsr (%esp)
 	xor %eax,%eax
+        addl $8, %esp
 	ret
 
 .global __fesetround
diff --git a/src/fenv/x86_64/fenv.s b/src/fenv/x86_64/fenv.s
index b5aeaf4..5e02716 100644
--- a/src/fenv/x86_64/fenv.s
+++ b/src/fenv/x86_64/fenv.s
@@ -1,32 +1,36 @@
 .global feclearexcept
 .type feclearexcept,@function
 feclearexcept:
-		# maintain exceptions in the sse mxcsr, clear x87 exceptions
+	# maintain exceptions in the sse mxcsr, clear x87 exceptions
+        subq $8, %rsp
 	mov %edi,%ecx
 	and $0x3f,%ecx
 	fnstsw %ax
 	test %eax,%ecx
 	jz 1f
 	fnclex
-1:	stmxcsr -8(%rsp)
+1:	stmxcsr (%rsp)
 	and $0x3f,%eax
-	or %eax,-8(%rsp)
-	test %ecx,-8(%rsp)
+	or %eax,(%rsp)
+	test %ecx,(%rsp)
 	jz 1f
 	not %ecx
-	and %ecx,-8(%rsp)
-	ldmxcsr -8(%rsp)
+	and %ecx,(%rsp)
+	ldmxcsr (%rsp)
 1:	xor %eax,%eax
+        addq $8, %rsp
 	ret
 
 .global feraiseexcept
 .type feraiseexcept,@function
 feraiseexcept:
+        subq $8, %rsp
 	and $0x3f,%edi
-	stmxcsr -8(%rsp)
-	or %edi,-8(%rsp)
-	ldmxcsr -8(%rsp)
+	stmxcsr (%rsp)
+	or %edi,(%rsp)
+	ldmxcsr (%rsp)
 	xor %eax,%eax
+        addq $8, %rsp
 	ret
 
 .global __fesetround
diff --git a/src/math/x32/exp2l.s b/src/math/x32/exp2l.s
index e9edb96..2935942 100644
--- a/src/math/x32/exp2l.s
+++ b/src/math/x32/exp2l.s
@@ -4,8 +4,10 @@ expm1l:
 	fldt 8(%esp)
 	fldl2e
 	fmulp
-	movl $0xc2820000,-4(%esp)
-	flds -4(%esp)
+        subl $4, %esp
+	movl $0xc2820000,(%esp)
+	flds (%esp)
+        addl $4, %esp
 	fucomip %st(1),%st
 	fld1
 	jb 1f
diff --git a/src/math/x86_64/exp2l.s b/src/math/x86_64/exp2l.s
index effab2b..e7e7d9e 100644
--- a/src/math/x86_64/exp2l.s
+++ b/src/math/x86_64/exp2l.s
@@ -4,8 +4,10 @@ expm1l:
 	fldt 8(%rsp)
 	fldl2e
 	fmulp
-	movl $0xc2820000,-4(%rsp)
-	flds -4(%rsp)
+        subq $4, %rsp
+	movl $0xc2820000,(%rsp)
+	flds (%rsp)
+        addq $4, %rsp
 	fucomip %st(1),%st
 	fld1
 	jb 1f
-- 
2.1.4

Re: Patch: Negative stack pointer references

[Rich Felker]

On Fri, Dec 25, 2015 at 09:57:34PM +0100, Markus Wichmann wrote:
> Hi all,
> 
> I found a few instances in musl where negative stack pointer offsets
> were used in the handwritten assembly. That is problematic, because if a
> signal arrives and is handled during the time that scratch space is in
> use (unlikely but possible), and sigaltstack() is not used, then that
> scratch space will be overwritten.
> 
> This was just something I saw while randomly reading the code. Also, I
> only searched for the error pattern using a regex, so the problem may
> persist with (possibly) negative nonconstant offsets to the stack
> pointer, use of the same space with another register as base, or use of
> a negative constant offset my regex failed to match. I searched for
> 
> -\d\+(%[er]sp)
> -0[xX]\x\+(%[er]sp)
> 
> in all .s, .c, and .h files.
> 
> Also, the problem may exist in architectures other than AMD64 or x32.
> 
> I'm not a subscriber, so please CC me in this thread.

This is intentional and is safe. Google "x86_64 red zone".

Rich