[PATCH] Fixing an uninitialized variable that can lead to #GP.

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fixing an uninitialized variable that can lead to #GP.

Anthony Steinhauser
In the current implementation of the TAA mitigation if the cpuid_level
is 6 and it's an Intel CPU, the sefflags_edx variable is used without
being initialized. If the SEFF0EDX_ARCH_CAP bit is accidentally flipped
in it, the rdmsr on the unimplemented MSR_ARCH_CAPABILITIES index leads
to a #GP fault.

This change initializes the sefflags_edx variable to 0 which is
consistent with the MSR_ARCH_CAPABILITIES being unavailable.
---
 sys/arch/amd64/amd64/cpu.c | 2 +-
 sys/arch/i386/i386/cpu.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sys/arch/amd64/amd64/cpu.c b/sys/arch/amd64/amd64/cpu.c
index 48ab6b5e7f3..f9beff0d5e3 100644
--- a/sys/arch/amd64/amd64/cpu.c
+++ b/sys/arch/amd64/amd64/cpu.c
@@ -1164,7 +1164,7 @@ void
 cpu_tsx_disable(struct cpu_info *ci)
 {
  uint64_t msr;
- uint32_t dummy, sefflags_edx;
+ uint32_t dummy, sefflags_edx = 0;
 
  /* this runs before identifycpu() populates ci_feature_sefflags_edx */
  if (cpuid_level >= 0x07)
diff --git a/sys/arch/i386/i386/cpu.c b/sys/arch/i386/i386/cpu.c
index b31a431c594..76f1b65bede 100644
--- a/sys/arch/i386/i386/cpu.c
+++ b/sys/arch/i386/i386/cpu.c
@@ -473,7 +473,7 @@ void
 cpu_tsx_disable(struct cpu_info *ci)
 {
  uint64_t msr;
- uint32_t dummy, sefflags_edx;
+ uint32_t dummy, sefflags_edx = 0;
 
  /* this runs before identifycpu() populates ci_feature_sefflags_edx */
  if (cpuid_level >= 0x07)
--
2.25.0.341.g760bfbb309-goog

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixing an uninitialized variable that can lead to #GP.

Mike Larkin-2
On Sun, Feb 09, 2020 at 06:17:47PM -0800, Anthony Steinhauser wrote:

> In the current implementation of the TAA mitigation if the cpuid_level
> is 6 and it's an Intel CPU, the sefflags_edx variable is used without
> being initialized. If the SEFF0EDX_ARCH_CAP bit is accidentally flipped
> in it, the rdmsr on the unimplemented MSR_ARCH_CAPABILITIES index leads
> to a #GP fault.
>
> This change initializes the sefflags_edx variable to 0 which is
> consistent with the MSR_ARCH_CAPABILITIES being unavailable.
> ---
>  sys/arch/amd64/amd64/cpu.c | 2 +-
>  sys/arch/i386/i386/cpu.c   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sys/arch/amd64/amd64/cpu.c b/sys/arch/amd64/amd64/cpu.c
> index 48ab6b5e7f3..f9beff0d5e3 100644
> --- a/sys/arch/amd64/amd64/cpu.c
> +++ b/sys/arch/amd64/amd64/cpu.c
> @@ -1164,7 +1164,7 @@ void
>  cpu_tsx_disable(struct cpu_info *ci)
>  {
>   uint64_t msr;
> - uint32_t dummy, sefflags_edx;
> + uint32_t dummy, sefflags_edx = 0;
>  
>   /* this runs before identifycpu() populates ci_feature_sefflags_edx */
>   if (cpuid_level >= 0x07)
> diff --git a/sys/arch/i386/i386/cpu.c b/sys/arch/i386/i386/cpu.c
> index b31a431c594..76f1b65bede 100644
> --- a/sys/arch/i386/i386/cpu.c
> +++ b/sys/arch/i386/i386/cpu.c
> @@ -473,7 +473,7 @@ void
>  cpu_tsx_disable(struct cpu_info *ci)
>  {
>   uint64_t msr;
> - uint32_t dummy, sefflags_edx;
> + uint32_t dummy, sefflags_edx = 0;
>  
>   /* this runs before identifycpu() populates ci_feature_sefflags_edx */
>   if (cpuid_level >= 0x07)
> --
> 2.25.0.341.g760bfbb309-goog
>

Probably safer to use rdmsr_safe for this sort of thing also.

-ml

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fixing an uninitialized variable that can lead to #GP.

Jonathan Gray-11
In reply to this post by Anthony Steinhauser
On Sun, Feb 09, 2020 at 06:17:47PM -0800, Anthony Steinhauser wrote:
> In the current implementation of the TAA mitigation if the cpuid_level
> is 6 and it's an Intel CPU, the sefflags_edx variable is used without
> being initialized. If the SEFF0EDX_ARCH_CAP bit is accidentally flipped
> in it, the rdmsr on the unimplemented MSR_ARCH_CAPABILITIES index leads
> to a #GP fault.
>
> This change initializes the sefflags_edx variable to 0 which is
> consistent with the MSR_ARCH_CAPABILITIES being unavailable.

Thanks for the report.  Committed a different fix:

Index: i386/i386/cpu.c
===================================================================
RCS file: /cvs/src/sys/arch/i386/i386/cpu.c,v
retrieving revision 1.98
diff -u -p -r1.98 cpu.c
--- i386/i386/cpu.c 20 Dec 2019 07:55:30 -0000 1.98
+++ i386/i386/cpu.c 10 Feb 2020 03:04:02 -0000
@@ -476,8 +476,10 @@ cpu_tsx_disable(struct cpu_info *ci)
  uint32_t dummy, sefflags_edx;
 
  /* this runs before identifycpu() populates ci_feature_sefflags_edx */
- if (cpuid_level >= 0x07)
- CPUID_LEAF(0x7, 0, dummy, dummy, dummy, sefflags_edx);
+ if (cpuid_level < 0x07)
+ return;
+ CPUID_LEAF(0x7, 0, dummy, dummy, dummy, sefflags_edx);
+
  if (strcmp(cpu_vendor, "GenuineIntel") == 0 &&
     (sefflags_edx & SEFF0EDX_ARCH_CAP)) {
  msr = rdmsr(MSR_ARCH_CAPABILITIES);
Index: amd64/amd64/cpu.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/cpu.c,v
retrieving revision 1.143
diff -u -p -r1.143 cpu.c
--- amd64/amd64/cpu.c 20 Dec 2019 07:49:31 -0000 1.143
+++ amd64/amd64/cpu.c 10 Feb 2020 03:03:51 -0000
@@ -1167,8 +1167,10 @@ cpu_tsx_disable(struct cpu_info *ci)
  uint32_t dummy, sefflags_edx;
 
  /* this runs before identifycpu() populates ci_feature_sefflags_edx */
- if (cpuid_level >= 0x07)
- CPUID_LEAF(0x7, 0, dummy, dummy, dummy, sefflags_edx);
+ if (cpuid_level < 0x07)
+ return;
+ CPUID_LEAF(0x7, 0, dummy, dummy, dummy, sefflags_edx);
+
  if (strcmp(cpu_vendor, "GenuineIntel") == 0 &&
     (sefflags_edx & SEFF0EDX_ARCH_CAP)) {
  msr = rdmsr(MSR_ARCH_CAPABILITIES);