Re: [PATCH] riscv/entry: get correct syscall number from syscall_get_nr()

["Björn Töpel" <bjorn@kernel.org>]

Celeste Liu <coelacanthushex@gmail.com> writes:

>>> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>>> index 51ebfd23e0076447518081d137102a9a11ff2e45..3125fab8ee4af468ace9f692dd34e1797555cce3 100644
>>> --- a/arch/riscv/kernel/traps.c
>>> +++ b/arch/riscv/kernel/traps.c
>>> @@ -316,18 +316,25 @@ void do_trap_ecall_u(struct pt_regs *regs)
>>>  {
>>>  	if (user_mode(regs)) {
>>>  		long syscall = regs->a7;
>>> +		long res;
>>>  
>>>  		regs->epc += 4;
>>>  		regs->orig_a0 = regs->a0;
>>> -		regs->a0 = -ENOSYS;
>>>  
>>>  		riscv_v_vstate_discard(regs);
>>>  
>>> -		syscall = syscall_enter_from_user_mode(regs, syscall);
>>> +		res = syscall_enter_from_user_mode(regs, syscall);
>>> +		/*
>>> +		 * Call syscall_get_nr() again because syscall_enter_from_user_mode()
>>> +		 * may change a7 register.
>>> +		 */
>>> +		syscall = syscall_get_nr(current, regs);
>>>  
>>>  		add_random_kstack_offset();
>>>  
>>> -		if (syscall >= 0 && syscall < NR_syscalls)
>>> +		if (syscall < 0 || syscall >= NR_syscalls)
>>> +			regs->a0 = -ENOSYS;
>>> +		else if (res != -1)
>>>  			syscall_handler(regs, syscall);
>> 
>> Here we can perform the syscall, even if res is -1. E.g., if this path
>> [2] is taken, we might have a valid syscall number in a7, but the
>> syscall should not be performed.
>
> I may misunderstand what you said, but I can't see the issue you pointed.
> A syscall is performed iff
>
> 1) syscall number in a7 must be valid, so it can reach "else if" branch.
> 2) res != -1, so syscall_enter_from_user_mode() doesn't return -1 to
>    inform the syscall should be skipped.

Ah, indeed. Apologies, that'll work!

Related, now wont this reintroduce the seccomp filtering problem? Say,
res is -1 *and* syscall invalid, then a0 updated by seccomp will be
overwritten here?

>> Also, one reason for the generic entry is so that it should be less
>> work. Here, you pull (IMO) details that belong to the common entry
>> implementation/API up the common entry user. Wdyt about pushing it down
>> to common entry? Something like:
>
> Yeah, we can. But I pull it out of common entry to get more simple API semantic:
>
> 1. syscall_enter_from_user_mode() will do two things:
>    1) the return value is only to inform whether the syscall should be skipped.
>    2) regs will be modified by filters (seccomp or ptrace and so on).
> 2. for common entry user, there is two informations: syscall number and
>    the return value of syscall_enter_from_user_mode() (called is_skipped below).
>    so there is three situations:
>    1) if syscall number is invalid, the syscall should not be performed, and
>       we set a0 to -ENOSYS to inform userspace the syscall doesn't exist.
>    2) if syscall number is valid, is_skipped will be used:
>       a) if is_skipped is -1, which means there are some filters reject this syscall,
>          so the syscall should not performed. (Of course, we can use bool instead to
>          get better semantic)
>       b) if is_skipped != -1, which means the filters approved this syscall,
>          so we invoke syscall handler with modified regs.
>
> In your design, the logical condition is not obvious. Why syscall_enter_from_user_mode()
> informed the syscall will be skipped but the syscall handler will be called
> when syscall number is invalid? The users need to think two things to get result:
> a) -1 means skip
> b) -1 < 0 in signed integer, so the skip condition is always a invalid syscall number.
>
> In may way, the users only need to think one thing: The syscall_enter_from_user_mode()
> said -1 means the syscall should not be performed, so use it as a condition of reject
> directly. They just need to combine the informations that they get from API as the
> condition of control flow.

I'm all-in for simpler API usage! Maybe massage the
syscall_enter_from_user_mode() (or a new one), so that additional
syscall_get_nr() call is not needed?


Björn