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

[Stefan Bader]

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.

-Stefan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20180131/1ad97bf5/attachment.sig>