linux-kernel - Re: [PATCH v1 3/3] x86/entry: Set FRED RSP0 on return to userspace instead of context switch

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: <87ttfw18jy.ffs@tglx>
Date: Wed, 07 Aug 2024 20:39:45 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: "Xin Li (Intel)" <xin@...or.com>, linux-kernel@...r.kernel.org
Cc: mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
 x86@...nel.org, hpa@...or.com, peterz@...radead.org,
 andrew.cooper3@...rix.com, seanjc@...gle.com
Subject: Re: [PATCH v1 3/3] x86/entry: Set FRED RSP0 on return to userspace
 instead of context switch

On Tue, Aug 06 2024 at 22:47, Xin Li wrote:
> --- a/arch/x86/include/asm/entry-common.h
> +++ b/arch/x86/include/asm/entry-common.h
> @@ -51,6 +51,11 @@ static inline void arch_exit_to_user_mode_prepare(struct pt_regs *regs,
>  		if (ti_work & _TIF_USER_RETURN_NOTIFY)
>  			fire_user_return_notifiers();
>  
> +		if (cpu_feature_enabled(X86_FEATURE_FRED) &&
> +		    (ti_work & _TIF_LOAD_USER_STATES))
> +			wrmsrns(MSR_IA32_FRED_RSP0,
> +				(unsigned long)task_stack_page(current) + THREAD_SIZE);

Eyes are bleeding. If you do the inline in patch 1 then this becomes

	if (cpu_feature_enabled(X86_FEATURE_FRED) && (ti_work & _TIF_LOAD_USER_STATES))
		wrmsrns(MSR_IA32_FRED_RSP0, (unsigned long)task_stack_page(current) + THREAD_SIZE);

which is in the 100 character limit and readable.

Though I wonder if this should not have a per CPU cache for that.

        if (cpu_feature_enabled(X86_FEATURE_FRED) {
        	unsigned long rsp0 = (unsigned long) task_stack_page(current) + THREAD_SIZE;

		if (__this_cpu_read(cpu_fred_rsp0) != rsp0) {
			wrmsrns(MSR_IA32_FRED_RSP0, fred_rsp0);
                        this_cpu_write(cpu_fred_rsp0, rsp0);
                }
        }	

Hmm?

>  		if (unlikely(ti_work & _TIF_IO_BITMAP))
>  			tss_update_io_bitmap();
>  
> diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
> index c3bd0c0758c9..a31ea544cc0e 100644
> --- a/arch/x86/include/asm/switch_to.h
> +++ b/arch/x86/include/asm/switch_to.h
> @@ -71,8 +71,7 @@ static inline void update_task_stack(struct task_struct *task)
>  	this_cpu_write(cpu_tss_rw.x86_tss.sp1, task->thread.sp0);
>  #else
>  	if (cpu_feature_enabled(X86_FEATURE_FRED)) {
> -		/* WRMSRNS is a baseline feature for FRED. */

This comment is wrong when using the alternative WRMSR fallback and
should be remove by the alternatives patch.

> -		wrmsrns(MSR_IA32_FRED_RSP0, (unsigned long)task_stack_page(task) + THREAD_SIZE);
> +		set_thread_flag(TIF_LOAD_USER_STATES);

Thanks,

        tglx