| Message ID | 20230718210037.250665-1-CoelacanthusHex@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] riscv: entry: set a0 = -ENOSYS only when syscall != -1 | expand |
Celeste Liu <coelacanthushex@gmail.com> writes: > When we test seccomp with 6.4 kernel, we found errno has wrong value. > If we deny NETLINK_AUDIT with EAFNOSUPPORT, after f0bddf50586d, we will > get ENOSYS instead. We got same result with commit 9c2598d43510 ("riscv: entry: > Save a0 prior syscall_enter_from_user_mode()"). > > After analysing code, we think that regs->a0 = -ENOSYS should only be executed > when syscall != -1 In __seccomp_filter, when seccomp rejected this syscall with > specified errno, they will set a0 to return number as syscall ABI, and then > return -1. This return number is finally pass as return number of > syscall_enter_from_user_mode, and then is compared with NR_syscalls after > converted to ulong (so it will be ULONG_MAX). The condition > syscall < NR_syscalls will always be false, so regs->a0 = -ENOSYS is always > executed. It covered a0 set by seccomp, so we always get ENOSYS when match > seccomp RET_ERRNO rule. > > Fixes: f0bddf50586d ("riscv: entry: Convert to generic entry") > Reported-by: Felix Yan <felixonmars@archlinux.org> > Co-developed-by: Ruizhe Pan <c141028@gmail.com> > Signed-off-by: Ruizhe Pan <c141028@gmail.com> > Co-developed-by: Shiqi Zhang <shiqi@isrc.iscas.ac.cn> > Signed-off-by: Shiqi Zhang <shiqi@isrc.iscas.ac.cn> > Signed-off-by: Celeste Liu <CoelacanthusHex@gmail.com> > Tested-by: Felix Yan <felixonmars@archlinux.org> Reviewed-by: Björn Töpel <bjorn@rivosinc.com>
On Wed, Jul 19, 2023 at 5:01 AM Celeste Liu <coelacanthushex@gmail.com> wrote: > > When we test seccomp with 6.4 kernel, we found errno has wrong value. > If we deny NETLINK_AUDIT with EAFNOSUPPORT, after f0bddf50586d, we will > get ENOSYS instead. We got same result with commit 9c2598d43510 ("riscv: entry: > Save a0 prior syscall_enter_from_user_mode()"). > > After analysing code, we think that regs->a0 = -ENOSYS should only be executed > when syscall != -1 In __seccomp_filter, when seccomp rejected this syscall with > specified errno, they will set a0 to return number as syscall ABI, and then > return -1. This return number is finally pass as return number of > syscall_enter_from_user_mode, and then is compared with NR_syscalls after > converted to ulong (so it will be ULONG_MAX). The condition > syscall < NR_syscalls will always be false, so regs->a0 = -ENOSYS is always > executed. It covered a0 set by seccomp, so we always get ENOSYS when match > seccomp RET_ERRNO rule. > > Fixes: f0bddf50586d ("riscv: entry: Convert to generic entry") > Reported-by: Felix Yan <felixonmars@archlinux.org> > Co-developed-by: Ruizhe Pan <c141028@gmail.com> > Signed-off-by: Ruizhe Pan <c141028@gmail.com> > Co-developed-by: Shiqi Zhang <shiqi@isrc.iscas.ac.cn> > Signed-off-by: Shiqi Zhang <shiqi@isrc.iscas.ac.cn> > Signed-off-by: Celeste Liu <CoelacanthusHex@gmail.com> > Tested-by: Felix Yan <felixonmars@archlinux.org> > --- > > v2 -> v3: use if-statement instead of set default value, > clarify the type of syscall > v1 -> v2: added explanation on why always got ENOSYS > > arch/riscv/kernel/traps.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c > index f910dfccbf5d2..5cef728745420 100644 > --- a/arch/riscv/kernel/traps.c > +++ b/arch/riscv/kernel/traps.c > @@ -297,6 +297,10 @@ asmlinkage __visible __trap_section void do_trap_break(struct pt_regs *regs) > asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs) > { > if (user_mode(regs)) { > + /* > + * Convert negative numbers to very high and thus out of range > + * numbers for comparisons. > + */ > ulong syscall = regs->a7; > > regs->epc += 4; > @@ -308,7 +312,7 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs) > > if (syscall < NR_syscalls) > syscall_handler(regs, syscall); > - else > + else if ((long)syscall != -1L) Maybe we should define an explicit macro for this ERRNO in __seccomp_filter, and this style obeys the coding convention. For this patch: Reviewed-by: Guo Ren <guoren@kernel.org> Cc: loongarch guy, please check loongarch's code. :) > regs->a0 = -ENOSYS; > > syscall_exit_to_user_mode(regs); > -- > 2.41.0 >
On Jul 19 2023, Celeste Liu wrote: > @@ -308,7 +312,7 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs) > > if (syscall < NR_syscalls) > syscall_handler(regs, syscall); > - else > + else if ((long)syscall != -1L) You can also use syscall != -1UL or even syscall != -1.
... > > + /* > > + * Convert negative numbers to very high and thus out of range > > + * numbers for comparisons. > > + */ > > ulong syscall = regs->a7; > > > > regs->epc += 4; > > @@ -308,7 +312,7 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs) > > > > if (syscall < NR_syscalls) If you leave 'syscall' signed and write: if (syscall >= 0 && syscall < NR_syscalls) the compiler will use a single unsigned compare. There is no need to 'optimise' it yourself. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Andreas Schwab <schwab@suse.de> writes: > On Jul 19 2023, Celeste Liu wrote: > >> @@ -308,7 +312,7 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs) >> >> if (syscall < NR_syscalls) >> syscall_handler(regs, syscall); >> - else >> + else if ((long)syscall != -1L) > > You can also use syscall != -1UL or even syscall != -1. The former is indeed better for the eyes! :-) The latter will get a -Wsign-compare warning, no? Björn
On July 20, 2023 12:28:47 AM GMT+08:00, "Björn Töpel" <bjorn@kernel.org> wrote: >Andreas Schwab <schwab@suse.de> writes: > >> On Jul 19 2023, Celeste Liu wrote: >> >>> @@ -308,7 +312,7 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs) >>> >>> if (syscall < NR_syscalls) >>> syscall_handler(regs, syscall); >>> - else >>> + else if ((long)syscall != -1L) >> >> You can also use syscall != -1UL or even syscall != -1. > >The former is indeed better for the eyes! :-) The latter will get a >-Wsign-compare warning, no? > > >Björn Well, that's true. And I just found out that by C standards, converting ulong to long is implementation-defined behavior, unlike long to ulong which is well-defined. So it is really better than (long)syscall != -1L.
Celeste Liu <coelacanthushex@gmail.com> writes: > On July 20, 2023 12:28:47 AM GMT+08:00, "Björn Töpel" <bjorn@kernel.org> wrote: >>Andreas Schwab <schwab@suse.de> writes: >> >>> On Jul 19 2023, Celeste Liu wrote: >>> >>>> @@ -308,7 +312,7 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs) >>>> >>>> if (syscall < NR_syscalls) >>>> syscall_handler(regs, syscall); >>>> - else >>>> + else if ((long)syscall != -1L) >>> >>> You can also use syscall != -1UL or even syscall != -1. >> >>The former is indeed better for the eyes! :-) The latter will get a >>-Wsign-compare warning, no? >> >> >>Björn > > Well, that's true. And I just found out that by C standards, converting > ulong to long is implementation-defined behavior, unlike long to ulong > which is well-defined. So it is really better than (long)syscall != -1L. If you're respinning, I suggest you use David's suggestion: * Remove the comment I suggest you to add * Use (signed) long * Add syscall >= 0 && * else if (syscall != -1) Which is the least amount of surprises IMO.
On July 20, 2023 5:08:37 PM GMT+08:00, "Björn Töpel" <bjorn@kernel.org> wrote: >Celeste Liu <coelacanthushex@gmail.com> writes: > >> On July 20, 2023 12:28:47 AM GMT+08:00, "Björn Töpel" <bjorn@kernel.org> wrote: >>>Andreas Schwab <schwab@suse.de> writes: >>> >>>> On Jul 19 2023, Celeste Liu wrote: >>>> >>>>> @@ -308,7 +312,7 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs) >>>>> >>>>> if (syscall < NR_syscalls) >>>>> syscall_handler(regs, syscall); >>>>> - else >>>>> + else if ((long)syscall != -1L) >>>> >>>> You can also use syscall != -1UL or even syscall != -1. >>> >>>The former is indeed better for the eyes! :-) The latter will get a >>>-Wsign-compare warning, no? >>> >>> >>>Björn >> >> Well, that's true. And I just found out that by C standards, converting >> ulong to long is implementation-defined behavior, unlike long to ulong >> which is well-defined. So it is really better than (long)syscall != -1L. > >If you're respinning, I suggest you use David's suggestion: > * Remove the comment I suggest you to add > * Use (signed) long > * Add syscall >= 0 && > * else if (syscall != -1) > >Which is the least amount of surprises IMO. v4 has sent
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c index f910dfccbf5d2..5cef728745420 100644 --- a/arch/riscv/kernel/traps.c +++ b/arch/riscv/kernel/traps.c @@ -297,6 +297,10 @@ asmlinkage __visible __trap_section void do_trap_break(struct pt_regs *regs) asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs) { if (user_mode(regs)) { + /* + * Convert negative numbers to very high and thus out of range + * numbers for comparisons. + */ ulong syscall = regs->a7; regs->epc += 4; @@ -308,7 +312,7 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs) if (syscall < NR_syscalls) syscall_handler(regs, syscall); - else + else if ((long)syscall != -1L) regs->a0 = -ENOSYS; syscall_exit_to_user_mode(regs);
When we test seccomp with 6.4 kernel, we found errno has wrong value. If we deny NETLINK_AUDIT with EAFNOSUPPORT, after f0bddf50586d, we will get ENOSYS instead. We got same result with commit 9c2598d43510 ("riscv: entry: Save a0 prior syscall_enter_from_user_mode()"). After analysing code, we think that regs->a0 = -ENOSYS should only be executed when syscall != -1 In __seccomp_filter, when seccomp rejected this syscall with specified errno, they will set a0 to return number as syscall ABI, and then return -1. This return number is finally pass as return number of syscall_enter_from_user_mode, and then is compared with NR_syscalls after converted to ulong (so it will be ULONG_MAX). The condition syscall < NR_syscalls will always be false, so regs->a0 = -ENOSYS is always executed. It covered a0 set by seccomp, so we always get ENOSYS when match seccomp RET_ERRNO rule. Fixes: f0bddf50586d ("riscv: entry: Convert to generic entry") Reported-by: Felix Yan <felixonmars@archlinux.org> Co-developed-by: Ruizhe Pan <c141028@gmail.com> Signed-off-by: Ruizhe Pan <c141028@gmail.com> Co-developed-by: Shiqi Zhang <shiqi@isrc.iscas.ac.cn> Signed-off-by: Shiqi Zhang <shiqi@isrc.iscas.ac.cn> Signed-off-by: Celeste Liu <CoelacanthusHex@gmail.com> Tested-by: Felix Yan <felixonmars@archlinux.org> --- v2 -> v3: use if-statement instead of set default value, clarify the type of syscall v1 -> v2: added explanation on why always got ENOSYS arch/riscv/kernel/traps.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)