| Message ID | 20180125094155.31241-1-juerg.haefliger@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,Trusty] x86/asm/msr: Make wrmsrl_safe() a function | expand |
On 25/01/18 09:41, Juerg Haefliger wrote: > From: Andy Lutomirski <luto@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@kernel.org> > Cc: <linux-kernel@vger.kernel.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: H. Peter Anvin <hpa@zytor.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > [ Applied small improvements. ] > Signed-off-by: Ingo Molnar <mingo@kernel.org> > (cherry picked from commit cf991de2f614f454b3cb2a300c06ecdf69f3a70d) > Signed-off-by: Juerg Haefliger <juerg.haefliger@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)) > > Makes sense. Looks good to me. Acked-by: Colin Ian King <colin.king@canonical.com>
On 2018-01-25 10:41:55 , Juerg Haefliger wrote: > From: Andy Lutomirski <luto@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@kernel.org> > Cc: <linux-kernel@vger.kernel.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: H. Peter Anvin <hpa@zytor.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > [ Applied small improvements. ] > Signed-off-by: Ingo Molnar <mingo@kernel.org> > (cherry picked from commit cf991de2f614f454b3cb2a300c06ecdf69f3a70d) > Signed-off-by: Juerg Haefliger <juerg.haefliger@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?
On 30.01.2018 18:51, Khaled Elmously wrote: > On 2018-01-25 10:41:55 , Juerg Haefliger wrote: >> From: Andy Lutomirski <luto@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@kernel.org> >> Cc: <linux-kernel@vger.kernel.org> >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: H. Peter Anvin <hpa@zytor.com> >> Cc: Linus Torvalds <torvalds@linux-foundation.org> >> Cc: Peter Zijlstra <peterz@infradead.org> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> [ Applied small improvements. ] >> Signed-off-by: Ingo Molnar <mingo@kernel.org> >> (cherry picked from commit cf991de2f614f454b3cb2a300c06ecdf69f3a70d) >> Signed-off-by: Juerg Haefliger <juerg.haefliger@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@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
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@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@kernel.org> > >> Cc: <linux-kernel@vger.kernel.org> > >> Cc: Andrew Morton <akpm@linux-foundation.org> > >> Cc: H. Peter Anvin <hpa@zytor.com> > >> Cc: Linus Torvalds <torvalds@linux-foundation.org> > >> Cc: Peter Zijlstra <peterz@infradead.org> > >> Cc: Thomas Gleixner <tglx@linutronix.de> > >> [ Applied small improvements. ] > >> Signed-off-by: Ingo Molnar <mingo@kernel.org> > >> (cherry picked from commit cf991de2f614f454b3cb2a300c06ecdf69f3a70d) > >> Signed-off-by: Juerg Haefliger <juerg.haefliger@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@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
On 2018-01-25 10:41:55 , Juerg Haefliger wrote: > From: Andy Lutomirski <luto@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@kernel.org> > Cc: <linux-kernel@vger.kernel.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: H. Peter Anvin <hpa@zytor.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > [ Applied small improvements. ] > Signed-off-by: Ingo Molnar <mingo@kernel.org> > (cherry picked from commit cf991de2f614f454b3cb2a300c06ecdf69f3a70d) > Signed-off-by: Juerg Haefliger <juerg.haefliger@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)) > Please disregard my earlier "probably NACK". Acked-by: Khalid Elmously <khalid.elmously@canonical.com>
On 25.01.2018 10:41, Juerg Haefliger wrote: > From: Andy Lutomirski <luto@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@kernel.org> > Cc: <linux-kernel@vger.kernel.org> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: H. Peter Anvin <hpa@zytor.com> > Cc: Linus Torvalds <torvalds@linux-foundation.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > [ Applied small improvements. ] > Signed-off-by: Ingo Molnar <mingo@kernel.org> > (cherry picked from commit cf991de2f614f454b3cb2a300c06ecdf69f3a70d) > Signed-off-by: Juerg Haefliger <juerg.haefliger@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)) > >
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))