Message ID | 87mssir1u6.fsf@oldenburg.str.redhat.com |
---|---|
State | New |
Headers | show |
Series | Linux: Switch back to assembly syscall wrapper for prctl (bug 29770) | expand |
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 >
* 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
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: > 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
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.
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