AArch64 ILP32 strcmp bug

AArch64 ILP32 strcmp bug

[Kinsey Moore]

Hi,
It appears that the hand-coded assembly in AArch64 strcmp does not sanitize the incoming address parameters in x0 and x1 when compiled for AArch64 ILP32. Based on my reading of the AArch64 Procedure Call Specification and GCC's output for similar function signatures, the callee is responsible for sanitization of the pointer addresses. I encountered this because I have a struct containing a pointer and length returned from another function that happens to get packed into a single register (x0) and GCC passes this unmodified into strcmp as the first argument.

According to the aapcs64: "Any part of a register or a stack slot that is not used for an argument (padding bits) has unspecified content at the callee entry point."

I suspect this is a problem for the majority of hand-written AArch64 assembly in newlib.

Please let me know if I missed something.

Thanks,
Kinsey Moore

Re: AArch64 ILP32 strcmp bug

[Richard Earnshaw]

On 20/07/2020 14:52, Kinsey Moore wrote:
> Hi,
> It appears that the hand-coded assembly in AArch64 strcmp does not sanitize the incoming address parameters in x0 and x1 when compiled for AArch64 ILP32. Based on my reading of the AArch64 Procedure Call Specification and GCC's output for similar function signatures, the callee is responsible for sanitization of the pointer addresses. I encountered this because I have a struct containing a pointer and length returned from another function that happens to get packed into a single register (x0) and GCC passes this unmodified into strcmp as the first argument.
> 
> According to the aapcs64: "Any part of a register or a stack slot that is not used for an argument (padding bits) has unspecified content at the callee entry point."
> 
> I suspect this is a problem for the majority of hand-written AArch64 assembly in newlib.
> 
> Please let me know if I missed something.
> 
> Thanks,
> Kinsey Moore
> 

Apologies, somehow this message got marked as read although it never
received a response.

I don't think we've really added support for ILP32 to newlib.  This may
just be one corner of a fairly large can of worms.

The Arm/AArch64 optimized assembly routines are really just copies
(provided that they've been kept up-to-date) of the code that Arm
publishes as part of its Arm Optimized Routines package
(https://github.com/ARM-software/optimized-routines); but even those do
not have ILP32 support at this time.  The best place to address this is
to raise an issue there.

R.

Re: AArch64 ILP32 strcmp bug

[Joel Sherrill]

Thanks Richard!

On Wed, Nov 25, 2020 at 5:29 AM Richard Earnshaw via Newlib <
newlib@sourceware.org> wrote:

> On 20/07/2020 14:52, Kinsey Moore wrote:
> > Hi,
> > It appears that the hand-coded assembly in AArch64 strcmp does not
> sanitize the incoming address parameters in x0 and x1 when compiled for
> AArch64 ILP32. Based on my reading of the AArch64 Procedure Call
> Specification and GCC's output for similar function signatures, the callee
> is responsible for sanitization of the pointer addresses. I encountered
> this because I have a struct containing a pointer and length returned from
> another function that happens to get packed into a single register (x0) and
> GCC passes this unmodified into strcmp as the first argument.
> >
> > According to the aapcs64: "Any part of a register or a stack slot that
> is not used for an argument (padding bits) has unspecified content at the
> callee entry point."
> >
> > I suspect this is a problem for the majority of hand-written AArch64
> assembly in newlib.
> >
> > Please let me know if I missed something.
> >
> > Thanks,
> > Kinsey Moore
> >
>
> Apologies, somehow this message got marked as read although it never
> received a response.
>
> I don't think we've really added support for ILP32 to newlib.  This may
> just be one corner of a fairly large can of worms.
>
> The Arm/AArch64 optimized assembly routines are really just copies
> (provided that they've been kept up-to-date) of the code that Arm
> publishes as part of its Arm Optimized Routines package
> (https://github.com/ARM-software/optimized-routines); but even those do
> not have ILP32 support at this time.  The best place to address this is
> to raise an issue there.
>

I'm sure Kinsey will do that and look into providing a bare
metal test case that fails now.

In the meantime, would you think a patch to disable the optimized
method when ilp32 is appropriate for newlib? There is still the risk of
other methods having bugs. The alternative I see is to completely
PREFER_SIZE_OVER_SPEED for aarch64 and disable all of the
aarch64 assembly which seems worse.

Since this isn't RTEMS specific, I'd like to find a temporary workaround
that avoids the issue for all aarch64 targets.

Thanks.

--joel

R.
>

RE: AArch64 ILP32 strcmp bug

[Kinsey Moore]

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

-----Original Message-----
From: Richard Earnshaw <Richard.Earnshaw@foss.arm.com> 
Sent: Wednesday, November 25, 2020 05:29
To: Kinsey Moore <kinsey.moore@oarcorp.com>; newlib@sourceware.org
Subject: Re: AArch64 ILP32 strcmp bug

>On 20/07/2020 14:52, Kinsey Moore wrote:
>> Hi,
>> It appears that the hand-coded assembly in AArch64 strcmp does not sanitize the incoming address parameters in x0 and x1 when compiled for AArch64 ILP32. Based on my reading of the AArch64 Procedure Call Specification and GCC's output for similar function signatures, the callee is responsible for sanitization of the pointer addresses. I encountered this because I have a struct containing a pointer and length returned from another function that happens to get packed into a single register (x0) and GCC passes this unmodified into strcmp as the first argument.
>> 
>> According to the aapcs64: "Any part of a register or a stack slot that is not used for an argument (padding bits) has unspecified content at the callee entry point."
>> 
>> I suspect this is a problem for the majority of hand-written AArch64 assembly in newlib.
>> 
>> Please let me know if I missed something.
>
> Apologies, somehow this message got marked as read although it never received a response.
>
> I don't think we've really added support for ILP32 to newlib.  This may just be one corner of a fairly large can of worms.
> 
> The Arm/AArch64 optimized assembly routines are really just copies (provided that they've been kept up-to-date) of the code that Arm publishes as part of its Arm Optimized Routines package (https://github.com/ARM-software/optimized-routines); but even those do not have ILP32 support at this time.  The best place to address this is to raise an issue there.

Thanks for taking a look at this. I did see some flags related to ILP32 elsewhere in newlib which made me think it was supported but, as you mention, none of the assembly is setup for it. In the interim, I've been working around this by adding -DPREFER_SIZE_OVER_SPEED to the newlib CFLAGS to force use of compiled C sources instead of hand-written assembly. The patch to automatically specify this for RTEMS is attached, but I suspect that this may be applicable for all AArch64 targets that wish to take advantage of ILP32. I'll create an issue upstream.

Kinsey

[-- Attachment #2: newlib-ilp32.patch --]
[-- Type: application/octet-stream, Size: 945 bytes --]

--- a/newlib/configure.host	2020-11-25 08:38:24.751655619 -0600
+++ b/newlib/configure.host	2020-11-25 08:54:25.911655619 -0600
@@ -664,6 +664,16 @@
 newlib_cflags="${newlib_cflags} -DCLOCK_PROVIDED -DMALLOC_PROVIDED -DEXIT_PROVIDED -DSIGNAL_PROVIDED -DGETREENT_PROVIDED -DREENTRANT_SYSCALLS_PROVIDED -DHAVE_NANOSLEEP -DHAVE_BLKSIZE -DHAVE_FCNTL -DHAVE_ASSERT_FUNC"
         # turn off unsupported items in posix directory 
 	newlib_cflags="${newlib_cflags} -D_NO_GETLOGIN -D_NO_GETPWENT -D_NO_GETUT -D_NO_GETPASS -D_NO_SIGSET -D_NO_WORDEXP -D_NO_POPEN -D_NO_POSIX_SPAWN"
+
+	# The hand-written assembly in newlib does not currently support AArch64/ILP32 ABI
+	# Build from C source instead
+	case "${host}" in
+		aarch64*-*-rtems*)
+			newlib_cflags="${newlib_cflags} -DPREFER_SIZE_OVER_SPEED"
+			;;
+		*)
+			;;
+	esac
 	;;
 # VxWorks supplies its own version of malloc, and the newlib one
 # doesn't work because VxWorks does not have sbrk.

Re: AArch64 ILP32 strcmp bug

[Richard Earnshaw]

On 25/11/2020 17:31, Kinsey Moore wrote:
> -----Original Message-----
> From: Richard Earnshaw <Richard.Earnshaw@foss.arm.com> 
> Sent: Wednesday, November 25, 2020 05:29
> To: Kinsey Moore <kinsey.moore@oarcorp.com>; newlib@sourceware.org
> Subject: Re: AArch64 ILP32 strcmp bug
> 
>> On 20/07/2020 14:52, Kinsey Moore wrote:
>>> Hi,
>>> It appears that the hand-coded assembly in AArch64 strcmp does not sanitize the incoming address parameters in x0 and x1 when compiled for AArch64 ILP32. Based on my reading of the AArch64 Procedure Call Specification and GCC's output for similar function signatures, the callee is responsible for sanitization of the pointer addresses. I encountered this because I have a struct containing a pointer and length returned from another function that happens to get packed into a single register (x0) and GCC passes this unmodified into strcmp as the first argument.
>>>
>>> According to the aapcs64: "Any part of a register or a stack slot that is not used for an argument (padding bits) has unspecified content at the callee entry point."
>>>
>>> I suspect this is a problem for the majority of hand-written AArch64 assembly in newlib.
>>>
>>> Please let me know if I missed something.
>>
>> Apologies, somehow this message got marked as read although it never received a response.
>>
>> I don't think we've really added support for ILP32 to newlib.  This may just be one corner of a fairly large can of worms.
>>
>> The Arm/AArch64 optimized assembly routines are really just copies (provided that they've been kept up-to-date) of the code that Arm publishes as part of its Arm Optimized Routines package (https://github.com/ARM-software/optimized-routines); but even those do not have ILP32 support at this time.  The best place to address this is to raise an issue there.
> 
> Thanks for taking a look at this. I did see some flags related to ILP32 elsewhere in newlib which made me think it was supported but, as you mention, none of the assembly is setup for it. In the interim, I've been working around this by adding -DPREFER_SIZE_OVER_SPEED to the newlib CFLAGS to force use of compiled C sources instead of hand-written assembly. The patch to automatically specify this for RTEMS is attached, but I suspect that this may be applicable for all AArch64 targets that wish to take advantage of ILP32. I'll create an issue upstream.
> 
> Kinsey
> 

+
+	# The hand-written assembly in newlib does not currently support
AArch64/ILP32 ABI
+	# Build from C source instead
+	case "${host}" in
+		aarch64*-*-rtems*)
+			newlib_cflags="${newlib_cflags} -DPREFER_SIZE_OVER_SPEED"
+			;;
+		*)
+			;;
+	esac

I have a number of issues with that:
1) It's specific to rtems - what about other users?  There really should
be a way of detecting that ilp32 is being built so that it works for all.
2) It assumes that rtems will only be used for ILP32 (or that even if
you don't have ILP32 you'll get the small rather than the optimal code,
some of which is very sub-optimal).
3) It's very poorly targeted, there may well be other places in the code
that this affects and also no guarantee that it will prevent use of
assembler functions in all cases.

R.

Re: AArch64 ILP32 strcmp bug

[Keith Packard]

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

Joel Sherrill via Newlib <newlib@sourceware.org> writes:

> In the meantime, would you think a patch to disable the optimized
> method when ilp32 is appropriate for newlib? There is still the risk of
> other methods having bugs. The alternative I see is to completely
> PREFER_SIZE_OVER_SPEED for aarch64 and disable all of the
> aarch64 assembly which seems worse.

There's also setjmp/longjmp, which are only available in assembly form.

Here's a completely untested patch (I'm afraid I don't have an ilp32
aarch64 compiler available today) which may help with that:

diff --git a/newlib/libc/machine/aarch64/setjmp.S b/newlib/libc/machine/aarch64/setjmp.S
index 0856145bf..df94eebd1 100644
--- a/newlib/libc/machine/aarch64/setjmp.S
+++ b/newlib/libc/machine/aarch64/setjmp.S
@@ -45,6 +45,9 @@
 	.global	setjmp
 	.type	setjmp, %function
 setjmp:
+#ifndef __LP64__
+	mov	w0, w0
+#endif
 	mov	x16, sp
 #define REG_PAIR(REG1, REG2, OFFS)	stp REG1, REG2, [x0, OFFS]
 #define REG_ONE(REG1, OFFS)		str REG1, [x0, OFFS]
@@ -60,6 +63,10 @@ setjmp:
 	.global	longjmp
 	.type	longjmp, %function
 longjmp:
+#ifndef __LP64__
+	mov	w0, w0
+#endif
+	
 #define REG_PAIR(REG1, REG2, OFFS)	ldp REG1, REG2, [x0, OFFS]
 #define REG_ONE(REG1, OFFS)		ldr REG1, [x0, OFFS]
 	GPR_LAYOUT

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: AArch64 ILP32 strcmp bug

[Richard Earnshaw]

On 26/11/2020 18:41, Keith Packard wrote:
> Joel Sherrill via Newlib <newlib@sourceware.org> writes:
> 
>> In the meantime, would you think a patch to disable the optimized
>> method when ilp32 is appropriate for newlib? There is still the risk of
>> other methods having bugs. The alternative I see is to completely
>> PREFER_SIZE_OVER_SPEED for aarch64 and disable all of the
>> aarch64 assembly which seems worse.
> 
> There's also setjmp/longjmp, which are only available in assembly form.
> 
> Here's a completely untested patch (I'm afraid I don't have an ilp32
> aarch64 compiler available today) which may help with that:
> 
> diff --git a/newlib/libc/machine/aarch64/setjmp.S b/newlib/libc/machine/aarch64/setjmp.S
> index 0856145bf..df94eebd1 100644
> --- a/newlib/libc/machine/aarch64/setjmp.S
> +++ b/newlib/libc/machine/aarch64/setjmp.S
> @@ -45,6 +45,9 @@
>  	.global	setjmp
>  	.type	setjmp, %function
>  setjmp:
> +#ifndef __LP64__
> +	mov	w0, w0
> +#endif
>  	mov	x16, sp
>  #define REG_PAIR(REG1, REG2, OFFS)	stp REG1, REG2, [x0, OFFS]
>  #define REG_ONE(REG1, OFFS)		str REG1, [x0, OFFS]
> @@ -60,6 +63,10 @@ setjmp:
>  	.global	longjmp
>  	.type	longjmp, %function
>  longjmp:
> +#ifndef __LP64__
> +	mov	w0, w0
> +#endif
> +	
>  #define REG_PAIR(REG1, REG2, OFFS)	ldp REG1, REG2, [x0, OFFS]
>  #define REG_ONE(REG1, OFFS)		ldr REG1, [x0, OFFS]
>  	GPR_LAYOUT
> 

AFAICT gcc doesn't define __LP64__, so I don't think this will work;
it's also backwards - you shouldn't assume that because __LP64__ isn't
defined that we have a 32-bit pointer.  GCC does, however, define
__ILP32__ when building for that ABI.

The right way to do this is to create a new header file,
machine/aarch64/machine/asm.h, which contains a gas macro called, lets
say `ptr_param', that takes the *number* of register argument and is
then conditionally defined to either nothing or an assembly statement
that narrows the register with that number.  In the main function code
you can then write simply

	ptr_param 0

and avoid all conditional assembly in the main source files.

R.

Re: AArch64 ILP32 strcmp bug

[Keith Packard]

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

Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes:

> AFAICT gcc doesn't define __LP64__, so I don't think this will work;
> it's also backwards - you shouldn't assume that because __LP64__ isn't
> defined that we have a 32-bit pointer.  GCC does, however, define
> __ILP32__ when building for that ABI.

I borrowed that symbol from glibc; figuring it was better to use a
mechanism already in use elsewhere.

> The right way to do this is to create a new header file,
> machine/aarch64/machine/asm.h, which contains a gas macro called, lets
> say `ptr_param', that takes the *number* of register argument and is
> then conditionally defined to either nothing or an assembly statement
> that narrows the register with that number.  In the main function code
> you can then write simply
>
> 	ptr_param 0
>
> and avoid all conditional assembly in the main source files.

Yes, it would be good to see all of the asm code fixed up to work on
ilp32. For now, just getting setjmp/longjmp working and bypassing all of
the other aarch64 asm code on ilp32 will suffice to make the library
work. I've got an aarch64 toolchain with ilp32 support built. Figuring
out how to catch errors of this particular sort seems challenging
though; the bug report makes it clear that even the incorrect functions
work most of the time.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]