[PATCH] Linux: Switch back to assembly syscall wrapper for prctl (bug 29770)

[PATCH] Linux: Switch back to assembly syscall wrapper for prctl (bug 29770)

[Florian Weimer]

Commit ff026950e280bc3e9487b41b460fb31bc5b57721 ("Add a C wrapper for
prctl [BZ #25896]") replaced the assembler wrapper with a C function.
However, the C variadic function implementation has a different ABI
on powerpc64le-linux-gnu.  Switch back to the assembler implementation
on most targets and only keep the C implementation for x86-64 x32.

Also add the __prctl_time64 alias from commit
b39ffab860cd743a82c91946619f1b8158b0b65e ("Linux: Add time64 alias for
prctl") to sysdeps/unix/sysv/linux/syscalls.list; it was not yet
present in commit ff026950e280bc3e9487b41b460fb31bc5b57721.

This restores the old ABI on powerpc64le-linux-gnu, thus fixing
bug 29770.

Notes:

This is just a repost of my previous patch.  I still think it is the
right thing to do.  We now have a second case where the varargs
implementation causes stack corruption on powerpc64le-linux-gnu.  This
time it's the libasan interceptor for prctl:

  libasan uses incorrect prctl prototype
  <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113728>

Thanks,
Florian

---
 sysdeps/unix/sysv/linux/syscalls.list            | 1 +
 sysdeps/unix/sysv/linux/{ => x86_64/x32}/prctl.c | 5 +----
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/syscalls.list b/sysdeps/unix/sysv/linux/syscalls.list
index 73e941ef89..9ac42c3436 100644
--- a/sysdeps/unix/sysv/linux/syscalls.list
+++ b/sysdeps/unix/sysv/linux/syscalls.list
@@ -46,6 +46,7 @@ open_tree	EXTRA	open_tree	i:isU	open_tree
 pipe2		-	pipe2		i:fi	__pipe2		pipe2
 pidfd_open	EXTRA	pidfd_open	i:iU	pidfd_open
 pidfd_getfd	EXTRA	pidfd_getfd	i:iiU	pidfd_getfd
+prctl		EXTRA	prctl		i:iiiii	__prctl		prctl __prctl_time64
 pivot_root	EXTRA	pivot_root	i:ss	pivot_root
 pidfd_send_signal	EXTRA	pidfd_send_signal	i:iiPU	pidfd_send_signal
 process_madvise EXTRA   process_madvise i:iPniU process_madvise
diff --git a/sysdeps/unix/sysv/linux/prctl.c b/sysdeps/unix/sysv/linux/x86_64/x32/prctl.c
similarity index 93%
rename from sysdeps/unix/sysv/linux/prctl.c
rename to sysdeps/unix/sysv/linux/x86_64/x32/prctl.c
index 52d234ea0d..4bf1b479a0 100644
--- a/sysdeps/unix/sysv/linux/prctl.c
+++ b/sysdeps/unix/sysv/linux/x86_64/x32/prctl.c
@@ -1,4 +1,4 @@
-/* prctl - Linux specific syscall.
+/* prctl - Linux specific syscall.  x86-64 x32 version.
    Copyright (C) 2020-2024 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
@@ -40,6 +40,3 @@ __prctl (int option, ...)
 
 libc_hidden_def (__prctl)
 weak_alias (__prctl, prctl)
-#if __TIMESIZE != 64
-weak_alias (__prctl, __prctl_time64)
-#endif

base-commit: 7c8df0b9441e34928f2d7d70531e3d55e016c32e

RE: [PATCH] Linux: Switch back to assembly syscall wrapper for prctl (bug 29770)

[Simon Chopin]

Hi Florian,

On ven. 02 févr. 2024 22:30:25, Florian Weimer wrote:
> Commit ff026950e280bc3e9487b41b460fb31bc5b57721 ("Add a C wrapper for
> prctl [BZ #25896]") replaced the assembler wrapper with a C function.
> However, the C variadic function implementation has a different ABI
> on powerpc64le-linux-gnu.  Switch back to the assembler implementation
> on most targets and only keep the C implementation for x86-64 x32.
>
> Also add the __prctl_time64 alias from commit
> b39ffab860cd743a82c91946619f1b8158b0b65e ("Linux: Add time64 alias for
> prctl") to sysdeps/unix/sysv/linux/syscalls.list; it was not yet
> present in commit ff026950e280bc3e9487b41b460fb31bc5b57721.
>
> This restores the old ABI on powerpc64le-linux-gnu, thus fixing
> bug 29770.

Codewise it all looks good to me, but I have a perhaps dumb question:
at this point, aren't we breaking ABI again? Presumably, binaries have
been compiled against the varargs ABI, which AFAICT has been shipped in
RHEL 9 and Ubuntu 22.04 among others, which have been out for a while
now.

>
> Notes:
>
> This is just a repost of my previous patch.  I still think it is the
> right thing to do.  We now have a second case where the varargs
> implementation causes stack corruption on powerpc64le-linux-gnu.  This
> time it's the libasan interceptor for prctl:
>
>   libasan uses incorrect prctl prototype
>   <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113728>
>
> Thanks,
> Florian
>
> ---
>  sysdeps/unix/sysv/linux/syscalls.list            | 1 +
>  sysdeps/unix/sysv/linux/{ => x86_64/x32}/prctl.c | 5 +----
>  2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/sysdeps/unix/sysv/linux/syscalls.list b/sysdeps/unix/sysv/linux/syscalls.list
> index 73e941ef89..9ac42c3436 100644
> --- a/sysdeps/unix/sysv/linux/syscalls.list
> +++ b/sysdeps/unix/sysv/linux/syscalls.list
> @@ -46,6 +46,7 @@ open_tree	EXTRA	open_tree	i:isU	open_tree
>  pipe2		-	pipe2		i:fi	__pipe2		pipe2
>  pidfd_open	EXTRA	pidfd_open	i:iU	pidfd_open
>  pidfd_getfd	EXTRA	pidfd_getfd	i:iiU	pidfd_getfd
> +prctl		EXTRA	prctl		i:iiiii	__prctl		prctl __prctl_time64
>  pivot_root	EXTRA	pivot_root	i:ss	pivot_root
>  pidfd_send_signal	EXTRA	pidfd_send_signal	i:iiPU	pidfd_send_signal
>  process_madvise EXTRA   process_madvise i:iPniU process_madvise
> diff --git a/sysdeps/unix/sysv/linux/prctl.c b/sysdeps/unix/sysv/linux/x86_64/x32/prctl.c
> similarity index 93%
> rename from sysdeps/unix/sysv/linux/prctl.c
> rename to sysdeps/unix/sysv/linux/x86_64/x32/prctl.c
> index 52d234ea0d..4bf1b479a0 100644
> --- a/sysdeps/unix/sysv/linux/prctl.c
> +++ b/sysdeps/unix/sysv/linux/x86_64/x32/prctl.c
> @@ -1,4 +1,4 @@
> -/* prctl - Linux specific syscall.
> +/* prctl - Linux specific syscall.  x86-64 x32 version.
>     Copyright (C) 2020-2024 Free Software Foundation, Inc.
>     This file is part of the GNU C Library.
>
> @@ -40,6 +40,3 @@ __prctl (int option, ...)
>
>  libc_hidden_def (__prctl)
>  weak_alias (__prctl, prctl)
> -#if __TIMESIZE != 64
> -weak_alias (__prctl, __prctl_time64)
> -#endif
>
> base-commit: 7c8df0b9441e34928f2d7d70531e3d55e016c32e
>

Re: [PATCH] Linux: Switch back to assembly syscall wrapper for prctl (bug 29770)

[Florian Weimer]

* Simon Chopin:

> Hi Florian,
>
> On ven. 02 févr. 2024 22:30:25, Florian Weimer wrote:
>> Commit ff026950e280bc3e9487b41b460fb31bc5b57721 ("Add a C wrapper for
>> prctl [BZ #25896]") replaced the assembler wrapper with a C function.
>> However, the C variadic function implementation has a different ABI
>> on powerpc64le-linux-gnu.  Switch back to the assembler implementation
>> on most targets and only keep the C implementation for x86-64 x32.
>>
>> Also add the __prctl_time64 alias from commit
>> b39ffab860cd743a82c91946619f1b8158b0b65e ("Linux: Add time64 alias for
>> prctl") to sysdeps/unix/sysv/linux/syscalls.list; it was not yet
>> present in commit ff026950e280bc3e9487b41b460fb31bc5b57721.
>>
>> This restores the old ABI on powerpc64le-linux-gnu, thus fixing
>> bug 29770.
>
> Codewise it all looks good to me, but I have a perhaps dumb question:
> at this point, aren't we breaking ABI again? Presumably, binaries have
> been compiled against the varargs ABI, which AFAICT has been shipped in
> RHEL 9 and Ubuntu 22.04 among others, which have been out for a while
> now.

It's possible to call functions with the parameter save area present
that do not actually need it.  The issue is in the other direction only.

I believe the ABI was designed this way that it's possible to compile
lots of legacy software that uses implicit function declarations, where
the compiler has no information whether the function is defined variadic
or not.  Therefore, it creates the parameter save area for all such
calls.  I could probably word the commit message better, maybe like
this:

“
Commit ff026950e280bc3e9487b41b460fb31bc5b57721 ("Add a C wrapper for
prctl [BZ #25896]") replaced the assembler wrapper with a C function.
However, on powerpc64le-linux-gnu, the C variadic function
implementation requires extra work in the caller to set up the parameter
save area.  Calling a function that needs a parameter save area without
one (because the prototype used indicates the function is not variadic)
corrupts the caller's stack.  Switch back to the assembler
implementation on most targets and only keep the C implementation for
x86-64 x32.
”

Thanks,
Florian

Re: [PATCH] Linux: Switch back to assembly syscall wrapper for prctl (bug 29770)

[Andreas Schwab]

On Feb 13 2024, Florian Weimer wrote:

> “
> Commit ff026950e280bc3e9487b41b460fb31bc5b57721 ("Add a C wrapper for
> prctl [BZ #25896]") replaced the assembler wrapper with a C function.
> However, on powerpc64le-linux-gnu, the C variadic function
> implementation requires extra work in the caller to set up the parameter
> save area.  Calling a function that needs a parameter save area without
> one (because the prototype used indicates the function is not variadic)
> corrupts the caller's stack.  Switch back to the assembler
> implementation on most targets and only keep the C implementation for
> x86-64 x32.
> ”

That does not explain why the compiler did not set up the parameter save
area even though the declaration in <sys/prctl.h> is varadic.  Do I
understand correctly that some software uses a private declaration that
is prototyped but non-variadic?

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Linux: Switch back to assembly syscall wrapper for prctl (bug 29770)

[Florian Weimer]

* Andreas Schwab:

> On Feb 13 2024, Florian Weimer wrote:
>
>> “
>> Commit ff026950e280bc3e9487b41b460fb31bc5b57721 ("Add a C wrapper for
>> prctl [BZ #25896]") replaced the assembler wrapper with a C function.
>> However, on powerpc64le-linux-gnu, the C variadic function
>> implementation requires extra work in the caller to set up the parameter
>> save area.  Calling a function that needs a parameter save area without
>> one (because the prototype used indicates the function is not variadic)
>> corrupts the caller's stack.  Switch back to the assembler
>> implementation on most targets and only keep the C implementation for
>> x86-64 x32.
>> ”
>
> That does not explain why the compiler did not set up the parameter save
> area even though the declaration in <sys/prctl.h> is varadic.  Do I
> understand correctly that some software uses a private declaration that
> is prototyped but non-variadic?

Yes, GCC and LLVM upstream contain an unprototyped prctl function
declaration somewhere:

  libasan uses incorrect prctl prototype 
  <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113728>

This is not the only piece of software with this problem.  It's easier
to fix this in glibc than to figure out how to change the libasan
sanitizer to change the prototype there, so that's why I prefer this
approach.

Thanks,
Florian

Re: [PATCH] Linux: Switch back to assembly syscall wrapper for prctl (bug 29770)

[Andreas Schwab]

On Feb 13 2024, Florian Weimer wrote:

> Yes, GCC and LLVM upstream contain an unprototyped prctl function

s/un//

> declaration somewhere:
>
>   libasan uses incorrect prctl prototype 
>   <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113728>

Please add that information to the commit message.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."