[MODERATED] Re: [patch 13/15] Hidden 13

[Borislav Petkov <bp@suse.de>]

On Sun, May 13, 2018 at 04:01:01PM +0200, speck for Thomas Gleixner wrote:
> Subject: [patch 13/15] x86/bugs: Rework spec_ctrl base and mask logic
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> x86_spec_ctrL_mask is intended to mask out bits from a MSR_SPEC_CTRL value
> which are not to be modified. Though the implementation is not really used

s/Though/However,/

> and the bitmask is inverted for no real reason. Aside of that it is missing
> the STIBP bit if it is supported by the platform, so if the mask would be
> used in x86_virt_spec_ctrl() then it would prevent a guest from setting
> STIBP.
> 
> Add the STIBP bit if supported and use the mask in x86_spec_ctrl_set_guest()
> to sanitize the value which is supplied by the guest.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/kernel/cpu/bugs.c |   22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -68,6 +68,10 @@ void __init check_bugs(void)
>  	if (boot_cpu_has(X86_FEATURE_MSR_SPEC_CTRL))
>  		rdmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
>  
> +	/* Allow STIBP in MSR_SPEC_CTRL if supported */
> +	if (boot_cpu_has(X86_FEATURE_STIBP))
> +		x86_spec_ctrl_mask |= SPEC_CTRL_STIBP;
> +
>  	/* Select the proper spectre mitigation before patching alternatives */
>  	spectre_v2_select_mitigation();
>  
> @@ -134,19 +138,27 @@ static enum spectre_v2_mitigation spectr
>  	SPECTRE_V2_NONE;
>  
>  void
> -x86_virt_spec_ctrl(u64 guest_spec_ctrl, u64 guest_virt_spec_ctrl, bool guest)
> +x86_virt_spec_ctrl(u64 guest_spec_ctrl, u64 guest_virt_spec_ctrl, bool setguest)
>  {
>  	u64 hostssbd = ssbd_tif_to_spec_ctrl(current_thread_info()->flags);
> -	u64 msr, host = x86_spec_ctrl_base;
> +	u64 msr, guest, host = x86_spec_ctrl_base;
>  
>  	/* Is MSR_SPEC_CTRL implemented ? */
>  	if (static_cpu_has(X86_FEATURE_MSR_SPEC_CTRL)) {
> +		/*
> +		 * Restrict guest_spec_ctrl to supported values. Clear the
> +		 * modifiable bits in the host base value and or the
> +		 * modifiable bits from the guest value.
> +		 */
> +		guest = host & ~x86_spec_ctrl_mask;
> +		guest |= guest_spec_ctrl & x86_spec_ctrl_mask;
> +
>  		/* SSBD controlled in MSR_SPEC_CTRL */
>  		if (static_cpu_has(X86_FEATURE_SPEC_CTRL_SSBD))
>  			host |= hostssbd;
>  
> -		if (host != guest_spec_ctrl) {
> -			msr = guest ? guest_spec_ctrl : host;
> +		if (host != guest) {
> +			msr = setguest ? guest : host;

Just a nitpick:
			msrval = setguest ? guest : host;
			wrmsrl(MSR_IA32_SPEC_CTRL, msrval);

calling it "msrval" is a bit clearer as it shows that's you're selecting
the MSR *value* and not the MSR itself. (And yes, we do select which
MSRs to access in other places).

With that

Reviewed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--