linux-kernel - Re: [PATCH v4 0/7] ptrace: introduce PTRACE_SET_SYSCALL

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] [day] [month] [year] [list]

Message-ID: <yt9d8qqkyd43.fsf@linux.ibm.com>
Date: Wed, 05 Feb 2025 10:06:36 +0100
From: Sven Schnelle <svens@...ux.ibm.com>
To: "Dmitry V. Levin" <ldv@...ace.io>
Cc: Alexander Gordeev <agordeev@...ux.ibm.com>,
        Andrew Morton
 <akpm@...ux-foundation.org>,
        Oleg Nesterov <oleg@...hat.com>, Shuah Khan
 <shuah@...nel.org>,
        Alexey Gladkov <legion@...nel.org>,
        Eugene
 Syromyatnikov <evgsyr@...il.com>,
        Mike Frysinger <vapier@...too.org>, Renzo Davoli <renzo@...unibo.it>,
        Davide Berardi
 <berardi.dav@...il.com>,
        Heiko Carstens <hca@...ux.ibm.com>, Vasily
 Gorbik <gor@...ux.ibm.com>,
        Christian Borntraeger
 <borntraeger@...ux.ibm.com>,
        strace-devel@...ts.strace.io, linux-s390@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 0/7] ptrace: introduce PTRACE_SET_SYSCALL_INFO API

"Dmitry V. Levin" <ldv@...ace.io> writes:

> On Mon, Feb 03, 2025 at 12:35:42PM +0200, Dmitry V. Levin wrote:
>> On Mon, Feb 03, 2025 at 10:29:37AM +0100, Alexander Gordeev wrote:
>> > On Mon, Feb 03, 2025 at 08:58:49AM +0200, Dmitry V. Levin wrote:
>> > 
>> > Hi Dmitry,
>> > 
>> > > PTRACE_SET_SYSCALL_INFO is a generic ptrace API that complements
>> > > PTRACE_GET_SYSCALL_INFO by letting the ptracer modify details of
>> > > system calls the tracee is blocked in.
>> > ...
>> > 
>> > FWIW, I am getting these on s390:
>> > 
>> > # ./tools/testing/selftests/ptrace/set_syscall_info 
>> > TAP version 13
>> > 1..1
>> > # Starting 1 tests from 1 test cases.
>> > #  RUN           global.set_syscall_info ...
>> > # set_syscall_info.c:87:set_syscall_info:Expected exp_entry->nr (-1) == info->entry.nr (65535)
>> > # set_syscall_info.c:88:set_syscall_info:wait #3: PTRACE_GET_SYSCALL_INFO #2: syscall nr mismatch
>> > # set_syscall_info: Test terminated by assertion
>> > #          FAIL  global.set_syscall_info
>> > not ok 1 global.set_syscall_info
>> > # FAILED: 0 / 1 tests passed.
>> > # Totals: pass:0 fail:1 xfail:0 xpass:0 skip:0 error:0
>> > 
>> > I remember one of the earlier versions (v1 or v2) was working for me.
>> > 
>> > Thanks!
>> 
>> In v3, this test was extended to check whether PTRACE_GET_SYSCALL_INFO
>> called immediately after PTRACE_SET_SYSCALL_INFO returns the same syscall
>> number, and on s390 it apparently doesn't, thanks to its implementation
>> of syscall_get_nr() that returns 0xffff in this case.
>> 
>> To workaround this, we could either change syscall_get_nr() to return -1
>> in this case, or add an #ifdef __s390x__ exception to the test.
>> 
>> What would you prefer?
>
> OK, I'm going to apply the following s390 workaround to the test:
>
> diff --git a/tools/testing/selftests/ptrace/set_syscall_info.c b/tools/testing/selftests/ptrace/set_syscall_info.c
> index 0ec69401c008..4198248ef874 100644
> --- a/tools/testing/selftests/ptrace/set_syscall_info.c
> +++ b/tools/testing/selftests/ptrace/set_syscall_info.c
> @@ -71,6 +71,11 @@ check_psi_entry(struct __test_metadata *_metadata,
>  		const char *text)
>  {
>  	unsigned int i;
> +	int exp_nr = exp_entry->nr;
> +#if defined __s390__ || defined __s390x__
> +	/* s390 is the only architecture that has 16-bit syscall numbers */
> +	exp_nr &= 0xffff;
> +#endif
>  
>  	ASSERT_EQ(PTRACE_SYSCALL_INFO_ENTRY, info->op) {
>  		LOG_KILL_TRACEE("%s: entry stop mismatch", text);
> @@ -84,7 +89,7 @@ check_psi_entry(struct __test_metadata *_metadata,
>  	ASSERT_TRUE(info->stack_pointer) {
>  		LOG_KILL_TRACEE("%s: entry stop mismatch", text);
>  	}
> -	ASSERT_EQ(exp_entry->nr, info->entry.nr) {
> +	ASSERT_EQ(exp_nr, info->entry.nr) {
>  		LOG_KILL_TRACEE("%s: syscall nr mismatch", text);
>  	}
>  	for (i = 0; i < ARRAY_SIZE(exp_entry->args); ++i) {

Fine with me. As you already noted only 16 bit of the syscall number is
stored in pt_regs::int_code. A quick hack would be possible to do sign
extensions, so -1 would work. But i think this would be odd, because
positive numbers would still be limited.

So i think the patch you proposed is fine.