Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <f0240e15-223a-7600-4494-7a0a75155bdb@zytor.com>
 <F9F5E9D4-C1EE-455A-A6B1-4DF9D349BBAA@amacapital.net> <06a5e088-b0e6-c65e-73e6-edc740aa4256@zytor.com>
In-Reply-To: <06a5e088-b0e6-c65e-73e6-edc740aa4256@zytor.com>
From:   Andy Lutomirski <luto@amacapital.net>
Date:   Tue, 27 Apr 2021 16:23:06 -0700
Message-ID: <CALCETrW7Vu5ZU-Lv4RRG5DSGxMBJmDMqpvP7kqO16DwajproBQ@mail.gmail.com>
Subject: Re: pt_regs->ax == -ENOSYS
To:     "H. Peter Anvin" <hpa@zytor.com>
Cc:     Linus Torvalds <torvalds@linux-foundation.org>,
        Ingo Molnar <mingo@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Andrew Lutomirski <luto@kernel.org>,
        Borislav Petkov <bp@alien8.de>,
        LKML <linux-kernel@vger.kernel.org>,
        Oleg Nesterov <oleg@redhat.com>,
        Kees Cook <keescook@chromium.org>,
        Will Drewry <wad@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Apr 27, 2021 at 3:58 PM H. Peter Anvin <hpa@zytor.com> wrote:
>
> On 4/27/21 2:28 PM, Andy Lutomirski wrote:
> >
> >> On Apr 27, 2021, at 2:15 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> >>
> >> =EF=BB=BFTrying to stomp out some possible cargo cult programming?
> >>
> >> In the process of going through the various entry code paths, I have t=
o admit to being a bit confused why pt_regs->ax is set to -ENOSYS very earl=
y in the system call path.
> >>
> >
> > It has to get set to _something_, and copying orig_ax seems perhaps sil=
ly.  There could also be code that relies on ptrace poking -1 into the nr r=
esulting in -ENOSYS.
> >
>
> Yeah. I obviously ran into this working on the common entry-exit code
> for FRED; the frame has annoyingly different formats because of this,
> and I wanted to avoid slowing down the system call path.
>
> >> What is perhaps even more confusing is:
> >>
> >> __visible noinstr void do_syscall_64(struct pt_regs *regs, unsigned lo=
ng nr)
> >> {
> >>         nr =3D syscall_enter_from_user_mode(regs, nr);
> >>
> >>         instrumentation_begin();
> >>         if (likely(nr < NR_syscalls)) {
> >>                 nr =3D array_index_nospec(nr, NR_syscalls);
> >>                 regs->ax =3D 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 =3D array_index_nospec(nr & ~__X32_SYSCALL_BIT,
> >>                                         X32_NR_syscalls);
> >>                 regs->ax =3D 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 va=
lue(!) 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=E2=80=99re right.
> >
> >>
> >> Calling regs->ax =3D 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) =3D=3D -1 or < 0.
> >
> > I think the check should be -1 for 64 bit but (u32)nr =3D=3D (u32)-1 fo=
r the 32-bit path. Does that seem reasonable?
>
> I'm thinking overall that depending on 64-bit %rax is once again a
> mistake; I realize that the assembly code that did that kept breaking
> because people messed with it, but we still have:
>
> /*
>   * Only the low 32 bits of orig_ax are meaningful, so we return int.
>   * This importantly ignores the high bits on 64-bit, so comparisons
>   * sign-extend the low 32 bits.
>   */
> static inline int syscall_get_nr(struct task_struct *task, struct
> pt_regs *regs)
> {
>          return regs->orig_ax;
> }
>
> "Different interpretation of the same data" is a notorious security
> trap. Zero-extending orig_ax would cause different behavior on 32 and 64
> bits and differ from the above, so I'm thinking that just once and for
> all defining the system call number as a signed int for all the x86 ABIs
> would be the sanest.
>
> It still doesn't really answer the question if "movq $-1,%rax; syscall"
> or "movl $-1,%eax; syscall" could somehow cause bad things to happen,
> though, which makes me a little bit nervous still.
>

I much prefer the model of saying that the bits that make sense for
the syscall type (all 64 for 64-bit SYSCALL and the low 32 for
everything else) are all valid.  This way there are no weird reserved
bits, no weird ptrace() interactions, etc.  I'm a tiny bit concerned
that this would result in a backwards compatibility issue, but not
very.  This would involve changing syscall_get_nr(), but that doesn't
seem so bad.  The biggest problem is that seccomp hardcoded syscall
nrs to 32 bit.

An alternative would be to declare that we always truncate to 32 bits,
except that 64-bit SYSCALL with high bits set is an error and results
in ENOSYS. The ptrace interaction there is potentially nasty.

Basically, all choices here kind of suck, and I haven't done a real
analysis of all the issues...

--Andy