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

Message-ID: <mhng-bd15a99e-2c1e-4f0e-95ff-b6f14fe26681@palmer-ri-x1c9>
Date:   Wed, 08 Nov 2023 19:26:34 -0800 (PST)
From:   Palmer Dabbelt <palmer@...osinc.com>
To:     cleger@...osinc.com
CC:     shuah@...nel.org, krisman@...labora.com,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-riscv@...ts.infradead.org, cleger@...osinc.com
Subject:     Re: [PATCH] selftests: sud_test: return correct emulated syscall value on RISC-V

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:

    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.
    +	 */
    +#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?