Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-624-g7714e4406d-fm-20230801.001-g7714e440
Mime-Version: 1.0
Message-Id: <b0871ecb-a26b-4223-a4f5-28e1c8d468e4@app.fastmail.com>
In-Reply-To: <202308031551.034F346@keescook>
References: <20210726141141.2839385-1-arnd@kernel.org>
 <20210726141141.2839385-5-arnd@kernel.org> <202308031551.034F346@keescook>
Date:   Wed, 09 Aug 2023 21:42:43 +0200
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Kees Cook" <keescook@chromium.org>,
        "Arnd Bergmann" <arnd@kernel.org>,
        "Lecopzer Chen" <lecopzer.chen@mediatek.com>
Cc:     "Russell King" <linux@armlinux.org.uk>,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        Linux-Arch <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
Content-Type: text/plain
Precedence: bulk

On Fri, Aug 4, 2023, at 01:17, Kees Cook wrote:
> 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.

Thanks for tracking this down!

>> 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.

Right, I also see you addressed this better in your follow-up patch,
I'll comment there.

>> 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.


The odd arm behavior came up on IRC recently, and I saw that this
was what arm has always done, but I could not figure out why this
is done. I tried to see where s390 and arm64 changed the behavior
but can't find it. Do you have the commit IDs?

      Arnd