RE: [PATCH v13 26/35] x86/fred: FRED entry/exit and dispatch code

From: "Li, Xin3" <xin3.li@intel.com>
To: "andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: "tglx@linutronix.de" <tglx@linutronix.de>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"bp@alien8.de" <bp@alien8.de>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	"x86@kernel.org" <x86@kernel.org>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"Lutomirski, Andy" <luto@kernel.org>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>,
	"seanjc@google.com" <seanjc@google.com>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"Gross, Jurgen" <jgross@suse.com>,
	"Shankar, Ravi V" <ravi.v.shankar@intel.com>,
	"mhiramat@kernel.org" <mhiramat@kernel.org>,
	"jiangshanlai@gmail.com" <jiangshanlai@gmail.com>,
	"nik.borisov@suse.com" <nik.borisov@suse.com>,
	"Kang, Shan" <shan.kang@intel.com>
Subject: RE: [PATCH v13 26/35] x86/fred: FRED entry/exit and dispatch code
Date: Tue, 5 Dec 2023 19:03:10 +0000	[thread overview]
Message-ID: <SA1PR11MB67340C4246F7BED4D0B09837A885A@SA1PR11MB6734.namprd11.prod.outlook.com> (raw)
In-Reply-To: <f260ddf9-be67-48e0-8121-6f58d46f7978@citrix.com>

> > +static noinstr void fred_intx(struct pt_regs *regs) {
> > +	switch (regs->fred_ss.vector) {
> > +	/* INT0 */
> 
> INTO (for overflow), not INT-zero.  However...
> 
> > +	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.
> 
> SWINT is strictly the INT $imm8 instruction.
> 
> > ...
> > +static noinstr void fred_extint(struct pt_regs *regs) {
> > +	unsigned int vector = regs->fred_ss.vector;
> > +
> > +	if (WARN_ON_ONCE(vector < FIRST_EXTERNAL_VECTOR))
> > +		return;
> > +
> > +	if (likely(vector >= FIRST_SYSTEM_VECTOR)) {
> > +		irqentry_state_t state = irqentry_enter(regs);
> > +
> > +		instrumentation_begin();
> > +		sysvec_table[vector - FIRST_SYSTEM_VECTOR](regs);
> 
> array_index_mask_nospec()
> 
> This is easy for an attacker to abuse, to install non-function-pointer targets into
> the indirect predictor.
> 
> > +		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.
> 
> > +	case X86_TRAP_BR: return exc_bounds(regs);
> > +	case X86_TRAP_UD: return exc_invalid_op(regs);
> > +	case X86_TRAP_NM: return exc_device_not_available(regs);
> > +	case X86_TRAP_DF: return exc_double_fault(regs, error_code);
> > +	case X86_TRAP_TS: return exc_invalid_tss(regs, error_code);
> > +	case X86_TRAP_NP: return exc_segment_not_present(regs, error_code);
> > +	case X86_TRAP_SS: return exc_stack_segment(regs, error_code);
> > +	case X86_TRAP_GP: return exc_general_protection(regs, error_code);
> > +	case X86_TRAP_MF: return exc_coprocessor_error(regs);
> > +	case X86_TRAP_AC: return exc_alignment_check(regs, error_code);
> > +	case X86_TRAP_XF: return exc_simd_coprocessor_error(regs);
> > +
> > +#ifdef CONFIG_X86_MCE
> > +	case X86_TRAP_MC: return fred_exc_machine_check(regs); #endif #ifdef
> > +CONFIG_INTEL_TDX_GUEST
> > +	case X86_TRAP_VE: return exc_virtualization_exception(regs);
> > +#endif
> > +#ifdef CONFIG_X86_KERNEL_IBT
> 
> CONFIG_X86_CET
> 
> Userspace can use CET even if the kernel isn't compiled with IBT, so this
> exception needs handling.
> 
> > +	case X86_TRAP_CP: return exc_control_protection(regs, error_code);
> > +#endif
> > +	default: return fred_bad_type(regs, error_code);
> > +	}
> > +}
> > +
> > +__visible noinstr void fred_entry_from_user(struct pt_regs *regs) {
> > +	unsigned long error_code = regs->orig_ax;
> > +
> > +	/* Invalidate orig_ax so that syscall_get_nr() works correctly */
> > +	regs->orig_ax = -1;
> > +
> > +	switch (regs->fred_ss.type) {
> > +	case EVENT_TYPE_EXTINT:
> > +		return fred_extint(regs);
> > +	case EVENT_TYPE_NMI:
> > +		return fred_exc_nmi(regs);
> > +	case EVENT_TYPE_SWINT:
> > +		return fred_intx(regs);
> > +	case EVENT_TYPE_HWEXC:
> > +	case EVENT_TYPE_SWEXC:
> > +	case EVENT_TYPE_PRIV_SWEXC:
> > +		return fred_exception(regs, error_code);
> 
> PRIV_SWEXC should have it's own function and not fall into fred_exception().
> 
> It is strictly only the ICEBP (INT1) instruction at the moment, so should fall into
> bad_type() for any vector other than X86_TRAP_DB.
> 
> > +	case EVENT_TYPE_OTHER:
> > +		return fred_other(regs);
> > +	default:
> > +		return fred_bad_type(regs, error_code);
> > +	}
> > +}
> 
> ~Andrew


Thanks a lot for your quick review, will address soon.
    Xin