Re: [PATCH v5 04/10] ARM: syscall: always store thread_info->abi_syscall

From: Kees Cook <keescook@chromium.org>
To: Arnd Bergmann <arnd@kernel.org>,
	Lecopzer Chen <lecopzer.chen@mediatek.com>
Cc: Russell King <linux@armlinux.org.uk>,
	Arnd Bergmann <arnd@arndb.de>,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-arch@vger.kernel.org,
	linux-mm@kvack.org, Alexander Viro <viro@zeniv.linux.org.uk>,
	Linus Walleij <linus.walleij@linaro.org>,
	yj.chiang@mediatek.com, linux-hardening@vger.kernel.org
Subject: Re: [PATCH v5 04/10] ARM: syscall: always store thread_info->abi_syscall
Date: Thu, 3 Aug 2023 16:17:24 -0700	[thread overview]
Message-ID: <202308031551.034F346@keescook> (raw)
In-Reply-To: <20210726141141.2839385-5-arnd@kernel.org>

On Mon, Jul 26, 2021 at 04:11:35PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The system call number is used in a a couple of places, in particular
> ptrace, seccomp and /proc/<pid>/syscall.

*thread necromancy*

Hi!

So, it seems like the seccomp selftests broke in a few places due to
this change (back in v5.15). I really thought kernelci.org was running
the seccomp tests, but it seems like the coverage is spotty.

Specifically, the syscall_restart selftest fails, as well as syscall_errno
and syscall_faked (both via seccomp and PTRACE), starting with this patch.

> The last one apparently never worked reliably on ARM for tasks that are
> not currently getting traced.
> 
> Storing the syscall number in the normal entry path makes it work,
> as well as allowing us to see if the current system call is for OABI
> compat mode, which is the next thing I want to hook into.
> 
> Since the thread_info->syscall field is not just the number any more, it
> is now renamed to abi_syscall. In kernels that enable both OABI and EABI,
> the upper bits of this field encode 0x900000 (__NR_OABI_SYSCALL_BASE)
> for OABI tasks, while normal EABI tasks do not set the upper bits. This
> makes it possible to implement the in_oabi_syscall() helper later.
> 
> All other users of thread_info->syscall go through the syscall_get_nr()
> helper, which in turn filters out the ABI bits.

While I've reproducing the bisect done by mediatek, I'm still poking
around in here to figure out what's gone wrong. There was a recent patch
to fix this, but it looks like it's not complete:
https://lore.kernel.org/all/20230724121655.7894-1-lecopzer.chen@mediatek.com/

With the above applied, syscall_errno and syscall_faked start working
again, but not the syscall_restart test.

> Note that the ABI information is lost with PTRACE_SET_SYSCALL, so one
> cannot set the internal number to a particular version, but this was
> already the case. We could change it to let gdb encode the ABI type along
> with the syscall in a CONFIG_OABI_COMPAT-enabled kernel, but that itself
> would be a (backwards-compatible) ABI change, so I don't do it here.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Another issue of note, which may just be "by design" for arm32, is that
an invalid syscall (or, at least, a negative syscall) results in SIGILL,
rather than a errno=ENOSYS failure. This seems to have been true at least
as far back as v5.8 (where this was cleaned up for at least arm64 and
s390). There was a seccomp test added for it in v5.9, but it has been
failing for arm32 since then. :(

I mention this because the behavior of the syscall_restart test looks
like an invalid syscall: on restart a SIGILL is caught instead of the
syscall correctly continuing.

Anyway, I'll keep debugging this, but figured I'd mention it in case
anyone else had been seeing issues in here.

-Kees

-- 
Kees Cook