linux-kernel - Re: [PATCH] selftests: sud_test: return correct emulated syscall value on RISC-V

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: <mhng-9af5865a-9208-4308-9eb6-4ec07a4b4cb2@palmer-ri-x1c9a>
Date:   Thu, 09 Nov 2023 08:14:36 -0800 (PST)
From:   Palmer Dabbelt <palmer@...osinc.com>
To:     cleger@...osinc.com, tglx@...utronix.de
CC:     shuah@...nel.org, krisman@...labora.com,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-riscv@...ts.infradead.org
Subject:     Re: [PATCH] selftests: sud_test: return correct emulated syscall value on RISC-V

On Thu, 09 Nov 2023 00:22:46 PST (-0800), cleger@...osinc.com wrote:
>
>
> On 09/11/2023 04:26, Palmer Dabbelt wrote:
>> On Wed, 13 Sep 2023 07:07:11 PDT (-0700), cleger@...osinc.com wrote:
>>> Currently, the sud_test expects the emulated syscall to return the
>>> emulated syscall number. This assumption only works on architectures
>>> were the syscall calling convention use the same register for syscall
>>> number/syscall return value. This is not the case for RISC-V and thus
>>> the return value must be also emulated using the provided ucontext.
>>>
>>> Signed-off-by: Clément Léger <cleger@...osinc.com>
>>> ---
>>>  tools/testing/selftests/syscall_user_dispatch/sud_test.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/syscall_user_dispatch/sud_test.c
>>> b/tools/testing/selftests/syscall_user_dispatch/sud_test.c
>>> index b5d592d4099e..1b5553c19700 100644
>>> --- a/tools/testing/selftests/syscall_user_dispatch/sud_test.c
>>> +++ b/tools/testing/selftests/syscall_user_dispatch/sud_test.c
>>> @@ -158,6 +158,14 @@ static void handle_sigsys(int sig, siginfo_t
>>> *info, void *ucontext)
>>>
>>>      /* In preparation for sigreturn. */
>>>      SYSCALL_DISPATCH_OFF(glob_sel);
>>> +
>>> +    /*
>>> +     * Modify interrupted context returned value according to syscall
>>> +     * calling convention
>>> +     */
>>> +#if defined(__riscv)
>>> +    ((ucontext_t*)ucontext)->uc_mcontext.__gregs[REG_A0] =
>>> MAGIC_SYSCALL_1;
>>> +#endif
>>>  }
>>>
>>>  TEST(dispatch_and_return)
>>
>> I'm not sure if I'm just tired, but it took me a while to figure out why
>> this was necessary.  I think this is a better explanation:
>
> I think it's because this mechanism does not behave like other syscalls
> at all and the classic calling convention does not really apply...

Yep.  I also got tripped up because I mis-read the docs and though 
SIGSYS was only for some error case (where it's actually for all the 
intercepted syscalls).

>>    diff --git a/tools/testing/selftests/syscall_user_dispatch/sud_test.c
>> b/tools/testing/selftests/syscall_user_dispatch/sud_test.c
>>    index b5d592d4099e..a913fd90cfa3 100644
>>    --- a/tools/testing/selftests/syscall_user_dispatch/sud_test.c
>>    +++ b/tools/testing/selftests/syscall_user_dispatch/sud_test.c
>>    @@ -158,6 +158,16 @@ static void handle_sigsys(int sig, siginfo_t
>> *info, void *ucontext)
>>            /* In preparation for sigreturn. */
>>         SYSCALL_DISPATCH_OFF(glob_sel);
>>    +    /*
>>    +     * The tests for argument handling assume that `syscall(x) ==
>> x`.  This
>>    +     * is a NOP on x86 because the syscall number is passed in %rax,
>> which
>>    +     * happens to also be the function ABI return register.  Other
>>    +     * architectures may need to swizzle the arguments around.
>>    +     */
>
> Indeed, that is more clear. Should I send a v2 ?

I would, but +Thomas as it looks like he's the one taking patches for 
this.

>
>>    +#if defined(__riscv)
>>    +    (ucontext_t*)ucontext)->uc_mcontext.__gregs[REG_A0] =
>>    +        (ucontext_t*)ucontext)->uc_mcontext.__gregs[REG_A7];
>>    +#endif
>>     }
>>        TEST(dispatch_and_return)
>>
>> but also
>>
>> Reviewed-by: Palmer Dabbelt <palmer@...osinc.com>
>> Acked-by: Palmer Dabbelt <palmer@...osinc.com>
>>
>> as I agree this is correct.
>>
>> also: wouldn't arm64 also need to move x8 into x0 here, for essentially
>> the same reason as we do?
>
> Yes, as well as for a bunch of other architectures. I suspect this has
> only been tested on x86. AFAIK, this feature is mainly for wine usage
> which then makes sense for x86 and games.

Ya, makes sense -- I'd just looked at Arm to double-check my 
understanding here, as we usually don't find bugs in generic code before 
Arm does.

>
> Thanks,
>
> Clément