linux-kernel - Re: [RFC PATCH 1/1] x86/traps: Get rid of exception handlers' second argument error code

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] [day] [month] [year] [list]

Date:   Sat, 05 Aug 2023 23:02:59 +0200
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Peter Zijlstra <peterz@...radead.org>,
        "Li, Xin3" <xin3.li@...el.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>,
        "Lutomirski, Andy" <luto@...nel.org>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "H . Peter Anvin" <hpa@...or.com>,
        "Gross, Jurgen" <jgross@...e.com>,
        "Ostrovsky, Boris" <boris.ostrovsky@...cle.com>
Subject: Re: [RFC PATCH 1/1] x86/traps: Get rid of exception handlers'
 second argument error code

On Fri, Aug 04 2023 at 21:01, Peter Zijlstra wrote:
> On Fri, Aug 04, 2023 at 05:35:11PM +0000, Li, Xin3 wrote:
>> > > The commit d99015b1abbad ("x86: move entry_64.S register saving out of
>> > > the macros") introduced the changes to set orig_ax to -1, but I can't
>> > > see why it's required. Our tests on x86_64 and x86_32 seem fine if
>> > > orig_ax is left unchanged instead of set to -1.
>> > 
>> > That means that SYSCALL_NUM(regs) get to be garbage; or something like that.
>> 
>> I find SYSCALL_NUM(regs) in tools/testing/selftests/seccomp/seccomp_bpf.c,
>> but nothing obvious to me.
>> 
>> I think it's clear that once exceptions and IRQs are handled, the original
>> context will be fully recovered in a normal case.
>> 
>> Is it related to preemption after such a event?
>> 
>> I must have missed something; can you please elaborate it?
>
> arch/x86/include/asm/syscall.h
>
> syscall_get_nr() syscall_rollback()

Specifically it breaks signal handling. See arch_do_signal_or_restart()
and handle_signal().

So, no. This cannot be changed nilly willy especially not because
orig_ax is part of the UABI.

Even if it would work, this patch is completely unreviewable. There are
smarter ways to make such a change in digestable chunks.

I also looked at the latest FRED series and that's broken vs. orig_ax in
the same way.

Aside of it the series is not applying to any tree I'm aware of. It
fails because its based on a tree which has IRQ_MOVE_CLEANUP_VECTOR
removed, but that's nowhere in mainline or tip. This is not the way it
works, really.

Back to the problem at hand. I'm absolutely not understanding why this
all is so complicated.

FRED pushs the error code or 0 (in case there is no error code) right
into orig_ax. That's where the ERET[US] frame starts. ERT[US] do not
care at all about the error code location, they skip it by adding 8 to
RSP.

So the obvious thing to do is:

fred_entry_from_xxxx(struct pt_regs *regs)
{
        unsigned long error_code = regs->orig_ax;

        regs->orig_ax = -1;

        dispatch_1st_level(regs->type, regs, error_code);
}

Now the dispatching code in your series looks nice and shiny with all
those tables, but in practice it's not so shiny at all.

For one this code forces indirect calls even in places where there are
no indirect calls required.

But what's worse it causes you to make everything uniform one way or the
other and adding all this dispatch_table_##func muck, which is fully
duplicated code for absolutely zero reason. That's error prone and a
maintainence mess.

The vast majority of exceptions, interrupts whatever is not special at
all neither on FRED nor on IDT. The ones which need special treatment
are those which store additional information in the stack frame and
these are the exception - not the rule. And those require special
treatment anyway whether you make the rest look uniform or not.

IOW, your approach of making everything uniform is completely
wrong. There is a reason why some system vectors are not using
DEFINE_IDTENTRY_SYSVEC().  Particularly the RESCHEDULE_VECTOR. This has
been performance optimized with a lot of effort and no, we are not going
to sacrifice that just because it makes it sooo simple for FRED.

This is not about simplicity. This is about sane and maintainable code
which preserves the carefully optimized code paths which are hotpath no
matter whether there is IDT or FRED.

I'm going to reply on your last failed resend attempt after creating a
sane mail thread out of the mess you created (including the insanely
large and inappropriate cc list).

Thanks,

        tglx