[RFC PATCH 0/1] Fix handling semctl on x86-32 kernels

[RFC PATCH 0/1] Fix handling semctl on x86-32 kernels

[Maxim Zhukov]

I'm not sure about this patch. Maybe there is a better way to solve this problem:
all libc sends with cmd IPC_64 flag[1][2][3], but x86-32 kernel does not have compat
syscall layer to handle correctly semctl command.

[1]: https://elixir.bootlin.com/uclibc-ng/v1.0.40/source/libc/misc/sysvipc/sem.c#L58
[2]: https://elixir.bootlin.com/musl/v1.2.3/source/src/ipc/semctl.c#L48 -> https://elixir.bootlin.com/musl/v1.2.3/source/src/ipc/ipc.h#L22
[3]: https://elixir.bootlin.com/glibc/glibc-2.35/source/sysdeps/unix/sysv/linux/semctl.c#L124

Maxim Zhukov (1):
  ipc, sem: fix backward compatibility with x86-32 kernels

 ipc/sem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.36.1

[RFC PATCH 1/1] ipc, sem: fix backward compatibility with x86-32 kernels

[Maxim Zhukov]

Since with commit 275f22148e87 ("ipc: rename old-style shmctl/semctl/msgctl syscalls")
we have changed behavior:

ksys_semctl lost parse ipc version (ipc_parse_version), because the
new syscall works with IPC_64 only, but the parse function has some
side effect - it removes IPC_64 bit from `cmd`.

Some libc forced sends IPC_64 bit in semctl syscall[1][2][3], this leads to
a bug - X86-32 kernel does not have compat headers and does not
correctly parse received command (cmd) from semctl syscall: cmd have actual
command and IPC_64 bit, thus throw EINVAL error in ksys_semctl

This commit forcibly removes IPC_64 bit from the cmd for restore
backward compatibility.

[1]: https://elixir.bootlin.com/uclibc-ng/v1.0.40/source/libc/misc/sysvipc/sem.c#L58
[2]: https://elixir.bootlin.com/musl/v1.2.3/source/src/ipc/semctl.c#L48 -> https://elixir.bootlin.com/musl/v1.2.3/source/src/ipc/ipc.h#L22
[3]: https://elixir.bootlin.com/glibc/glibc-2.35/source/sysdeps/unix/sysv/linux/semctl.c#L124

Signed-off-by: Maxim Zhukov <mussitantesmortem@gmail.com>
---
 ipc/sem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index 0dbdb98fdf2d..824244170000 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -1706,7 +1706,7 @@ static long ksys_semctl(int semid, int semnum, int cmd, unsigned long arg, int v
 
 SYSCALL_DEFINE4(semctl, int, semid, int, semnum, int, cmd, unsigned long, arg)
 {
-	return ksys_semctl(semid, semnum, cmd, arg, IPC_64);
+	return ksys_semctl(semid, semnum, cmd & (~IPC_64), arg, IPC_64);
 }
 
 #ifdef CONFIG_ARCH_WANT_IPC_PARSE_VERSION
-- 
2.36.1

Re: [RFC PATCH 1/1] ipc, sem: fix backward compatibility with x86-32 kernels

[Andrew Morton]

On Sun, 15 May 2022 23:01:03 +0300 Maxim Zhukov <crazycdeveloper@gmail.com> wrote:

> Since with commit 275f22148e87 ("ipc: rename old-style shmctl/semctl/msgctl syscalls")
> we have changed behavior:
> 
> ksys_semctl lost parse ipc version (ipc_parse_version), because the
> new syscall works with IPC_64 only, but the parse function has some
> side effect - it removes IPC_64 bit from `cmd`.
> 
> Some libc forced sends IPC_64 bit in semctl syscall[1][2][3], this leads to
> a bug - X86-32 kernel does not have compat headers and does not
> correctly parse received command (cmd) from semctl syscall: cmd have actual
> command and IPC_64 bit, thus throw EINVAL error in ksys_semctl
> 
> This commit forcibly removes IPC_64 bit from the cmd for restore
> backward compatibility.
> 
> [1]: https://elixir.bootlin.com/uclibc-ng/v1.0.40/source/libc/misc/sysvipc/sem.c#L58
> [2]: https://elixir.bootlin.com/musl/v1.2.3/source/src/ipc/semctl.c#L48 -> https://elixir.bootlin.com/musl/v1.2.3/source/src/ipc/ipc.h#L22
> [3]: https://elixir.bootlin.com/glibc/glibc-2.35/source/sysdeps/unix/sysv/linux/semctl.c#L124
> 

Thanks.

275f22148e87 was three years ago.  Can you suggest why it took so long
for this to be discovered?

Re: [RFC PATCH 1/1] ipc, sem: fix backward compatibility with x86-32 kernels

[Arnd Bergmann]

On Sun, May 15, 2022 at 9:01 PM Maxim Zhukov <crazycdeveloper@gmail.com> wrote:
>
> Since with commit 275f22148e87 ("ipc: rename old-style shmctl/semctl/msgctl syscalls")
> we have changed behavior:
>
> ksys_semctl lost parse ipc version (ipc_parse_version), because the
> new syscall works with IPC_64 only, but the parse function has some
> side effect - it removes IPC_64 bit from `cmd`.
>
> Some libc forced sends IPC_64 bit in semctl syscall[1][2][3], this leads to
> a bug - X86-32 kernel does not have compat headers and does not
> correctly parse received command (cmd) from semctl syscall: cmd have actual
> command and IPC_64 bit, thus throw EINVAL error in ksys_semctl

That is unfortunate, and clearly against the intention of my commit
from back then:
the idea was that any libc that moves from the old to the new syscalls would
drop support for the ancient IPC version and no longer have to pass the IPC_64
flag.

Given how long it took to run into this bug, let's try to figure out
exactly what
options we have to address this before applying any patch.

> This commit forcibly removes IPC_64 bit from the cmd for restore
> backward compatibility.
>
> [1]: https://elixir.bootlin.com/uclibc-ng/v1.0.40/source/libc/misc/sysvipc/sem.c#L58
> [2]: https://elixir.bootlin.com/musl/v1.2.3/source/src/ipc/semctl.c#L48 -> https://elixir.bootlin.com/musl/v1.2.3/source/src/ipc/ipc.h#L22
> [3]: https://elixir.bootlin.com/glibc/glibc-2.35/source/sysdeps/unix/sysv/linux/semctl.c#L124

I think musl handles it correctly here: it always calls the old-style
ipc() syscall if that is
available.

For glibc, I'm not completely sure but I think that also does it
correctly, defining
IPC_64 to 0 for the configuration in which it calls sys_semctl().

The uclibc-ng implementation is clearly wrong here, I assume that's what you
tested with? While your patch would make uclibc-ng work on the affected
architectures and not break the other libc implementations, it is still an
ABI change to allow the 0x100 bit to be set in the "cmd" value. This is
different from both the traditional behavior on the ipc() syscall, and from
the traditional behavior on architectures that had semctl() without IPC_64.

> Signed-off-by: Maxim Zhukov <mussitantesmortem@gmail.com>
> ---
>  ipc/sem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ipc/sem.c b/ipc/sem.c
> index 0dbdb98fdf2d..824244170000 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -1706,7 +1706,7 @@ static long ksys_semctl(int semid, int semnum, int cmd, unsigned long arg, int v
>
>  SYSCALL_DEFINE4(semctl, int, semid, int, semnum, int, cmd, unsigned long, arg)
>  {
> -       return ksys_semctl(semid, semnum, cmd, arg, IPC_64);
> +       return ksys_semctl(semid, semnum, cmd & (~IPC_64), arg, IPC_64);
>  }

I don't think it makes sense to do this for semctl but not also for
shmctl and msqctl --
whatever we end up doing should be the same across all three.

       Arnd

Re: [RFC PATCH 1/1] ipc, sem: fix backward compatibility with x86-32 kernels

[Arnd Bergmann]

On Mon, May 16, 2022 at 10:06 PM Andrew Morton
<akpm@linux-foundation.org> wrote:
>
> On Sun, 15 May 2022 23:01:03 +0300 Maxim Zhukov <crazycdeveloper@gmail.com> wrote:
>
> > Since with commit 275f22148e87 ("ipc: rename old-style shmctl/semctl/msgctl syscalls")
> > we have changed behavior:
> >
> > ksys_semctl lost parse ipc version (ipc_parse_version), because the
> > new syscall works with IPC_64 only, but the parse function has some
> > side effect - it removes IPC_64 bit from `cmd`.
> >
> > Some libc forced sends IPC_64 bit in semctl syscall[1][2][3], this leads to
> > a bug - X86-32 kernel does not have compat headers and does not
> > correctly parse received command (cmd) from semctl syscall: cmd have actual
> > command and IPC_64 bit, thus throw EINVAL error in ksys_semctl
> >
> > This commit forcibly removes IPC_64 bit from the cmd for restore
> > backward compatibility.
> >
> > [1]: https://elixir.bootlin.com/uclibc-ng/v1.0.40/source/libc/misc/sysvipc/sem.c#L58
> > [2]: https://elixir.bootlin.com/musl/v1.2.3/source/src/ipc/semctl.c#L48 -> https://elixir.bootlin.com/musl/v1.2.3/source/src/ipc/ipc.h#L22
> > [3]: https://elixir.bootlin.com/glibc/glibc-2.35/source/sysdeps/unix/sysv/linux/semctl.c#L124
> >
>
> Thanks.
>
> 275f22148e87 was three years ago.  Can you suggest why it took so long
> for this to be discovered?

I think it only shows up with a uclibc-ng built against 32-bit kernel headers
from linux-5.1 or new for m68k, mips-o32, powerpc, s390, sh, sparc, and
x86-32 (list is from my original commit 275f22148e87), not for the more
popular musl or glibc libraries.

If sysvipc is used this rarely on uclibc-ng, maybe we can fix it by making
it behave the same way as glibc instead? I agree the kernel interface is
easy to get wrong here because of the subtle difference between ipc()
and semctl(), but this was an intentional design choice at the time, and
it did not affect the behavior of the existing syscalls, only the newly
added calls.

        Arnd