linux-kernel - Re: pt_regs->ax == -ENOSYS

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]

Date:   Tue, 27 Apr 2021 14:28:29 -0700
From:   Andy Lutomirski <luto@...capital.net>
To:     "H. Peter Anvin" <hpa@...or.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andrew Lutomirski <luto@...nel.org>,
        Borislav Petkov <bp@...en8.de>, linux-kernel@...r.kernel.org,
        oleg@...hat.com, Kees Cook <keescook@...omium.org>,
        Will Drewry <wad@...omium.org>
Subject: Re: pt_regs->ax == -ENOSYS


> On Apr 27, 2021, at 2:15 PM, H. Peter Anvin <hpa@...or.com> wrote:
> 
> Trying to stomp out some possible cargo cult programming?
> 
> In the process of going through the various entry code paths, I have to admit to being a bit confused why pt_regs->ax is set to -ENOSYS very early in the system call path.
> 

It has to get set to _something_, and copying orig_ax seems perhaps silly.  There could also be code that relies on ptrace poking -1 into the nr resulting in -ENOSYS.

> What is perhaps even more confusing is:
> 
> __visible noinstr void do_syscall_64(struct pt_regs *regs, unsigned long nr)
> {
>        nr = syscall_enter_from_user_mode(regs, nr);
> 
>        instrumentation_begin();
>        if (likely(nr < NR_syscalls)) {
>                nr = array_index_nospec(nr, NR_syscalls);
>                regs->ax = sys_call_table[nr](regs);
> #ifdef CONFIG_X86_X32_ABI
>        } else if (likely((nr & __X32_SYSCALL_BIT) &&
>                          (nr & ~__X32_SYSCALL_BIT) < X32_NR_syscalls)) {
>                nr = array_index_nospec(nr & ~__X32_SYSCALL_BIT,
>                                        X32_NR_syscalls);
>                regs->ax = x32_sys_call_table[nr](regs);
> #endif
>        }
>        instrumentation_end();
>        syscall_exit_to_user_mode(regs);
> }
> #endif
> 
> Now, unless I'm completely out to sea, it seems to me that if syscall_enter_from_user_mode() changes the system call number to an invalid number and pt_regs->ax to !-ENOSYS then the system call will return a different value(!) depending on if it is out of range for the table (whatever was poked into pt_regs->ax) or if it corresponds to a hole in the table. This seems to me at least to be The Wrong Thing.

I think you’re right.

> 
> Calling regs->ax = sys_ni_syscall() in an else clause would arguably be the right thing here, except possibly in the case where nr (or (int)nr, see below) == -1 or < 0.

I think the check should be -1 for 64 bit but (u32)nr == (u32)-1 for the 32-bit path. Does that seem reasonable?

> 
> Now, syscall_get_nr() returns the low 32 bits of the system call number unconditionally. There are places where we look at the sign of this number, which means that 0xffffffff7fffffff is "positive" and 0x7fffffffffffffff is "negative". We have gone back and forth more than once on if we should look at %rax or just %eax on a system call... I have to admit that the current design makes me a bit nervous.
> 
> Finally, can anything bad happen in some weird corner case inside one of the syscall_*_mode() calls or after an interrupt if someone tries to call syscall(-1) or another negative number?
> 
> Food for thought or just my not being up to date?
> 
> Thanks,
> 
>    -hpa
>