| 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
| ||
|
Message-ID: <87cz2a6n72.ffs@tglx>
Date: Mon, 05 Jun 2023 15:41:37 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Xin Li <xin3.li@...el.com>, linux-kernel@...r.kernel.org,
x86@...nel.org, kvm@...r.kernel.org
Cc: mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
hpa@...or.com, peterz@...radead.org, andrew.cooper3@...rix.com,
seanjc@...gle.com, pbonzini@...hat.com, ravi.v.shankar@...el.com,
jiangshanlai@...il.com, shan.kang@...el.com
Subject: Re: [PATCH v8 21/33] x86/fred: FRED initialization code
On Mon, Apr 10 2023 at 01:14, Xin Li wrote:
>
> +/*
> + * The actual assembly entry and exit points
> + */
> +extern __visible void fred_entrypoint_user(void);
Why is this defined in this patch and not at the point where the
function is introduced?
> +/*
> + * Initialization
> + */
> +void cpu_init_fred_exceptions(void);
> +void fred_setup_apic(void);
> +
> #endif /* __ASSEMBLY__ */
>
> +#else
> +#define cpu_init_fred_exceptions() BUG()
> +#define fred_setup_apic() BUG()
static inline stubs please.
> @@ -2054,28 +2055,6 @@ static void wrmsrl_cstar(unsigned long val)
> /* May not be marked __init: used by software suspend */
> void syscall_init(void)
> {
> - wrmsr(MSR_STAR, 0, (__USER32_CS << 16) | __KERNEL_CS);
> - wrmsrl(MSR_LSTAR, (unsigned long)entry_SYSCALL_64);
> -
> -#ifdef CONFIG_IA32_EMULATION
> - wrmsrl_cstar((unsigned long)entry_SYSCALL_compat);
> - /*
> - * This only works on Intel CPUs.
> - * On AMD CPUs these MSRs are 32-bit, CPU truncates MSR_IA32_SYSENTER_EIP.
> - * This does not cause SYSENTER to jump to the wrong location, because
> - * AMD doesn't allow SYSENTER in long mode (either 32- or 64-bit).
> - */
> - wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)__KERNEL_CS);
> - wrmsrl_safe(MSR_IA32_SYSENTER_ESP,
> - (unsigned long)(cpu_entry_stack(smp_processor_id()) + 1));
> - wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (u64)entry_SYSENTER_compat);
> -#else
> - wrmsrl_cstar((unsigned long)ignore_sysret);
> - wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)GDT_ENTRY_INVALID_SEG);
> - wrmsrl_safe(MSR_IA32_SYSENTER_ESP, 0ULL);
> - wrmsrl_safe(MSR_IA32_SYSENTER_EIP, 0ULL);
> -#endif
> -
> /*
> * Flags to clear on syscall; clear as much as possible
> * to minimize user space-kernel interference.
> @@ -2086,6 +2065,41 @@ void syscall_init(void)
> X86_EFLAGS_IF|X86_EFLAGS_DF|X86_EFLAGS_OF|
> X86_EFLAGS_IOPL|X86_EFLAGS_NT|X86_EFLAGS_RF|
> X86_EFLAGS_AC|X86_EFLAGS_ID);
> +
> + /*
> + * The default user and kernel segments
> + */
> + wrmsr(MSR_STAR, 0, (__USER32_CS << 16) | __KERNEL_CS);
> +
> + if (cpu_feature_enabled(X86_FEATURE_FRED)) {
> + /* Both sysexit and sysret cause #UD when FRED is enabled */
> + wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)GDT_ENTRY_INVALID_SEG);
> + wrmsrl_safe(MSR_IA32_SYSENTER_ESP, 0ULL);
> + wrmsrl_safe(MSR_IA32_SYSENTER_EIP, 0ULL);
> + } else {
> + wrmsrl(MSR_LSTAR, (unsigned long)entry_SYSCALL_64);
> +
> +#ifdef CONFIG_IA32_EMULATION
> + wrmsrl_cstar((unsigned long)entry_SYSCALL_compat);
> + /*
> + * This only works on Intel CPUs.
> + * On AMD CPUs these MSRs are 32-bit, CPU truncates
> + * MSR_IA32_SYSENTER_EIP.
> + * This does not cause SYSENTER to jump to the wrong
> + * location, because AMD doesn't allow SYSENTER in
> + * long mode (either 32- or 64-bit).
> + */
> + wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)__KERNEL_CS);
> + wrmsrl_safe(MSR_IA32_SYSENTER_ESP,
> + (unsigned long)(cpu_entry_stack(smp_processor_id()) + 1));
> + wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (u64)entry_SYSENTER_compat);
> +#else
> + wrmsrl_cstar((unsigned long)ignore_sysret);
> + wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)GDT_ENTRY_INVALID_SEG);
> + wrmsrl_safe(MSR_IA32_SYSENTER_ESP, 0ULL);
> + wrmsrl_safe(MSR_IA32_SYSENTER_EIP, 0ULL);
> +#endif
> + }
> }
Sigh. Can you please split this into
static void idt_syscall_init(void)
{
All the existing gunk
}
void syscall_init(void)
{
/* The default user and kernel segments */
wrmsr(MSR_STAR, 0, (__USER32_CS << 16) | __KERNEL_CS);
idt_syscall_init();
}
in a first step and then in the next patch add the FRED muck?
> +/*
> + * Initialize FRED on this CPU. This cannot be __init as it is called
> + * during CPU hotplug.
Really no need to repeat this comment vs. __init all over the place.
> + */
> +void cpu_init_fred_exceptions(void)
> +{
> + wrmsrl(MSR_IA32_FRED_CONFIG,
> + FRED_CONFIG_REDZONE | /* Reserve for CALL emulation */
Please don't use tail comments. Nowhere.
> + FRED_CONFIG_INT_STKLVL(0) |
> + FRED_CONFIG_ENTRYPOINT(fred_entrypoint_user));
> +
> +/*
> + * Initialize system vectors from a FRED perspective, so
> + * lapic_assign_system_vectors() can do its job.
> + */
> +void __init fred_setup_apic(void)
> +{
> + int i;
> +
> + for (i = 0; i < FIRST_EXTERNAL_VECTOR; i++)
> + set_bit(i, system_vectors);
> + /*
> + * Don't set the non assigned system vectors in the
> + * system_vectors bitmap. Otherwise they show up in
> + * /proc/interrupts.
> + */
> +#ifdef CONFIG_SMP
> + set_bit(IRQ_MOVE_CLEANUP_VECTOR, system_vectors);
> +#endif
> +
> + for (i = 0; i < NR_SYSTEM_VECTORS; i++) {
> + if (get_system_interrupt_handler(i) != NULL) {
This _cannot be NULL. The system vector table must be fully populated
with either the real handler or the spurious handler. Otherwise you need
a NULL pointer check in the dispatch path.
> + set_bit(i + FIRST_SYSTEM_VECTOR, system_vectors);
> + }
> + }
> +
> + /* The rest are fair game... */
Can you please refrain from adding useless comments. Commenting the
obvious is a distraction and not helpful in any way. Comment the things
which are not obvious in the first place.
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -1537,6 +1537,14 @@ static system_interrupt_handler system_interrupt_handlers[NR_SYSTEM_VECTORS] = {
>
> #undef SYSV
>
> +system_interrupt_handler get_system_interrupt_handler(unsigned int i)
> +{
> + if (i >= NR_SYSTEM_VECTORS)
> + return NULL;
Seriously?
> + return system_interrupt_handlers[i];
Get rid of this completely confusing and useless function and look the
table up at the only call site. I'm all for defensive programming, but
this is hideous.
Thanks,
tglx
Powered by blists - more mailing lists