(probably NACK): [SRU][Trusty][PATCH] x86/asm/msr: Make wrmsrl_safe() a function

[Khaled Elmously]

On 2018-01-31 08:08:59 , Stefan Bader wrote:
> On 30.01.2018 18:51, Khaled Elmously wrote:
> > On 2018-01-25 10:41:55 , Juerg Haefliger wrote:
> >> From: Andy Lutomirski <luto at kernel.org>
> >>
> >> CVE-2017-5753
> >> CVE-2017-5715
> >>
> >> The wrmsrl_safe macro performs invalid shifts if the value
> >> argument is 32 bits.  This makes it unnecessarily awkward to
> >> write code that puts an unsigned long into an MSR.
> >>
> >> Convert it to a real inline function.
> >>
> >> For inspiration, see:
> >>
> >>   7c74d5b7b7b6 ("x86/asm/entry/64: Fix MSR_IA32_SYSENTER_CS MSR value").
> >>
> >> Signed-off-by: Andy Lutomirski <luto at kernel.org>
> >> Cc: <linux-kernel at vger.kernel.org>
> >> Cc: Andrew Morton <akpm at linux-foundation.org>
> >> Cc: H. Peter Anvin <hpa at zytor.com>
> >> Cc: Linus Torvalds <torvalds at linux-foundation.org>
> >> Cc: Peter Zijlstra <peterz at infradead.org>
> >> Cc: Thomas Gleixner <tglx at linutronix.de>
> >> [ Applied small improvements. ]
> >> Signed-off-by: Ingo Molnar <mingo at kernel.org>
> >> (cherry picked from commit cf991de2f614f454b3cb2a300c06ecdf69f3a70d)
> >> Signed-off-by: Juerg Haefliger <juerg.haefliger at canonical.com>
> >> ---
> >>  arch/x86/include/asm/msr.h | 9 +++++++--
> >>  1 file changed, 7 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
> >> index de36f22eb0b9..13bf576c401d 100644
> >> --- a/arch/x86/include/asm/msr.h
> >> +++ b/arch/x86/include/asm/msr.h
> >> @@ -205,8 +205,13 @@ do {                                                            \
> >>  
> >>  #endif	/* !CONFIG_PARAVIRT */
> >>  
> >> -#define wrmsrl_safe(msr, val) wrmsr_safe((msr), (u32)(val),		\
> >> -					     (u32)((val) >> 32))
> >> +/*
> >> + * 64-bit version of wrmsr_safe():
> >> + */
> >> +static inline int wrmsrl_safe(u32 msr, u64 val)
> >> +{
> >> +	return wrmsr_safe(msr, (u32)val,  (u32)(val >> 32));
> >> +}
> >>  
> >>  #define write_tsc(low, high) wrmsr(MSR_IA32_TSC, (low), (high))
> >>
> > 
> > Sorry if this is a stupid question, but: Could you please clarify how you know that this patch is actually related to Spectre?
> > 
> > The CVE pages for CVE-2017-5753 and CVE-2017-5715 (Spectre v1 and v2) don't mention this patch as a fix, the names on the patch don't align with those who worked on the Spectre mitigations, the upstream patch is from June 2015 (long before Spectre) and the change itself seems unrelated to speculative execution/prediction.
> > 
> > Are you sure you have the correct CVE references?
> > 
> It is a precaution that got attributed to the CVE fix. Most likely I would say
> because of:
> 
> commit 6d6db0aaffb3cb905df9850bab2654b80506f3b4
> Author: Borislav Petkov <bp at suse.de>
> Date:   Sun Mar 9 18:05:23 2014 +0100
> 
>     x86: Add another set of MSR accessor functions
> 
> That was part of pre-requisites to allow the spectre mitigations we had to apply
> more cleanly. The patch defines msr_(set|clear)_bit() functions and those via
> __flip_bit() use msr_write()->wrmsrl_safe().
> 
> So attributing it to the CVE is ok as it improves the pre-reqs backported for
> the mitigation patches.
> 

I see. That makes sense, thanks for the explanation, and sorry for the false alarm.

> -Stefan
> 

-Khaled