I just got this assigned to amd64-microcode in Debian, but it is something
that needs to be worked around by the EFI/BIOS and/or the kernel.
Since we all know just how well it pans out to depend on BIOS/EFI updates
for *anything*, I'm raising the issue here. IMHO looks like it would be
worthwhile to either set the relevant MSR in the kernel if the BIOS didn't
do it, or at least warn the user of the need for a BIOS/EFI update...
It is the usual hangs-core type of CPU errata (therefore, the best type
since it won't cause silent data corruption). gcc-produced code managed to
trigger it (in DragonFly BSD).
A quick search under arch/x86 did not locate any existing workaround for
this issue.
Date: Wed, 27 Nov 2013 21:23:37 -0500 (EST)
From: [email protected]
To: [email protected]
Cc: [email protected]
Subject: CVE-2013-6885 AMD Publ. 51810 Errata 793 system hang
The person who requested CVE-2013-6885 asked that we send the CVE
assignment here because various open-source software will probably be
adding code to prevent this denial of service attack.
http://support.amd.com/TechDocs/51810_16h_00h-0Fh_Rev_Guide.pdf
http://lists.dragonflybsd.org/pipermail/kernel/2011-December/046594.html
http://www.zdnet.com/blog/hardware/amd-owns-up-to-cpu-bug/18924
793 Specific Combination of Writes to Write Combined Memory
Types and Locked Instructions May Cause Core Hang
Under a highly specific and detailed set of internal timing
conditions, a locked instruction may trigger a timing sequence whereby
the write to a write combined memory type is not flushed, causing the
locked instruction to stall indefinitely.
Potential Effect on System
Processor core hang.
Suggested Workaround
BIOS should set MSRC001_1020[15] = 1b.
No fix planned
- --
CVE assignment team, MITRE CVE Numbering Authority
M/S M300
202 Burlington Road, Bedford, MA 01730 USA
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On Tue, Jan 14, 2014 at 09:41:33AM -0200, Henrique de Moraes Holschuh wrote:
> I just got this assigned to amd64-microcode in Debian, but it is something
> that needs to be worked around by the EFI/BIOS and/or the kernel.
>
> Since we all know just how well it pans out to depend on BIOS/EFI updates
> for *anything*, I'm raising the issue here. IMHO looks like it would be
> worthwhile to either set the relevant MSR in the kernel if the BIOS didn't
> do it, or at least warn the user of the need for a BIOS/EFI update...
>
> It is the usual hangs-core type of CPU errata (therefore, the best type
> since it won't cause silent data corruption). gcc-produced code managed to
> trigger it (in DragonFly BSD).
I think this is a different issue than the dragonfly issue. In any case,
if AMD delivers all BIOS with this workaround enabled, we don't need to
do anything. And I asked them about it so we'll have an answer soon, I
hope.
In any case, I'm on it.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 01/14/2014 03:55 AM, Borislav Petkov wrote:
> On Tue, Jan 14, 2014 at 09:41:33AM -0200, Henrique de Moraes Holschuh wrote:
>> I just got this assigned to amd64-microcode in Debian, but it is something
>> that needs to be worked around by the EFI/BIOS and/or the kernel.
>>
>> Since we all know just how well it pans out to depend on BIOS/EFI updates
>> for *anything*, I'm raising the issue here. IMHO looks like it would be
>> worthwhile to either set the relevant MSR in the kernel if the BIOS didn't
>> do it, or at least warn the user of the need for a BIOS/EFI update...
>>
>> It is the usual hangs-core type of CPU errata (therefore, the best type
>> since it won't cause silent data corruption). gcc-produced code managed to
>> trigger it (in DragonFly BSD).
>
> I think this is a different issue than the dragonfly issue. In any case,
> if AMD delivers all BIOS with this workaround enabled, we don't need to
> do anything. And I asked them about it so we'll have an answer soon, I
> hope.
>
> In any case, I'm on it.
>
Seriously, though, if this MSR can be set at runtime without problems
(which covers 98% of all workarounds, but not 100%) then it seems like a
no-brainer to just do it as long as the offending CPUs can be identified
by the kernel.
-hpa
On Tue, Jan 14, 2014 at 07:14:48AM -0800, H. Peter Anvin wrote:
> Seriously, though, if this MSR can be set at runtime without problems
> (which covers 98% of all workarounds, but not 100%) then it seems
> like a no-brainer to just do it as long as the offending CPUs can be
> identified by the kernel.
Yeah, ok, let me cook it up.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
From: Borislav Petkov <[email protected]>
This adds the workaround for erratum 793 as a precaution in case not
every BIOS implements it. This addresses CVE-2013-6885.
Signed-off-by: Borislav Petkov <[email protected]>
---
arch/x86/kernel/cpu/amd.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 4a48e8bbd857..e4d6f8c91f51 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -507,6 +507,17 @@ static void early_init_amd(struct cpuinfo_x86 *c)
set_cpu_cap(c, X86_FEATURE_EXTD_APICID);
}
#endif
+
+#define MSR_AMD_LS_CFG 0xc0011020
+ /* F16h, models 00h-0fh, erratum 793 */
+ if (c->x86 == 0x16 && c->x86_model <= 0xf) {
+ u64 val;
+
+ rdmsrl(MSR_AMD_LS_CFG, val);
+ if (!(val & BIT(15)))
+ wrmsrl(MSR_AMD_LS_CFG, val | BIT(15));
+ }
+
}
static const int amd_erratum_383[];
--
1.8.5.2.192.g7794a68
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 01/14/2014 08:27 AM, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
>
> This adds the workaround for erratum 793 as a precaution in case not
> every BIOS implements it. This addresses CVE-2013-6885.
>
> Signed-off-by: Borislav Petkov <[email protected]>
> +
> +#define MSR_AMD_LS_CFG 0xc0011020
> + /* F16h, models 00h-0fh, erratum 793 */
> + if (c->x86 == 0x16 && c->x86_model <= 0xf) {
Please put MSR_AMD_LS_CFG in msr-index.h or at least at the top of the
file, and put the CVE reference in the comment.
Otherwise, looks good.
-hpa
On Tue, Jan 14, 2014 at 08:30:31AM -0800, H. Peter Anvin wrote:
> Please put MSR_AMD_LS_CFG in msr-index.h or at least at the top of the
> file,
Yeah, can we agree on some kind of a rule for msr-index.h? Something
like the rule for pci.h, for example?
My only wish is to have all the MSRs at one place so one can search them
fast. This has been the only thing that I wanted to have in the past,
from fiddling with MSRs for so long.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 01/14/2014 08:42 AM, Borislav Petkov wrote:
> On Tue, Jan 14, 2014 at 08:30:31AM -0800, H. Peter Anvin wrote:
>> Please put MSR_AMD_LS_CFG in msr-index.h or at least at the top of the
>> file,
>
> Yeah, can we agree on some kind of a rule for msr-index.h? Something
> like the rule for pci.h, for example?
>
> My only wish is to have all the MSRs at one place so one can search them
> fast. This has been the only thing that I wanted to have in the past,
> from fiddling with MSRs for so long.
>
That is what msr-index.h is supposed to be.
-hpa
On Tue, Jan 14, 2014 at 02:15:08PM -0600, Aravind Gopalakrishnan wrote:
> Scrubbing the RevGuide for Fam16, I found couple more Errata that need
> workarounds:
Before you go and do that, just ask internally whether those workarounds
are being delivered with AGESA instead. We don't want to have a fix in
the kernel for *every* erratum if it can be helped.
We're making an exception for this one because it can freeze the core
just by executing a locked insn which can be done even in userspace.
> @Boris: Could you please create a branch at bp.git with your patch so
> I can base my work on top of it?
For future AMD patches, make sure to almost always use tip master, i.e.
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git#master
to base your work off of.
> Also, I tested this patch against linux.git on a fam16 based machine
> and it works fine.
Thanks, much appreciated. I'll add your Tested-by.
> (I wasn't sure if you had access to a fam16 based box)
Yeah, I don't. So I'm relying on you for future patches :-)
Thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
From: Borislav Petkov <[email protected]>
This adds the workaround for erratum 793 as a precaution in case not
every BIOS implements it. This addresses CVE-2013-6885.
Signed-off-by: Borislav Petkov <[email protected]>
Tested-by: Aravind Gopalakrishnan <[email protected]>
---
arch/x86/include/uapi/asm/msr-index.h | 1 +
arch/x86/kernel/cpu/amd.c | 10 ++++++++++
2 files changed, 11 insertions(+)
diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
index 37813b5ddc37..59cea185ad1d 100644
--- a/arch/x86/include/uapi/asm/msr-index.h
+++ b/arch/x86/include/uapi/asm/msr-index.h
@@ -184,6 +184,7 @@
#define MSR_AMD64_PATCH_LOADER 0xc0010020
#define MSR_AMD64_OSVW_ID_LENGTH 0xc0010140
#define MSR_AMD64_OSVW_STATUS 0xc0010141
+#define MSR_AMD64_LS_CFG 0xc0011020
#define MSR_AMD64_DC_CFG 0xc0011022
#define MSR_AMD64_BU_CFG2 0xc001102a
#define MSR_AMD64_IBSFETCHCTL 0xc0011030
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 4a48e8bbd857..a4bec9cd293a 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -507,6 +507,16 @@ static void early_init_amd(struct cpuinfo_x86 *c)
set_cpu_cap(c, X86_FEATURE_EXTD_APICID);
}
#endif
+
+ /* F16h erratum 793, CVE-2013-6885 */
+ if (c->x86 == 0x16 && c->x86_model <= 0xf) {
+ u64 val;
+
+ rdmsrl(MSR_AMD64_LS_CFG, val);
+ if (!(val & BIT(15)))
+ wrmsrl(MSR_AMD_LS_CFG, val | BIT(15));
+ }
+
}
static const int amd_erratum_383[];
--
1.8.5.2.192.g7794a68
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 01/14/2014 03:07 PM, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
>
> This adds the workaround for erratum 793 as a precaution in case not
> every BIOS implements it. This addresses CVE-2013-6885.
>
> Signed-off-by: Borislav Petkov <[email protected]>
> Tested-by: Aravind Gopalakrishnan <[email protected]>
> ---
> arch/x86/include/uapi/asm/msr-index.h | 1 +
> arch/x86/kernel/cpu/amd.c | 10 ++++++++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
> index 37813b5ddc37..59cea185ad1d 100644
> --- a/arch/x86/include/uapi/asm/msr-index.h
> +++ b/arch/x86/include/uapi/asm/msr-index.h
> @@ -184,6 +184,7 @@
> #define MSR_AMD64_PATCH_LOADER 0xc0010020
> #define MSR_AMD64_OSVW_ID_LENGTH 0xc0010140
> #define MSR_AMD64_OSVW_STATUS 0xc0010141
> +#define MSR_AMD64_LS_CFG 0xc0011020
> #define MSR_AMD64_DC_CFG 0xc0011022
> #define MSR_AMD64_BU_CFG2 0xc001102a
> #define MSR_AMD64_IBSFETCHCTL 0xc0011030
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 4a48e8bbd857..a4bec9cd293a 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -507,6 +507,16 @@ static void early_init_amd(struct cpuinfo_x86 *c)
> set_cpu_cap(c, X86_FEATURE_EXTD_APICID);
> }
> #endif
> +
> + /* F16h erratum 793, CVE-2013-6885 */
> + if (c->x86 == 0x16 && c->x86_model <= 0xf) {
> + u64 val;
> +
> + rdmsrl(MSR_AMD64_LS_CFG, val);
> + if (!(val & BIT(15)))
> + wrmsrl(MSR_AMD_LS_CFG, val | BIT(15));
> + }
> +
> }
>
> static const int amd_erratum_383[];
>
In file included from
/home/hpa/kernel/distwork/arch/x86/include/asm/processor.h:20:0,
from
/home/hpa/kernel/distwork/arch/x86/include/asm/thread_info.h:22,
from
/home/hpa/kernel/distwork/include/linux/thread_info.h:54,
from
/home/hpa/kernel/distwork/arch/x86/include/asm/elf.h:7,
from /home/hpa/kernel/distwork/include/linux/elf.h:4,
from /home/hpa/kernel/distwork/arch/x86/kernel/cpu/amd.c:4:
/home/hpa/kernel/distwork/arch/x86/kernel/cpu/amd.c: In function
‘early_init_amd’:
/home/hpa/kernel/distwork/arch/x86/kernel/cpu/amd.c:518:11: error:
‘MSR_AMD_LS_CFG’ undeclared (first use in this function)
wrmsrl(MSR_AMD_LS_CFG, val | BIT(15));
^
/home/hpa/kernel/distwork/arch/x86/include/asm/msr.h:156:20: note: in
definition of macro ‘wrmsrl’
native_write_msr((msr), (u32)((u64)(val)), (u32)((u64)(val) >> 32))
^
/home/hpa/kernel/distwork/arch/x86/kernel/cpu/amd.c:518:11: note: each
undeclared identifier is reported only once for each function it appear
s in
wrmsrl(MSR_AMD_LS_CFG, val | BIT(15));
^
/home/hpa/kernel/distwork/arch/x86/include/asm/msr.h:156:20: note: in
definition of macro ‘wrmsrl’
native_write_msr((msr), (u32)((u64)(val)), (u32)((u64)(val) >> 32))
-hpa
Commit-ID: 3b56496865f9f7d9bcb2f93b44c63f274f08e3b6
Gitweb: http://git.kernel.org/tip/3b56496865f9f7d9bcb2f93b44c63f274f08e3b6
Author: Borislav Petkov <[email protected]>
AuthorDate: Wed, 15 Jan 2014 00:07:11 +0100
Committer: H. Peter Anvin <[email protected]>
CommitDate: Tue, 14 Jan 2014 16:39:07 -0800
x86, cpu, amd: Add workaround for family 16h, erratum 793
This adds the workaround for erratum 793 as a precaution in case not
every BIOS implements it. This addresses CVE-2013-6885.
Erratum text:
[Revision Guide for AMD Family 16h Models 00h-0Fh Processors,
document 51810 Rev. 3.04 November 2013]
793 Specific Combination of Writes to Write Combined Memory Types and
Locked Instructions May Cause Core Hang
Description
Under a highly specific and detailed set of internal timing
conditions, a locked instruction may trigger a timing sequence whereby
the write to a write combined memory type is not flushed, causing the
locked instruction to stall indefinitely.
Potential Effect on System
Processor core hang.
Suggested Workaround
BIOS should set MSR
C001_1020[15] = 1b.
Fix Planned
No fix planned
[ hpa: updated description, fixed typo in MSR name ]
Signed-off-by: Borislav Petkov <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Tested-by: Aravind Gopalakrishnan <[email protected]>
Signed-off-by: H. Peter Anvin <[email protected]>
---
arch/x86/include/uapi/asm/msr-index.h | 1 +
arch/x86/kernel/cpu/amd.c | 10 ++++++++++
2 files changed, 11 insertions(+)
diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
index 37813b5..59cea18 100644
--- a/arch/x86/include/uapi/asm/msr-index.h
+++ b/arch/x86/include/uapi/asm/msr-index.h
@@ -184,6 +184,7 @@
#define MSR_AMD64_PATCH_LOADER 0xc0010020
#define MSR_AMD64_OSVW_ID_LENGTH 0xc0010140
#define MSR_AMD64_OSVW_STATUS 0xc0010141
+#define MSR_AMD64_LS_CFG 0xc0011020
#define MSR_AMD64_DC_CFG 0xc0011022
#define MSR_AMD64_BU_CFG2 0xc001102a
#define MSR_AMD64_IBSFETCHCTL 0xc0011030
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index bca023b..59bfebc 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -508,6 +508,16 @@ static void early_init_amd(struct cpuinfo_x86 *c)
set_cpu_cap(c, X86_FEATURE_EXTD_APICID);
}
#endif
+
+ /* F16h erratum 793, CVE-2013-6885 */
+ if (c->x86 == 0x16 && c->x86_model <= 0xf) {
+ u64 val;
+
+ rdmsrl(MSR_AMD64_LS_CFG, val);
+ if (!(val & BIT(15)))
+ wrmsrl(MSR_AMD64_LS_CFG, val | BIT(15));
+ }
+
}
static const int amd_erratum_383[];
On 01/14/2014 04:45 PM, tip-bot for Borislav Petkov wrote:
> + rdmsrl(MSR_AMD64_LS_CFG, val);
> + if (!(val & BIT(15)))
> + wrmsrl(MSR_AMD64_LS_CFG, val | BIT(15));
Incidentally, I'm wondering if we shouldn't have a
set_in_msr()/clear_in_msr() set of functions which would incorporate the
above construct:
void set_in_msr(u32 msr, u64 mask)
{
u64 old, new;
old = rdmsrl(msr);
new = old | mask;
if (old != new)
wrmsrl(msr, new);
}
... and the obvious equivalent for clear_in_msr().
The perhaps only question is if it should be "set/clear_bit_in_msr()"
rather than having to haul a full 64-bit mask in the common case.
-hpa
* H. Peter Anvin <[email protected]> wrote:
> On 01/14/2014 04:45 PM, tip-bot for Borislav Petkov wrote:
> > + rdmsrl(MSR_AMD64_LS_CFG, val);
> > + if (!(val & BIT(15)))
> > + wrmsrl(MSR_AMD64_LS_CFG, val | BIT(15));
>
> Incidentally, I'm wondering if we shouldn't have a
> set_in_msr()/clear_in_msr() set of functions which would incorporate the
> above construct:
>
> void set_in_msr(u32 msr, u64 mask)
> {
> u64 old, new;
>
> old = rdmsrl(msr);
> new = old | mask;
> if (old != new)
> wrmsrl(msr, new);
> }
>
> ... and the obvious equivalent for clear_in_msr().
>
> The perhaps only question is if it should be "set/clear_bit_in_msr()"
> rather than having to haul a full 64-bit mask in the common case.
I'd suggest the introduction of a standard set of methods operating on
MSRs:
msr_read()
msr_write()
msr_set_bit()
msr_clear_bit()
msr_set_mask()
msr_clear_mask()
etc.
msr_read() would essentially map to rdmsr_safe(). Each method has a
return value that can be checked for failure.
Note that the naming of 'msr_set_bit()' and 'msr_clear_bit()' mirrors
that of bitops, and set_mask/clear_mask is named along a similar
pattern, so that it's more immediately obvious what's going on.
With such methods in place we could use them in most new code, and
would use 'raw, unsafe' rdmsr()/wrmsr() only in very specific,
justified cases.
Thanks,
Ingo
On Tue, Jan 14, 2014 at 04:38:27PM -0800, H. Peter Anvin wrote:
> In file included from
> /home/hpa/kernel/distwork/arch/x86/include/asm/processor.h:20:0,
Finnish!
Well, Boris the dumbass forgot to amend, changes were still in the index
but not committed. This is what I get when I do a couple of things at
the same time. :(
---
From: Borislav Petkov <[email protected]>
This adds the workaround for erratum 793 as a precaution in case not
every BIOS implements it. This addresses CVE-2013-6885.
Signed-off-by: Borislav Petkov <[email protected]>
Tested-by: Aravind Gopalakrishnan <[email protected]>
---
arch/x86/include/uapi/asm/msr-index.h | 1 +
arch/x86/kernel/cpu/amd.c | 10 ++++++++++
2 files changed, 11 insertions(+)
diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
index 37813b5ddc37..59cea185ad1d 100644
--- a/arch/x86/include/uapi/asm/msr-index.h
+++ b/arch/x86/include/uapi/asm/msr-index.h
@@ -184,6 +184,7 @@
#define MSR_AMD64_PATCH_LOADER 0xc0010020
#define MSR_AMD64_OSVW_ID_LENGTH 0xc0010140
#define MSR_AMD64_OSVW_STATUS 0xc0010141
+#define MSR_AMD64_LS_CFG 0xc0011020
#define MSR_AMD64_DC_CFG 0xc0011022
#define MSR_AMD64_BU_CFG2 0xc001102a
#define MSR_AMD64_IBSFETCHCTL 0xc0011030
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 4a48e8bbd857..6ac3a67ad471 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -507,6 +507,16 @@ static void early_init_amd(struct cpuinfo_x86 *c)
set_cpu_cap(c, X86_FEATURE_EXTD_APICID);
}
#endif
+
+ /* F16h erratum 793, CVE-2013-6885 */
+ if (c->x86 == 0x16 && c->x86_model <= 0xf) {
+ u64 val;
+
+ rdmsrl(MSR_AMD64_LS_CFG, val);
+ if (!(val & BIT(15)))
+ wrmsrl(MSR_AMD64_LS_CFG, val | BIT(15));
+ }
+
}
static const int amd_erratum_383[];
--
1.8.5.2.192.g7794a68
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Wed, Jan 15, 2014 at 07:28:17AM +0100, Ingo Molnar wrote:
> > The perhaps only question is if it should be "set/clear_bit_in_msr()"
> > rather than having to haul a full 64-bit mask in the common case.
I'd prefer the _bit() variant because it is easy to use in all those
set-chicken-bit cases.
> I'd suggest the introduction of a standard set of methods operating on
> MSRs:
>
> msr_read()
> msr_write()
> msr_set_bit()
> msr_clear_bit()
> msr_set_mask()
> msr_clear_mask()
>
> etc.
>
> msr_read() would essentially map to rdmsr_safe(). Each method has a
> return value that can be checked for failure.
I'm not sure we want to use the _safe() variants by default as it would
generate the exception tables even in cases where they're clearly not
needed.
> Note that the naming of 'msr_set_bit()' and 'msr_clear_bit()' mirrors
> that of bitops, and set_mask/clear_mask is named along a similar
> pattern, so that it's more immediately obvious what's going on.
Yes, I completely agree - this is something I will do after the merge
window.
The question about the need for the _mask() variants will be best
answered after going over the sources and checking whether there
actually is a need for setting more than one bit in an MSR.
Thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 01/15/2014 05:36 AM, Borislav Petkov wrote:
>>
>> msr_read() would essentially map to rdmsr_safe(). Each method has a
>> return value that can be checked for failure.
>
> I'm not sure we want to use the _safe() variants by default as it would
> generate the exception tables even in cases where they're clearly not
> needed.
>
It would be particularly silly if what you end up with is in effect to
wrap msr_read/write() in a BUG_ON(), which is the effect of the current
(trapping) form. There is something to be said for hard errors.
-hpa
* H. Peter Anvin <[email protected]> wrote:
> On 01/15/2014 05:36 AM, Borislav Petkov wrote:
> >>
> >> msr_read() would essentially map to rdmsr_safe(). Each method has a
> >> return value that can be checked for failure.
> >
> > I'm not sure we want to use the _safe() variants by default as it
> > would generate the exception tables even in cases where they're
> > clearly not needed.
I don't think those new methods should be inline functions - thus
there will be only one exception entry for each.
> It would be particularly silly if what you end up with is in effect
> to wrap msr_read/write() in a BUG_ON(), which is the effect of the
> current (trapping) form. There is something to be said for hard
> errors.
Right, the fact that most of our MSR accesses today are
crash-on-failure, which happens to trigger crashes on a regular
schedule, where most of the crashes are 'harmless' situation except
that they crash the systems for good.
So I think defaulting to soft failures is the right approach.
Thanks,
Ingo
On 01/15/2014 10:38 AM, Ingo Molnar wrote:
>
> Right, the fact that most of our MSR accesses today are
> crash-on-failure, which happens to trigger crashes on a regular
> schedule, where most of the crashes are 'harmless' situation except
> that they crash the systems for good.
>
> So I think defaulting to soft failures is the right approach.
>
Well, as I said, as long as people don't start doing:
BUG_ON(msr_read(...))
... 'cuz that's just silly. Also, on 64 bits the code generated by
going from 2x32 bits to 1x64 and sometimes back can add up (on 32 bits
there is no difference.)
-hpa
On 1/14/2014 2:38 PM, Borislav Petkov wrote:
> Before you go and do that, just ask internally whether those
> workarounds are being delivered with AGESA instead. We don't want to
> have a fix in the kernel for *every* erratum if it can be helped.
> We're making an exception for this one because it can freeze the core
> just by executing a locked insn which can be done even in userspace.
Ok, So looks like workaround for E776 went in early, so it's probably
safe to assume all BIOSes will have the fix.
The workaround for E792 was checked in this month and *will* be picked
up by BIOS later..
We might want to still have a software fix for this just in case someone
uses older BIOSes..
> For future AMD patches, make sure to almost always use tip master,
> i.e. git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git#master
> to base your work off of.
I shall spin a patch on top of tip and send it out soon
Thanks,
-Aravind.
On Thu, Jan 16, 2014 at 11:58:13AM -0600, Aravind Gopalakrishnan wrote:
> We might want to still have a software fix for this just in case
> someone uses older BIOSes..
Are you saying there are F16h BIOSes out there which might not have the
fix?
Also, for PCI config space fixes, take a look at
arch/x86/kernel/quirks.c
Also, code in arch/x86/kernel/amd_nb.c could come in handy, especially
if you have multiple F16h sockets configurations.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Thu, 16 Jan 2014, Aravind Gopalakrishnan wrote:
> We might want to still have a software fix for this just in case
> someone uses older BIOSes..
There is no "just in case" when it comes to someone using outdated firmware.
It is a *given*, except for hardware that is only used in HPC and multiple
(> 2) socket servers/workstations (which might be the case here, I don't
know what processors we're talking about).
The vast majority of PC users will never update their firmware, unless the
update is automatically offered to them, and sometimes not even in that
case.
--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh
On 01/16/2014 04:21 PM, Henrique de Moraes Holschuh wrote:
> On Thu, 16 Jan 2014, Aravind Gopalakrishnan wrote:
>> We might want to still have a software fix for this just in case
>> someone uses older BIOSes..
>
> There is no "just in case" when it comes to someone using outdated firmware.
> It is a *given*, except for hardware that is only used in HPC and multiple
> (> 2) socket servers/workstations (which might be the case here, I don't
> know what processors we're talking about).
>
> The vast majority of PC users will never update their firmware, unless the
> update is automatically offered to them, and sometimes not even in that
> case.
>
Yes, the real question is: did the broken BIOS ship in production
systems? We don't care about engineering samples/beta boxes, but once
it is sold as a product, it is real.
-hpa
On Thu, Jan 16, 2014 at 10:21:24PM -0200, Henrique de Moraes Holschuh wrote:
> On Thu, 16 Jan 2014, Aravind Gopalakrishnan wrote:
> > We might want to still have a software fix for this just in case
> > someone uses older BIOSes..
>
> There is no "just in case" when it comes to someone using outdated firmware.
> It is a *given*, except for hardware that is only used in HPC and multiple
> (> 2) socket servers/workstations (which might be the case here, I don't
> know what processors we're talking about).
>
> The vast majority of PC users will never update their firmware, unless the
> update is automatically offered to them, and sometimes not even in that
> case.
We also cannot carry *every* erratum workaround in the kernel just
because people don't update firmware. Firmware is becoming ubiquitous,
sadly, and because of that, admins should provision for firmware
upgrades too.
Besides, *even* if we put *all* errata fixes in the kernel, you'd need
to update it anyway and reboot. In this case, you can just as well
update your firmware instead, which involves that same reboot.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 01/17/2014 02:18 AM, Borislav Petkov wrote:
>
> We also cannot carry *every* erratum workaround in the kernel just
> because people don't update firmware. Firmware is becoming ubiquitous,
> sadly, and because of that, admins should provision for firmware
> upgrades too.
>
> Besides, *even* if we put *all* errata fixes in the kernel, you'd need
> to update it anyway and reboot. In this case, you can just as well
> update your firmware instead, which involves that same reboot.
>
Actually I by and large disagree with that. There is a limit, of
course, but when it comes to flipping an MSR in init code, the bar is
pretty darn low. We have quirks for all kind of hardware, and this is
just another example.
What *is* important, though, is that the workaround is well commented so
that when someone comes and wonders "WTF is this, and what constraints
does it have on it" they can get back to the primary sources (errata
documents, mailing list discussions, CVEs, etc.) without undue effort.
The effort of a kernel update is much lower, especially since the kernel
is generally automatically updated. It would be awesome if that was
done for firmware, but in the absence of central distribution, arbitrary
EOL sunsets, and a standard OS-driven firmware installer, it just isn't
going to happen widely. Yes, that is a problem.
-hpa
On Fri, Jan 17, 2014 at 08:23:24AM -0800, H. Peter Anvin wrote:
> Actually I by and large disagree with that. There is a limit, of
> course, but when it comes to flipping an MSR in init code, the bar is
> pretty darn low. We have quirks for all kind of hardware, and this is
> just another example.
No, you don't. :-)
You would much prefer to have the workaround done in the BIOS and only
if there's a coverage hole, only then to do it in the kernel.
I'm not saying we shouldn't do it in the kernel per se - I'm saying we
should do it only when really necessary. And it doesn't hurt to talk
about it first before inviting in all fixes for all errata for all
families of all vendors. No, you don't want that, believe me. :-)
> The effort of a kernel update is much lower, especially since the
> kernel is generally automatically updated.
Does that even matter? I think what matters is whether we reboot or not,
i.e. HA crap. If we have to reboot, we might just as well flash the BIOS
- it takes almost as long.
> It would be awesome if that was done for firmware, but in the absence
> of central distribution, arbitrary EOL sunsets, and a standard
> OS-driven firmware installer,
Oh, the brave new world of UEFI wants to address that - it is supposed
to flash the firmware from the OS. OEM vendors have their home grown
solutions already, as I'm sure you know.
> it just isn't going to happen widely Yes, that is a problem.
That definitely is a problem.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 1/17/2014 11:02 AM, Borislav Petkov wrote:
> On Fri, Jan 17, 2014 at 08:23:24AM -0800, H. Peter Anvin wrote:
>> Actually I by and large disagree with that. There is a limit, of
>> course, but when it comes to flipping an MSR in init code, the bar is
>> pretty darn low. We have quirks for all kind of hardware, and this is
>> just another example.
> No, you don't. :-)
>
> You would much prefer to have the workaround done in the BIOS and only
> if there's a coverage hole, only then to do it in the kernel.
>
> I'm not saying we shouldn't do it in the kernel per se - I'm saying we
> should do it only when really necessary. And it doesn't hurt to talk
> about it first before inviting in all fixes for all errata for all
> families of all vendors. No, you don't want that, believe me. :-)
Right, so the chip goes into multiple platforms (server,desktop..) and
this Erratum affects all platforms the chip fits in..
Also, the problem is that we can't be certain how many systems in the
field carry BIOSes with the fix for this. Therefore,
it is better to insulate ourselves than trust BIOS to do the right thing..
>
>> The effort of a kernel update is much lower, especially since the
>> kernel is generally automatically updated.
> Does that even matter? I think what matters is whether we reboot or not,
> i.e. HA crap. If we have to reboot, we might just as well flash the BIOS
> - it takes almost as long.
True, but isn't the case that it's more *likely* for sys admins to
resort to updating kernels than program new BIOS?
The alternative would be to update microcode, but there is no microcode
patch file for this family yet on linux-firmware..
I've got a patch worked up for this anyway; Sending it as separate mail..
Thanks,
Aravind.
On 01/17/2014 09:02 AM, Borislav Petkov wrote:
>
> No, you don't. :-)
>
> You would much prefer to have the workaround done in the BIOS and only
> if there's a coverage hole, only then to do it in the kernel.
>
Hence the question: did an unfixed BIOS ever make it into a shipping
production system? Because if it did, we do have a coverage hole.
-hpa
On 1/17/2014 11:42 AM, H. Peter Anvin wrote:
>
> Hence the question: did an unfixed BIOS ever make it into a shipping
> production system? Because if it did, we do have a coverage hole.
>
> -hpa
>
Right, and the answer to your question is: there is a *chance* it did..
From internal discussions, I could
gather that the fix went into BIOS code earlier for desktop/mobile
versions than it did for server versions;
So there is a chance production systems (desktop ones) might never see
the issue, but server ones will, as
BIOSes spun only from now onwards will have the workaround..
-Aravind.
On Fri, Jan 17, 2014 at 12:05:37PM -0600, Aravind Gopalakrishnan wrote:
> On 1/17/2014 11:42 AM, H. Peter Anvin wrote:
> >
> >Hence the question: did an unfixed BIOS ever make it into a shipping
> >production system? Because if it did, we do have a coverage hole.
> >
> Right, and the answer to your question is: there is a *chance* it
> did.. From internal discussions, I could gather that the fix went into
> BIOS code earlier for desktop/mobile versions than it did for server
> versions;
>
> So there is a chance production systems (desktop ones) might never see
> the issue, but server ones will, as BIOSes spun only from now onwards
> will have the workaround..
This is exactly what I mean: I would like to have this question answered
before any erratum fix goes into the kernel.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Tue 2014-01-14 17:27:23, Borislav Petkov wrote:
> From: Borislav Petkov <[email protected]>
>
> This adds the workaround for erratum 793 as a precaution in case not
> every BIOS implements it. This addresses CVE-2013-6885.
>
> Signed-off-by: Borislav Petkov <[email protected]>
> ---
> arch/x86/kernel/cpu/amd.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 4a48e8bbd857..e4d6f8c91f51 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -507,6 +507,17 @@ static void early_init_amd(struct cpuinfo_x86 *c)
> set_cpu_cap(c, X86_FEATURE_EXTD_APICID);
> }
> #endif
> +
> +#define MSR_AMD_LS_CFG 0xc0011020
> + /* F16h, models 00h-0fh, erratum 793 */
> + if (c->x86 == 0x16 && c->x86_model <= 0xf) {
> + u64 val;
> +
> + rdmsrl(MSR_AMD_LS_CFG, val);
> + if (!(val & BIT(15)))
> + wrmsrl(MSR_AMD_LS_CFG, val | BIT(15));
Would it make sense to printk() a warning?
BIOS is clearly buggy in this case, and it may cause problems with
another operating system, earlier kernel, or maybe early in boot
before MSR is written...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Fri, Jan 17, 2014 at 11:28:06PM +0100, Pavel Machek wrote:
> Would it make sense to printk() a warning?
No because people come and start bitching about their dmesg containing
a warning and whether their hardware is b0rked without even reading the
actual words.
> BIOS is clearly buggy in this case, and it may cause problems with
> another operating system, earlier kernel, or maybe early in boot
> before MSR is written...
Well, if the problem happens, the machine will lock. If it doesn't lock
=> no problem and we apply the workaround => no problem at all. :-)
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On 01/17/2014 02:50 PM, Borislav Petkov wrote:
> On Fri, Jan 17, 2014 at 11:28:06PM +0100, Pavel Machek wrote:
>> Would it make sense to printk() a warning?
>
> No because people come and start bitching about their dmesg containing
> a warning and whether their hardware is b0rked without even reading the
> actual words.
>
Printing a warning is appropriate if we can't actually fix the problem
in the OS. If we actually make the problem go away then we have just
done our job and we can be done with it.
-hpa
On Fri, Jan 17, 2014 at 02:51:52PM -0800, H. Peter Anvin wrote:
> Printing a warning is appropriate if we can't actually fix the problem
> in the OS. If we actually make the problem go away then we have just
> done our job and we can be done with it.
And we do make it go away so no warning and a gold star for us!
:-)
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Fri 2014-01-17 14:51:52, H. Peter Anvin wrote:
> On 01/17/2014 02:50 PM, Borislav Petkov wrote:
> > On Fri, Jan 17, 2014 at 11:28:06PM +0100, Pavel Machek wrote:
> >> Would it make sense to printk() a warning?
> >
> > No because people come and start bitching about their dmesg containing
> > a warning and whether their hardware is b0rked without even reading the
> > actual words.
Have you checked your dmesg recently? Normal people don't read
it... it is just too much of it.
> Printing a warning is appropriate if we can't actually fix the problem
> in the OS. If we actually make the problem go away then we have just
> done our job and we can be done with it.
I disagree. Older kernel versions still may have problem, etc.
We normally do print warnings for problems we work around. We want
vendors to fix their hardware, too...
ACPI BIOS Warning (bug): 32/64X FACS address mismatch in FADT -
0xBDB5FF40/0x00000000BDB64F40, using 32 (20131115/tbfadt-522)
[Firmware Bug]: ACPI: BIOS _OSI(Linux) query ignored
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 01/17/2014 04:29 PM, Pavel Machek wrote:
>
> Have you checked your dmesg recently? Normal people don't read
> it... it is just too much of it.
>
>> Printing a warning is appropriate if we can't actually fix the problem
>> in the OS. If we actually make the problem go away then we have just
>> done our job and we can be done with it.
>
> I disagree. Older kernel versions still may have problem, etc.
>
> We normally do print warnings for problems we work around. We want
> vendors to fix their hardware, too...
>
> ACPI BIOS Warning (bug): 32/64X FACS address mismatch in FADT -
> 0xBDB5FF40/0x00000000BDB64F40, using 32 (20131115/tbfadt-522)
> [Firmware Bug]: ACPI: BIOS _OSI(Linux) query ignored
>
You say people don't read their dmesg...
... because there is too much ...
... so let's add more?
What am I missing here?
-hpa
On Fri 2014-01-17 17:21:00, H. Peter Anvin wrote:
> On 01/17/2014 04:29 PM, Pavel Machek wrote:
> >
> > Have you checked your dmesg recently? Normal people don't read
> > it... it is just too much of it.
> >
> >> Printing a warning is appropriate if we can't actually fix the problem
> >> in the OS. If we actually make the problem go away then we have just
> >> done our job and we can be done with it.
> >
> > I disagree. Older kernel versions still may have problem, etc.
> >
> > We normally do print warnings for problems we work around. We want
> > vendors to fix their hardware, too...
> >
> > ACPI BIOS Warning (bug): 32/64X FACS address mismatch in FADT -
> > 0xBDB5FF40/0x00000000BDB64F40, using 32 (20131115/tbfadt-522)
> > [Firmware Bug]: ACPI: BIOS _OSI(Linux) query ignored
> >
>
> You say people don't read their dmesg...
> ... because there is too much ...
> ... so let's add more?
I'd say that proposed "your bios has a bug" is way more useful than
stuff such as:
pci_bus 0000:00: root bus resource [bus 00-ff]
pci_bus 0000:00: root bus resource [io 0x0000-0x0cf7]
pci_bus 0000:00: root bus resource [io 0x0d00-0xffff]
pci_bus 0000:00: root bus resource [mem 0x000a0000-0x000bffff]
pci_bus 0000:00: root bus resource [mem 0xc0000000-0xffffffff]
pci 0000:00:00.0: [8086:2e30] type 00 class 0x060000
pci 0000:00:01.0: [8086:2e31] type 01 class 0x060400
pci 0000:00:01.0: PME# supported from D0 D3hot D3cold
pci 0000:00:01.0: System wakeup disabled by ACPI
pci 0000:00:02.0: [8086:2e32] type 00 class 0x030000
pci 0000:00:02.0: reg 0x10: [mem 0xd0000000-0xd03fffff 64bit]
pci 0000:00:02.0: reg 0x18: [mem 0xc0000000-0xcfffffff 64bit pref]
pci 0000:00:02.0: reg 0x20: [io 0xf140-0xf147]
pci 0000:00:02.1: [8086:2e33] type 00 class 0x038000
pci 0000:00:02.1: reg 0x10: [mem 0xd0400000-0xd04fffff 64bit]
pci 0000:00:1b.0: [8086:27d8] type 00 class 0x040300
pci 0000:00:1b.0: reg 0x10: [mem 0xd0600000-0xd0603fff 64bit]
pci 0000:00:1b.0: PME# supported from D0 D3hot D3cold
pci 0000:00:1c.0: [8086:27d0] type 01 class 0x060400
pci 0000:00:1c.0: PME# supported from D0 D3hot D3cold
pci 0000:00:1c.0: System wakeup disabled by ACPI
pci 0000:00:1c.1: [8086:27d2] type 01 class 0x060400
pci 0000:00:1c.1: PME# supported from D0 D3hot D3cold
pci 0000:00:1c.1: System wakeup disabled by ACPI
pci 0000:00:1d.0: [8086:27c8] type 00 class 0x0c0300
pci 0000:00:1d.0: reg 0x20: [io 0xf080-0xf09f]
pci 0000:00:1d.0: System wakeup disabled by ACPI
So yes. Lets add useful stuff.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sat, Jan 18, 2014 at 03:01:55AM +0100, Pavel Machek wrote:
> I'd say that proposed "your bios has a bug"
You can always unconditionally print "your bios has a bug". Just like
that. Without even looking.
So no, I don't think adding a warning for this one case makes sense.
If you want to add a warning yourself, you can put it in rc.local using
userspace goodies like msr-tools. With them you can toggle that bit ad
absurdum - no need for the kernel except msr.ko.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Sat 2014-01-18 11:42:31, Borislav Petkov wrote:
> On Sat, Jan 18, 2014 at 03:01:55AM +0100, Pavel Machek wrote:
> > I'd say that proposed "your bios has a bug"
>
> You can always unconditionally print "your bios has a bug". Just like
> that. Without even looking.
Yeah, and we have linux-firmware testing toolkits, to discourage buggy
bioses. In this case it is "your bios has rare and dangerous bug, see CVS-1234".
> If you want to add a warning yourself, you can put it in rc.local using
> userspace goodies like msr-tools. With them you can toggle that bit ad
> absurdum - no need for the kernel except msr.ko.
?? By the time userspace is booted, you have overwritten the MSR, and
information, if the BIOS is buggy, was lost.
So linux-firmware testing toolkit actually has no chance to report
this one.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Sat, Jan 18, 2014 at 12:08:29PM +0100, Pavel Machek wrote:
> ?? By the time userspace is booted, you have overwritten the MSR, and
> information, if the BIOS is buggy, was lost.
Bullshit - if you don't apply the workaround, the bit in the MSR remains
unset.
Drop this senseless conversation and go play with something else. At
least I am.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Sat 2014-01-18 12:26:20, Borislav Petkov wrote:
> On Sat, Jan 18, 2014 at 12:08:29PM +0100, Pavel Machek wrote:
> > ?? By the time userspace is booted, you have overwritten the MSR, and
> > information, if the BIOS is buggy, was lost.
>
> Bullshit - if you don't apply the workaround, the bit in the MSR remains
> unset.
If you do not apply the workaround, you have the buggy system that may
or may not boot. There's even CVE for it.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html