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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK8P3a3UFFt8uf_64J-aRbmB-zdH8TpEaRHSdf9JzPk9SYzYgA@mail.gmail.com>
Date:   Mon, 16 May 2022 23:00:33 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Maxim Zhukov <crazycdeveloper@...il.com>
Cc:     chi.minghao@....com.cn, varad.gautam@...e.com,
        Arnd Bergmann <arnd@...db.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Shakeel Butt <shakeelb@...gle.com>, vasily.averin@...ux.dev,
        Manfred Spraul <manfred@...orfullife.com>,
        Davidlohr Bueso <dbueso@...e.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Maxim Zhukov <mussitantesmortem@...il.com>
Subject: Re: [RFC PATCH 1/1] ipc, sem: fix backward compatibility with x86-32 kernels

On Sun, May 15, 2022 at 9:01 PM Maxim Zhukov <crazycdeveloper@...il.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@...il.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