| 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
| ||
|
Message-ID: <70cb8e5b-7ee5-494a-a02a-ef286f8c970c@csgroup.eu>
Date: Tue, 21 Jan 2025 12:28:46 +0100
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Madhavan Srinivasan <maddy@...ux.ibm.com>, "Dmitry V. Levin"
<ldv@...ace.io>
Cc: Alexey Gladkov <legion@...nel.org>, Oleg Nesterov <oleg@...hat.com>,
Michael Ellerman <mpe@...erman.id.au>,
Eugene Syromyatnikov <evgsyr@...il.com>, Mike Frysinger <vapier@...too.org>,
Renzo Davoli <renzo@...unibo.it>, Davide Berardi <berardi.dav@...il.com>,
strace-devel@...ts.strace.io, Nicholas Piggin <npiggin@...il.com>,
Naveen N Rao <naveen@...nel.org>, linuxppc-dev@...ts.ozlabs.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/7] powerpc: properly negate error in
syscall_set_return_value()
Le 21/01/2025 à 12:13, Madhavan Srinivasan a écrit :
>
>
> On 1/20/25 10:42 PM, Dmitry V. Levin wrote:
>> On Mon, Jan 20, 2025 at 02:51:38PM +0100, Christophe Leroy wrote:
>>> Le 14/01/2025 à 18:04, Dmitry V. Levin a écrit :
>>>> On Mon, Jan 13, 2025 at 06:34:44PM +0100, Christophe Leroy wrote:
>>>>> Le 13/01/2025 à 18:10, Dmitry V. Levin a écrit :
>>>>>> Bring syscall_set_return_value() in sync with syscall_get_error(),
>>>>>> and let upcoming ptrace/set_syscall_info selftest pass on powerpc.
>>>>>>
>
> Sorry for getting to this thread late.
>
> Tried the series without this patch in
>
> 1) power9 PowerNV system and in power10 pSeries lpar
>
> # ./set_syscall_info
> TAP version 13
> 1..1
> # Starting 1 tests from 1 test cases.
> # RUN global.set_syscall_info ...
> # OK global.set_syscall_info
> ok 1 global.set_syscall_info
> # PASSED: 1 / 1 tests passed.
> # Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
>
> and in both case set_syscall_info passes.
> Will look at it further.
I guess it works because power9/10 are using scv not sc for system call,
hence using the new ABI ?
Christophe
>
> Maddy
>
>>>>>> This reverts commit 1b1a3702a65c ("powerpc: Don't negate error in
>>>>>> syscall_set_return_value()").
>>>>>
>>>>> There is a clear detailed explanation in that commit of why it needs to
>>>>> be done.
>>>>>
>>>>> If you think that commit is wrong you have to explain why with at least
>>>>> the same level of details.
>>>>
>>>> OK, please have a look whether this explanation is clear and detailed enough:
>>>>
>>>> =======
>>>> powerpc: properly negate error in syscall_set_return_value()
>>>>
>>>> When syscall_set_return_value() is used to set an error code, the caller
>>>> specifies it as a negative value in -ERRORCODE form.
>>>>
>>>> In !trap_is_scv case the error code is traditionally stored as follows:
>>>> gpr[3] contains a positive ERRORCODE, and ccr has 0x10000000 flag set.
>>>> Here are a few examples to illustrate this convention. The first one
>>>> is from syscall_get_error():
>>>> /*
>>>> * If the system call failed,
>>>> * regs->gpr[3] contains a positive ERRORCODE.
>>>> */
>>>> return (regs->ccr & 0x10000000UL) ? -regs->gpr[3] : 0;
>>>>
>>>> The second example is from regs_return_value():
>>>> if (is_syscall_success(regs))
>>>> return regs->gpr[3];
>>>> else
>>>> return -regs->gpr[3];
>>>>
>>>> The third example is from check_syscall_restart():
>>>> regs->result = -EINTR;
>>>> regs->gpr[3] = EINTR;
>>>> regs->ccr |= 0x10000000;
>>>>
>>>> Compared with these examples, the failure of syscall_set_return_value()
>>>> to assign a positive ERRORCODE into regs->gpr[3] is clearly visible:
>>>> /*
>>>> * In the general case it's not obvious that we must deal with
>>>> * CCR here, as the syscall exit path will also do that for us.
>>>> * However there are some places, eg. the signal code, which
>>>> * check ccr to decide if the value in r3 is actually an error.
>>>> */
>>>> if (error) {
>>>> regs->ccr |= 0x10000000L;
>>>> regs->gpr[3] = error;
>>>> } else {
>>>> regs->ccr &= ~0x10000000L;
>>>> regs->gpr[3] = val;
>>>> }
>>>>
>>>> This fix brings syscall_set_return_value() in sync with syscall_get_error()
>>>> and lets upcoming ptrace/set_syscall_info selftest pass on powerpc.
>>>>
>>>> Fixes: 1b1a3702a65c ("powerpc: Don't negate error in syscall_set_return_value()").
>>>> =======
>>>>
>>>>
>>>
>>> I think there is still something going wrong.
>>>
>>> do_seccomp() sets regs->gpr[3] = -ENOSYS; by default.
>>>
>>> Then it calls __secure_computing() which returns what __seccomp_filter()
>>> returns.
>>>
>>> In case of error, __seccomp_filter() calls syscall_set_return_value()
>>> with a negative value then returns -1
>>>
>>> do_seccomp() is called by do_syscall_trace_enter() which returns -1 when
>>> do_seccomp() doesn't return 0.
>>>
>>> do_syscall_trace_enter() is called by system_call_exception() and
>>> returns -1, so syscall_exception() returns regs->gpr[3]
>>>
>>> In entry_32.S, transfer_to_syscall, syscall_exit_prepare() is then
>>> called with the return of syscall_exception() as first parameter, which
>>> leads to:
>>>
>>> if (unlikely(r3 >= (unsigned long)-MAX_ERRNO) && is_not_scv) {
>>> if (likely(!(ti_flags & (_TIF_NOERROR | _TIF_RESTOREALL)))) {
>>> r3 = -r3;
>>> regs->ccr |= 0x10000000; /* Set SO bit in CR */
>>> }
>>> }
>>
>> Note the "unlikely" keyword here reminding us once more that in !scv case
>> regs->gpr[3] does not normally have -ERRORCODE form.
>>
>>> By chance, because you have already changed the sign of gpr[3], the
>>> above test fails and nothing is done to r3, and because you have also
>>> already set regs->ccr it works.
>>>
>>> But all this looks inconsistent with the fact that do_seccomp sets
>>> -ENOSYS as default value
>>>
>>> Also, when do_seccomp() returns 0, do_syscall_trace_enter() check the
>>> syscall number and when it is wrong it goes to skip: which sets
>>> regs->gpr[3] = -ENOSYS;
>>
>> It looks like do_seccomp() and do_syscall_trace_enter() get away by sheer
>> luck, implicitly relying on syscall_exit_prepare() transparently fixing
>> regs->gpr[3] for them.
>>
>>> So really I think it is not in line with your changes to set positive
>>> value in gpr[3].
>>>
>>> Maybe your change is still correct but it needs to be handled completely
>>> in that case.
>>
>> By the way, is there any reasons why do_seccomp() and
>> do_syscall_trace_enter() don't use syscall_set_return_value() yet?
>>
>>
>
Powered by blists - more mailing lists