linux-kernel - RE: [PATCH v13 26/35] x86/fred: FRED entry/exit and dispatch 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] [thread-next>] [day] [month] [year] [list]
Message-ID: <SA1PR11MB673472A25E72022F68882869A884A@SA1PR11MB6734.namprd11.prod.outlook.com>
Date:   Wed, 6 Dec 2023 19:19:26 +0000
From:   "Li, Xin3" <xin3.li@...el.com>
To:     "andrew.cooper3@...rix.com" <andrew.cooper3@...rix.com>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "xen-devel@...ts.xenproject.org" <xen-devel@...ts.xenproject.org>
CC:     "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "x86@...nel.org" <x86@...nel.org>, "hpa@...or.com" <hpa@...or.com>,
        "Lutomirski, Andy" <luto@...nel.org>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "seanjc@...gle.com" <seanjc@...gle.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "Gross, Jurgen" <jgross@...e.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        "mhiramat@...nel.org" <mhiramat@...nel.org>,
        "jiangshanlai@...il.com" <jiangshanlai@...il.com>,
        "nik.borisov@...e.com" <nik.borisov@...e.com>,
        "Kang, Shan" <shan.kang@...el.com>
Subject: RE: [PATCH v13 26/35] x86/fred: FRED entry/exit and dispatch code

> >>> +	case X86_TRAP_OF:
> >>> +		exc_overflow(regs);
> >>> +		return;
> >>> +
> >>> +	/* INT3 */
> >>> +	case X86_TRAP_BP:
> >>> +		exc_int3(regs);
> >>> +		return;
> >> ... neither OF nor BP will ever enter fred_intx() because they're
> >> type SWEXC not SWINT.
> > Per FRED spec 5.0, section 7.3 Software Interrupts and Related Instructions:
> > INT n (opcode CD followed by an immediate byte): There are 256 such
> > software interrupt instructions, one for each value n of the immediate
> > byte (0–255).
> >
> > And appendix B Event Stack Levels:
> > If the event is an execution of INT n (opcode CD n for 8-bit value n),
> > the event stack level is 0. The event type is 4 (software interrupt)
> > and the vector is n.
> >
> > So int $0x4 and int $0x3 (use asm(".byte 0xCD, 0x03")) get here.
> >
> > But into (0xCE) and int3 (0xCC) do use event type SWEXC.
> >
> > BTW, into is NOT allowed in 64-bit mode but "int $0x4" is allowed.
> 
> There is certainly fun to be had with CD 03 and CD 04 byte patterns, but if you
> meant to mean those here, then the comments are wrong.
> 
> Vectors 3 and 4 are installed with DPL3 because that is necessary to make CC and
> CE function in userspace.  It also suggests that the SWINT vs SWEXC distinction
> was retrofitted to architecture after the 286, because exceptions don't check DPL
> and ICEBP delivers #DB from userspace even when Vector 1 has a DPL of 0.
> 
> While CC is for most cases indistinguishable from CD 03, CE behaves entirely
> differently to CD 04.  CD 04 doesn't #UD in 64bit mode, and will trigger
> exc_overflow() irrespective of the state of EFLAGS.OF.
> 
> 
> The SDM goes out of it's way to say not to use the CD 03 byte pattern (and it
> does take effort to emit this byte pattern - e.g. GAS will silently translate "int $3"
> to "int3"), and there's no plausible way software is using CD 04 in place of CE.
> 
> So why do we care about containing to make mistakes of the IDT era work in a
> FRED world?

First, I agree with you because it makes things simple and neat.

However, the latest SDM and FRED spec 5.0 both doesn't disallow it, so it
becomes an OS implementation choice.

> 
> Is there anything (other than perhaps the selftests) which would even notice?

I'm just conservative :)

If a user app can do it with IDT, we should still allow it when FRED is
enabled.  But if all key stakeholders don't care whatever gets broken
due to the change and agree to change it.

> >>> +		instrumentation_end();
> >>> +		irqentry_exit(regs, state);
> >>> +	} else {
> >>> +		common_interrupt(regs, vector);
> >>> +	}
> >>> +}
> >>> +
> >>> +static noinstr void fred_exception(struct pt_regs *regs, unsigned
> >>> +long error_code) {
> >>> +	/* Optimize for #PF. That's the only exception which matters
> >>> +performance
> >> wise */
> >>> +	if (likely(regs->fred_ss.vector == X86_TRAP_PF)) {
> >>> +		exc_page_fault(regs, error_code);
> >>> +		return;
> >>> +	}
> >>> +
> >>> +	switch (regs->fred_ss.vector) {
> >>> +	case X86_TRAP_DE: return exc_divide_error(regs);
> >>> +	case X86_TRAP_DB: return fred_exc_debug(regs);
> >>> +	case X86_TRAP_BP: return exc_int3(regs);
> >>> +	case X86_TRAP_OF: return exc_overflow(regs);
> >> Depending on what you want to do with BP/OF vs fred_intx(), this may
> >> need adjusting.
> >>
> >> If you are cross-checking type and vector, then these should be
> >> rejected for not being of type HWEXC.
> > You're right, the event type needs to be SWEXC for into and int3.
> >
> > However, would it be overkilling?  Assuming hardware and VMM are sane.
> 
> You either care about cross checking, or not.  Right now, this patch is a mix of the
> two approaches.
> 
> In my opinion, cross-checking is the better approach, because it means that
> violations of the assumptions get noticed more quickly, and hopefully by
> whomever is working on the new feature which alters the assumptions.

Yeah, I can make the change.

Thanks!
    Xin