Celeste Liu <coelacanthushex@xxxxxxxxx> 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
