linux-kernel - Re: [PATCH] riscv/entry: get correct syscall number from syscall_get

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: <8734kpqu8k.fsf@all.your.base.are.belong.to.us>
Date: Mon, 21 Oct 2024 09:46:35 -0700
From: Björn Töpel <bjorn@...nel.org>
To: Celeste Liu <coelacanthushex@...il.com>, Celeste Liu via B4 Relay
 <devnull+CoelacanthusHex.gmail.com@...nel.org>, Paul Walmsley
 <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, Albert Ou
 <aou@...s.berkeley.edu>, Björn Töpel
 <bjorn@...osinc.com>
Cc: Palmer Dabbelt <palmer@...osinc.com>, Alexandre Ghiti <alex@...ti.fr>,
 "Dmitry V. Levin" <ldv@...ace.io>, Andrea Bolognani <abologna@...hat.com>,
 Felix Yan <felixonmars@...hlinux.org>, Ruizhe Pan <c141028@...il.com>,
 Shiqi Zhang <shiqi@...c.iscas.ac.cn>, Guo Ren <guoren@...nel.org>, Yao Zi
 <ziyao@...root.org>, Han Gao <gaohan@...as.ac.cn>,
 linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
 stable@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] riscv/entry: get correct syscall number from
 syscall_get_nr()

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