All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
@ 2019-08-14 21:17 Lendacky, Thomas
  2019-08-14 23:24 ` Non-random RDRAND " Pavel Machek
                   ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: Lendacky, Thomas @ 2019-08-14 21:17 UTC (permalink / raw)
  To: linux-kernel, linux-doc, linux-pm, x86
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Rafael J . Wysocki, Pavel Machek, Chen Yu, Jonathan Corbet

From: Tom Lendacky <thomas.lendacky@amd.com>

There have been reports of RDRAND issues after resuming from suspend on
some AMD family 15h and family 16h systems. This issue stems from BIOS
not performing the proper steps during resume to ensure RDRAND continues
to function properly.

RDRAND support is indicated by CPUID Fn00000001_ECX[30]. This bit can be
reset by clearing MSR C001_1004[62]. Any software that checks for RDRAND
support using CPUID, including the kernel,  will believe that RDRAND is
not supported.

Update the CPU initialization to clear the RDRAND CPUID bit for any family
15h and 16h processor that supports RDRAND. If it is known that the family
15h or family 16h system does not have an RDRAND resume issue or that the
system will not be placed in suspend, the "rdrand_force" kernel parameter
can be used to stop the clearing of the RDRAND CPUID bit.

Additionally, update the suspend and resume path to save and restore the
MSR C001_1004 value to ensure that the RDRAND CPUID setting remains in
place after resuming from suspend.

Note, that clearing the RDRAND CPUID bit does not prevent a processor
that normally supports the RDRAND instruction from executing the RDRAND
instruction. So any code that determined the support based on family and
model won't #UD.

Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 .../admin-guide/kernel-parameters.txt         |  8 ++
 arch/x86/include/asm/msr-index.h              |  1 +
 arch/x86/kernel/cpu/amd.c                     | 42 ++++++++++
 arch/x86/power/cpu.c                          | 83 ++++++++++++++++---
 4 files changed, 121 insertions(+), 13 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 47d981a86e2f..f47eb33958c1 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4090,6 +4090,14 @@
 			Run specified binary instead of /init from the ramdisk,
 			used for early userspace startup. See initrd.
 
+	rdrand_force	[X86]
+			On certain AMD processors, the advertisement of the
+			RDRAND instruction has been disabled by the kernel
+			because of buggy BIOS support, specifically around the
+			suspend/resume path. This option allows for overriding
+			that decision if it is known that the BIOS support for
+			RDRAND is not buggy on the system.
+
 	rdt=		[HW,X86,RDT]
 			Turn on/off individual RDT features. List is:
 			cmt, mbmtotal, mbmlocal, l3cat, l3cdp, l2cat, l2cdp,
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 6b4fc2788078..29ae2b66b9e9 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -381,6 +381,7 @@
 #define MSR_AMD64_PATCH_LEVEL		0x0000008b
 #define MSR_AMD64_TSC_RATIO		0xc0000104
 #define MSR_AMD64_NB_CFG		0xc001001f
+#define MSR_AMD64_CPUID_FN_00000001	0xc0011004
 #define MSR_AMD64_PATCH_LOADER		0xc0010020
 #define MSR_AMD64_OSVW_ID_LENGTH	0xc0010140
 #define MSR_AMD64_OSVW_STATUS		0xc0010141
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 3afe07d602dd..86ff1464302b 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -804,6 +804,40 @@ static void init_amd_ln(struct cpuinfo_x86 *c)
 	msr_set_bit(MSR_AMD64_DE_CFG, 31);
 }
 
+static bool rdrand_force;
+
+static int __init rdrand_force_cmdline(char *str)
+{
+	rdrand_force = true;
+
+	return 0;
+}
+early_param("rdrand_force", rdrand_force_cmdline);
+
+static void init_hide_rdrand(struct cpuinfo_x86 *c)
+{
+	/*
+	 * The nordrand option can clear X86_FEATURE_RDRAND, so check for
+	 * RDRAND support using the CPUID function directly.
+	 */
+	if (!(cpuid_ecx(1) & BIT(30)) || rdrand_force)
+		return;
+
+	msr_clear_bit(MSR_AMD64_CPUID_FN_00000001, 62);
+	clear_cpu_cap(c, X86_FEATURE_RDRAND);
+	pr_info_once("hiding RDRAND via CPUID\n");
+}
+
+static void init_amd_jg(struct cpuinfo_x86 *c)
+{
+	/*
+	 * Some BIOS implementations do not restore proper RDRAND support
+	 * across suspend and resume. Check on whether to hide the RDRAND
+	 * instruction support via CPUID.
+	 */
+	init_hide_rdrand(c);
+}
+
 static void init_amd_bd(struct cpuinfo_x86 *c)
 {
 	u64 value;
@@ -818,6 +852,13 @@ static void init_amd_bd(struct cpuinfo_x86 *c)
 			wrmsrl_safe(MSR_F15H_IC_CFG, value);
 		}
 	}
+
+	/*
+	 * Some BIOS implementations do not restore proper RDRAND support
+	 * across suspend and resume. Check on whether to hide the RDRAND
+	 * instruction support via CPUID.
+	 */
+	init_hide_rdrand(c);
 }
 
 static void init_amd_zn(struct cpuinfo_x86 *c)
@@ -860,6 +901,7 @@ static void init_amd(struct cpuinfo_x86 *c)
 	case 0x10: init_amd_gh(c); break;
 	case 0x12: init_amd_ln(c); break;
 	case 0x15: init_amd_bd(c); break;
+	case 0x16: init_amd_jg(c); break;
 	case 0x17: init_amd_zn(c); break;
 	}
 
diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
index 1c58d8982728..146c4fd90c3d 100644
--- a/arch/x86/power/cpu.c
+++ b/arch/x86/power/cpu.c
@@ -12,6 +12,7 @@
 #include <linux/smp.h>
 #include <linux/perf_event.h>
 #include <linux/tboot.h>
+#include <linux/dmi.h>
 
 #include <asm/pgtable.h>
 #include <asm/proto.h>
@@ -23,7 +24,7 @@
 #include <asm/debugreg.h>
 #include <asm/cpu.h>
 #include <asm/mmu_context.h>
-#include <linux/dmi.h>
+#include <asm/cpu_device_id.h>
 
 #ifdef CONFIG_X86_32
 __visible unsigned long saved_context_ebx;
@@ -393,15 +394,14 @@ static int __init bsp_pm_check_init(void)
 
 core_initcall(bsp_pm_check_init);
 
-static int msr_init_context(const u32 *msr_id, const int total_num)
+static int msr_build_context(const u32 *msr_id, const int num)
 {
-	int i = 0;
+	struct saved_msrs *saved_msrs = &saved_context.saved_msrs;
 	struct saved_msr *msr_array;
+	int total_num;
+	int i, j;
 
-	if (saved_context.saved_msrs.array || saved_context.saved_msrs.num > 0) {
-		pr_err("x86/pm: MSR quirk already applied, please check your DMI match table.\n");
-		return -EINVAL;
-	}
+	total_num = saved_msrs->num + num;
 
 	msr_array = kmalloc_array(total_num, sizeof(struct saved_msr), GFP_KERNEL);
 	if (!msr_array) {
@@ -409,19 +409,27 @@ static int msr_init_context(const u32 *msr_id, const int total_num)
 		return -ENOMEM;
 	}
 
-	for (i = 0; i < total_num; i++) {
-		msr_array[i].info.msr_no	= msr_id[i];
+	if (saved_msrs->array) {
+		/* Copy previous MSR save requests */
+		memcpy(msr_array, saved_msrs->array,
+		       sizeof(struct saved_msr) * saved_msrs->num);
+
+		kfree(saved_msrs->array);
+	}
+
+	for (i = saved_msrs->num, j = 0; i < total_num; i++, j++) {
+		msr_array[i].info.msr_no	= msr_id[j];
 		msr_array[i].valid		= false;
 		msr_array[i].info.reg.q		= 0;
 	}
-	saved_context.saved_msrs.num	= total_num;
-	saved_context.saved_msrs.array	= msr_array;
+	saved_msrs->num   = total_num;
+	saved_msrs->array = msr_array;
 
 	return 0;
 }
 
 /*
- * The following section is a quirk framework for problematic BIOSen:
+ * The following sections are a quirk framework for problematic BIOSen:
  * Sometimes MSRs are modified by the BIOSen after suspended to
  * RAM, this might cause unexpected behavior after wakeup.
  * Thus we save/restore these specified MSRs across suspend/resume
@@ -436,7 +444,7 @@ static int msr_initialize_bdw(const struct dmi_system_id *d)
 	u32 bdw_msr_id[] = { MSR_IA32_THERM_CONTROL };
 
 	pr_info("x86/pm: %s detected, MSR saving is needed during suspending.\n", d->ident);
-	return msr_init_context(bdw_msr_id, ARRAY_SIZE(bdw_msr_id));
+	return msr_build_context(bdw_msr_id, ARRAY_SIZE(bdw_msr_id));
 }
 
 static const struct dmi_system_id msr_save_dmi_table[] = {
@@ -451,9 +459,58 @@ static const struct dmi_system_id msr_save_dmi_table[] = {
 	{}
 };
 
+static int msr_save_cpuid_features(const struct x86_cpu_id *c)
+{
+	u32 cpuid_msr_id[] = {
+		MSR_AMD64_CPUID_FN_00000001,
+	};
+
+	pr_info("x86/pm: family %#hx cpu detected, MSR saving is needed during suspending.\n",
+		c->family);
+
+	return msr_build_context(cpuid_msr_id, ARRAY_SIZE(cpuid_msr_id));
+}
+
+static const struct x86_cpu_id msr_save_cpu_table[] = {
+	{
+		.vendor = X86_VENDOR_AMD,
+		.family = 0x15,
+		.model = X86_MODEL_ANY,
+		.feature = X86_FEATURE_ANY,
+		.driver_data = (kernel_ulong_t)msr_save_cpuid_features,
+	},
+	{
+		.vendor = X86_VENDOR_AMD,
+		.family = 0x16,
+		.model = X86_MODEL_ANY,
+		.feature = X86_FEATURE_ANY,
+		.driver_data = (kernel_ulong_t)msr_save_cpuid_features,
+	},
+	{}
+};
+
+typedef int (*pm_cpu_match_t)(const struct x86_cpu_id *);
+static int pm_cpu_check(const struct x86_cpu_id *c)
+{
+	const struct x86_cpu_id *m;
+	int ret = 0;
+
+	m = x86_match_cpu(msr_save_cpu_table);
+	if (m) {
+		pm_cpu_match_t fn;
+
+		fn = (pm_cpu_match_t)m->driver_data;
+		ret = fn(m);
+	}
+
+	return ret;
+}
+
 static int pm_check_save_msr(void)
 {
 	dmi_check_system(msr_save_dmi_table);
+	pm_cpu_check(msr_save_cpu_table);
+
 	return 0;
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Non-random RDRAND Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
  2019-08-14 21:17 [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h Lendacky, Thomas
@ 2019-08-14 23:24 ` Pavel Machek
  2019-08-14 23:38   ` Pavel Machek
                     ` (2 more replies)
  2019-08-15  7:21 ` Borislav Petkov
                   ` (2 subsequent siblings)
  3 siblings, 3 replies; 19+ messages in thread
From: Pavel Machek @ 2019-08-14 23:24 UTC (permalink / raw)
  To: Lendacky, Thomas, tytso, nhorman
  Cc: linux-kernel, linux-doc, linux-pm, x86, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Rafael J . Wysocki, Chen Yu,
	Jonathan Corbet

[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]

On Wed 2019-08-14 21:17:41, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> There have been reports of RDRAND issues after resuming from suspend on
> some AMD family 15h and family 16h systems. This issue stems from BIOS
> not performing the proper steps during resume to ensure RDRAND continues
> to function properly.

Burn it with fire!

I mean... people were afraid RDRAND would be backdoored, and you now
confirm ... it indeed _is_ backdoored? /., here's news for you!

So what is the impact? Does it give random-looking but predictable
numbers after resume? Does it give all zeros? Something else?

>  
> +	rdrand_force	[X86]
> +			On certain AMD processors, the advertisement of the
> +			RDRAND instruction has been disabled by the kernel
> +			because of buggy BIOS support, specifically around the
> +			suspend/resume path. This option allows for overriding
> +			that decision if it is known that the BIOS support for
> +			RDRAND is not buggy on the system.

But this is not how we normally deal with buggy BIOSes. We don't want
user to have to decide this...

Should we introduce black-list or white-list of BIOS versions?

Hmm. Actually.

You are the CPU vendor. Surely you can tell us how to init RDRAND in
kernel if BIOS failed to do that... can you?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Non-random RDRAND Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
  2019-08-14 23:24 ` Non-random RDRAND " Pavel Machek
@ 2019-08-14 23:38   ` Pavel Machek
  2019-08-15 13:01   ` Lendacky, Thomas
  2019-08-15 15:12   ` Theodore Y. Ts'o
  2 siblings, 0 replies; 19+ messages in thread
From: Pavel Machek @ 2019-08-14 23:38 UTC (permalink / raw)
  To: Lendacky, Thomas, tytso, nhorman, security
  Cc: linux-kernel, linux-doc, linux-pm, x86, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Rafael J . Wysocki, Chen Yu,
	Jonathan Corbet

[-- Attachment #1: Type: text/plain, Size: 1425 bytes --]

On Thu 2019-08-15 01:24:35, Pavel Machek wrote:
> On Wed 2019-08-14 21:17:41, Lendacky, Thomas wrote:
> > From: Tom Lendacky <thomas.lendacky@amd.com>
> > 
> > There have been reports of RDRAND issues after resuming from suspend on
> > some AMD family 15h and family 16h systems. This issue stems from BIOS
> > not performing the proper steps during resume to ensure RDRAND continues
> > to function properly.
> 
> Burn it with fire!
> 
> I mean... people were afraid RDRAND would be backdoored, and you now
> confirm ... it indeed _is_ backdoored? /., here's news for you!
> 
> So what is the impact? Does it give random-looking but predictable
> numbers after resume? Does it give all zeros? Something else?

Plus... We trust the RDRAND in some configurations:

        random.trust_cpu={on,off}
				[KNL] Enable or disable trusting the
				use of the CPU's random
				number generator (if available) to
	       		     fully seed the
				kernel's CRNG. Default is controlled by
				CONFIG_RANDOM_TRUST_CPU.

so.. does this mean /dev/random was giving non-random values for some
users?

Certainly it means userland users were getting non-random values. That
sounds like something worth CVE and informing affected users?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
  2019-08-14 21:17 [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h Lendacky, Thomas
  2019-08-14 23:24 ` Non-random RDRAND " Pavel Machek
@ 2019-08-15  7:21 ` Borislav Petkov
  2019-08-15 13:47   ` Lendacky, Thomas
  2019-08-15 20:59 ` Andrew Cooper
  2019-08-16 15:19 ` Andy Lutomirski
  3 siblings, 1 reply; 19+ messages in thread
From: Borislav Petkov @ 2019-08-15  7:21 UTC (permalink / raw)
  To: Lendacky, Thomas
  Cc: linux-kernel, linux-doc, linux-pm, x86, Thomas Gleixner,
	Ingo Molnar, Rafael J . Wysocki, Pavel Machek, Chen Yu,
	Jonathan Corbet

On Wed, Aug 14, 2019 at 09:17:41PM +0000, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> There have been reports of RDRAND issues after resuming from suspend on
> some AMD family 15h and family 16h systems. This issue stems from BIOS
> not performing the proper steps during resume to ensure RDRAND continues
> to function properly.

If this happens only during suspend/resume, this probably should
be done only on configurations which have CONFIG_SUSPEND and/or
CONFIG_HIBERNATION enabled. I'm assuming BIOS does init it properly
at least during boot - I mean, they should've passed some sort of a
certification.

OTOH, if the breakage happens on resume, they clearly didn't test the
BIOS suspend/resume. I mean, I'm not at all surprised - it is f*cking
BIOS. News at 11.

> RDRAND support is indicated by CPUID Fn00000001_ECX[30]. This bit can be
> reset by clearing MSR C001_1004[62]. Any software that checks for RDRAND
> support using CPUID, including the kernel,  will believe that RDRAND is
> not supported.
> 
> Update the CPU initialization to clear the RDRAND CPUID bit for any family
> 15h and 16h processor that supports RDRAND. If it is known that the family
> 15h or family 16h system does not have an RDRAND resume issue or that the
> system will not be placed in suspend, the "rdrand_force" kernel parameter
> can be used to stop the clearing of the RDRAND CPUID bit.
> 
> Additionally, update the suspend and resume path to save and restore the
> MSR C001_1004 value to ensure that the RDRAND CPUID setting remains in
> place after resuming from suspend.
> 
> Note, that clearing the RDRAND CPUID bit does not prevent a processor
> that normally supports the RDRAND instruction from executing the RDRAND
> instruction. So any code that determined the support based on family and
> model won't #UD.
> 
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  .../admin-guide/kernel-parameters.txt         |  8 ++
>  arch/x86/include/asm/msr-index.h              |  1 +
>  arch/x86/kernel/cpu/amd.c                     | 42 ++++++++++
>  arch/x86/power/cpu.c                          | 83 ++++++++++++++++---
>  4 files changed, 121 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 47d981a86e2f..f47eb33958c1 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4090,6 +4090,14 @@
>  			Run specified binary instead of /init from the ramdisk,
>  			used for early userspace startup. See initrd.
>  
> +	rdrand_force	[X86]
> +			On certain AMD processors, the advertisement of the
> +			RDRAND instruction has been disabled by the kernel
> +			because of buggy BIOS support, specifically around the
> +			suspend/resume path. This option allows for overriding
> +			that decision if it is known that the BIOS support for
> +			RDRAND is not buggy on the system.
> +
>  	rdt=		[HW,X86,RDT]
>  			Turn on/off individual RDT features. List is:
>  			cmt, mbmtotal, mbmlocal, l3cat, l3cdp, l2cat, l2cdp,
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 6b4fc2788078..29ae2b66b9e9 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -381,6 +381,7 @@
>  #define MSR_AMD64_PATCH_LEVEL		0x0000008b
>  #define MSR_AMD64_TSC_RATIO		0xc0000104
>  #define MSR_AMD64_NB_CFG		0xc001001f
> +#define MSR_AMD64_CPUID_FN_00000001	0xc0011004

I know the PPR has all the 0s but let's write it

MSR_AMD64_CPUID_FN_1

so that it is readable in the kernel.

>  #define MSR_AMD64_PATCH_LOADER		0xc0010020
>  #define MSR_AMD64_OSVW_ID_LENGTH	0xc0010140
>  #define MSR_AMD64_OSVW_STATUS		0xc0010141
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 3afe07d602dd..86ff1464302b 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -804,6 +804,40 @@ static void init_amd_ln(struct cpuinfo_x86 *c)
>  	msr_set_bit(MSR_AMD64_DE_CFG, 31);
>  }
>  
> +static bool rdrand_force;
> +
> +static int __init rdrand_force_cmdline(char *str)
> +{
> +	rdrand_force = true;
> +
> +	return 0;
> +}
> +early_param("rdrand_force", rdrand_force_cmdline);

Let's make this a more generic param:

	rdrand=force[, ...]

in case we wanna add some more opts here later.

> +
> +static void init_hide_rdrand(struct cpuinfo_x86 *c)

clear_rdrand_cpuid_bit()

is what this function does.

> +{
> +	/*
> +	 * The nordrand option can clear X86_FEATURE_RDRAND, so check for
> +	 * RDRAND support using the CPUID function directly.
> +	 */
> +	if (!(cpuid_ecx(1) & BIT(30)) || rdrand_force)
> +		return;
> +
> +	msr_clear_bit(MSR_AMD64_CPUID_FN_00000001, 62);
> +	clear_cpu_cap(c, X86_FEATURE_RDRAND);
> +	pr_info_once("hiding RDRAND via CPUID\n");

No need for that I guess - that's visible in /proc/cpuinfo.

> +}
> +
> +static void init_amd_jg(struct cpuinfo_x86 *c)
> +{
> +	/*
> +	 * Some BIOS implementations do not restore proper RDRAND support
> +	 * across suspend and resume. Check on whether to hide the RDRAND
> +	 * instruction support via CPUID.
> +	 */
> +	init_hide_rdrand(c);
> +}
> +
>  static void init_amd_bd(struct cpuinfo_x86 *c)
>  {
>  	u64 value;
> @@ -818,6 +852,13 @@ static void init_amd_bd(struct cpuinfo_x86 *c)
>  			wrmsrl_safe(MSR_F15H_IC_CFG, value);
>  		}
>  	}
> +
> +	/*
> +	 * Some BIOS implementations do not restore proper RDRAND support
> +	 * across suspend and resume. Check on whether to hide the RDRAND
> +	 * instruction support via CPUID.
> +	 */
> +	init_hide_rdrand(c);
>  }
>  
>  static void init_amd_zn(struct cpuinfo_x86 *c)
> @@ -860,6 +901,7 @@ static void init_amd(struct cpuinfo_x86 *c)
>  	case 0x10: init_amd_gh(c); break;
>  	case 0x12: init_amd_ln(c); break;
>  	case 0x15: init_amd_bd(c); break;
> +	case 0x16: init_amd_jg(c); break;
>  	case 0x17: init_amd_zn(c); break;
>  	}
> diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
> index 1c58d8982728..146c4fd90c3d 100644
> --- a/arch/x86/power/cpu.c
> +++ b/arch/x86/power/cpu.c
> @@ -12,6 +12,7 @@
>  #include <linux/smp.h>
>  #include <linux/perf_event.h>
>  #include <linux/tboot.h>
> +#include <linux/dmi.h>
>  
>  #include <asm/pgtable.h>
>  #include <asm/proto.h>
> @@ -23,7 +24,7 @@
>  #include <asm/debugreg.h>
>  #include <asm/cpu.h>
>  #include <asm/mmu_context.h>
> -#include <linux/dmi.h>
> +#include <asm/cpu_device_id.h>
>  
>  #ifdef CONFIG_X86_32
>  __visible unsigned long saved_context_ebx;
> @@ -393,15 +394,14 @@ static int __init bsp_pm_check_init(void)
>  
>  core_initcall(bsp_pm_check_init);
>  
> -static int msr_init_context(const u32 *msr_id, const int total_num)
> +static int msr_build_context(const u32 *msr_id, const int num)
>  {
> -	int i = 0;
> +	struct saved_msrs *saved_msrs = &saved_context.saved_msrs;
>  	struct saved_msr *msr_array;
> +	int total_num;
> +	int i, j;
>  
> -	if (saved_context.saved_msrs.array || saved_context.saved_msrs.num > 0) {
> -		pr_err("x86/pm: MSR quirk already applied, please check your DMI match table.\n");
> -		return -EINVAL;
> -	}
> +	total_num = saved_msrs->num + num;
>  
>  	msr_array = kmalloc_array(total_num, sizeof(struct saved_msr), GFP_KERNEL);
>  	if (!msr_array) {
> @@ -409,19 +409,27 @@ static int msr_init_context(const u32 *msr_id, const int total_num)
>  		return -ENOMEM;
>  	}
>  
> -	for (i = 0; i < total_num; i++) {
> -		msr_array[i].info.msr_no	= msr_id[i];
> +	if (saved_msrs->array) {
> +		/* Copy previous MSR save requests */
> +		memcpy(msr_array, saved_msrs->array,
> +		       sizeof(struct saved_msr) * saved_msrs->num);

Why do you need to copy those? Why can't you use the infrastructure like
msr_initialize_bdw() does?

> +		kfree(saved_msrs->array);
> +	}
> +
> +	for (i = saved_msrs->num, j = 0; i < total_num; i++, j++) {
> +		msr_array[i].info.msr_no	= msr_id[j];
>  		msr_array[i].valid		= false;
>  		msr_array[i].info.reg.q		= 0;
>  	}
> -	saved_context.saved_msrs.num	= total_num;
> -	saved_context.saved_msrs.array	= msr_array;
> +	saved_msrs->num   = total_num;
> +	saved_msrs->array = msr_array;
>  
>  	return 0;
>  }
>  
>  /*
> - * The following section is a quirk framework for problematic BIOSen:
> + * The following sections are a quirk framework for problematic BIOSen:
>   * Sometimes MSRs are modified by the BIOSen after suspended to
>   * RAM, this might cause unexpected behavior after wakeup.
>   * Thus we save/restore these specified MSRs across suspend/resume
> @@ -436,7 +444,7 @@ static int msr_initialize_bdw(const struct dmi_system_id *d)
>  	u32 bdw_msr_id[] = { MSR_IA32_THERM_CONTROL };
>  
>  	pr_info("x86/pm: %s detected, MSR saving is needed during suspending.\n", d->ident);
> -	return msr_init_context(bdw_msr_id, ARRAY_SIZE(bdw_msr_id));
> +	return msr_build_context(bdw_msr_id, ARRAY_SIZE(bdw_msr_id));
>  }
>  
>  static const struct dmi_system_id msr_save_dmi_table[] = {
> @@ -451,9 +459,58 @@ static const struct dmi_system_id msr_save_dmi_table[] = {
>  	{}
>  };
>  
> +static int msr_save_cpuid_features(const struct x86_cpu_id *c)
> +{
> +	u32 cpuid_msr_id[] = {
> +		MSR_AMD64_CPUID_FN_00000001,
> +	};
> +
> +	pr_info("x86/pm: family %#hx cpu detected, MSR saving is needed during suspending.\n",
> +		c->family);
> +
> +	return msr_build_context(cpuid_msr_id, ARRAY_SIZE(cpuid_msr_id));
> +}
> +
> +static const struct x86_cpu_id msr_save_cpu_table[] = {
> +	{
> +		.vendor = X86_VENDOR_AMD,
> +		.family = 0x15,
> +		.model = X86_MODEL_ANY,
> +		.feature = X86_FEATURE_ANY,
> +		.driver_data = (kernel_ulong_t)msr_save_cpuid_features,
> +	},
> +	{
> +		.vendor = X86_VENDOR_AMD,
> +		.family = 0x16,
> +		.model = X86_MODEL_ANY,
> +		.feature = X86_FEATURE_ANY,
> +		.driver_data = (kernel_ulong_t)msr_save_cpuid_features,
> +	},
> +	{}

I think you can make that table a single entry by setting

	.vendor  = X86_VENDOR_AMD,
	...
	.feature = X86_FEATURE_RDRAND,

and then checking family in msr_save_cpuid_features().

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Non-random RDRAND Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
  2019-08-14 23:24 ` Non-random RDRAND " Pavel Machek
  2019-08-14 23:38   ` Pavel Machek
@ 2019-08-15 13:01   ` Lendacky, Thomas
  2019-08-15 15:12   ` Theodore Y. Ts'o
  2 siblings, 0 replies; 19+ messages in thread
From: Lendacky, Thomas @ 2019-08-15 13:01 UTC (permalink / raw)
  To: Pavel Machek, tytso, nhorman
  Cc: linux-kernel, linux-doc, linux-pm, x86, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Rafael J . Wysocki, Chen Yu,
	Jonathan Corbet

On 8/14/19 6:24 PM, Pavel Machek wrote:
> On Wed 2019-08-14 21:17:41, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> There have been reports of RDRAND issues after resuming from suspend on
>> some AMD family 15h and family 16h systems. This issue stems from BIOS
>> not performing the proper steps during resume to ensure RDRAND continues
>> to function properly.
> 
> Burn it with fire!
> 
> I mean... people were afraid RDRAND would be backdoored, and you now
> confirm ... it indeed _is_ backdoored? /., here's news for you!
> 
> So what is the impact? Does it give random-looking but predictable
> numbers after resume? Does it give all zeros? Something else?

See this article:
https://www.phoronix.com/scan.php?page=news_item&px=AMD-CPUs-RdRand-Suspend

Thanks,
Tom

> 
>>  
>> +	rdrand_force	[X86]
>> +			On certain AMD processors, the advertisement of the
>> +			RDRAND instruction has been disabled by the kernel
>> +			because of buggy BIOS support, specifically around the
>> +			suspend/resume path. This option allows for overriding
>> +			that decision if it is known that the BIOS support for
>> +			RDRAND is not buggy on the system.
> 
> But this is not how we normally deal with buggy BIOSes. We don't want
> user to have to decide this...
> 
> Should we introduce black-list or white-list of BIOS versions?
> 
> Hmm. Actually.
> 
> You are the CPU vendor. Surely you can tell us how to init RDRAND in
> kernel if BIOS failed to do that... can you?
> 
> 									Pavel
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
  2019-08-15  7:21 ` Borislav Petkov
@ 2019-08-15 13:47   ` Lendacky, Thomas
  2019-08-15 15:34     ` Borislav Petkov
  0 siblings, 1 reply; 19+ messages in thread
From: Lendacky, Thomas @ 2019-08-15 13:47 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: linux-kernel, linux-doc, linux-pm, x86, Thomas Gleixner,
	Ingo Molnar, Rafael J . Wysocki, Pavel Machek, Chen Yu,
	Jonathan Corbet

On 8/15/19 2:21 AM, Borislav Petkov wrote:
> On Wed, Aug 14, 2019 at 09:17:41PM +0000, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> There have been reports of RDRAND issues after resuming from suspend on
>> some AMD family 15h and family 16h systems. This issue stems from BIOS
>> not performing the proper steps during resume to ensure RDRAND continues
>> to function properly.
> 
> If this happens only during suspend/resume, this probably should
> be done only on configurations which have CONFIG_SUSPEND and/or
> CONFIG_HIBERNATION enabled. I'm assuming BIOS does init it properly

Sure, that makes sense. I'll tie it to CONFIG_PM_SLEEP since that is what
arch/x86/power/cpu.c is dependent on.

> at least during boot - I mean, they should've passed some sort of a
> certification.
> 
> OTOH, if the breakage happens on resume, they clearly didn't test the
> BIOS suspend/resume. I mean, I'm not at all surprised - it is f*cking
> BIOS. News at 11.
> 
>> RDRAND support is indicated by CPUID Fn00000001_ECX[30]. This bit can be
>> reset by clearing MSR C001_1004[62]. Any software that checks for RDRAND
>> support using CPUID, including the kernel,  will believe that RDRAND is
>> not supported.
>>
>> Update the CPU initialization to clear the RDRAND CPUID bit for any family
>> 15h and 16h processor that supports RDRAND. If it is known that the family
>> 15h or family 16h system does not have an RDRAND resume issue or that the
>> system will not be placed in suspend, the "rdrand_force" kernel parameter
>> can be used to stop the clearing of the RDRAND CPUID bit.
>>
>> Additionally, update the suspend and resume path to save and restore the
>> MSR C001_1004 value to ensure that the RDRAND CPUID setting remains in
>> place after resuming from suspend.
>>
>> Note, that clearing the RDRAND CPUID bit does not prevent a processor
>> that normally supports the RDRAND instruction from executing the RDRAND
>> instruction. So any code that determined the support based on family and
>> model won't #UD.
>>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>  .../admin-guide/kernel-parameters.txt         |  8 ++
>>  arch/x86/include/asm/msr-index.h              |  1 +
>>  arch/x86/kernel/cpu/amd.c                     | 42 ++++++++++
>>  arch/x86/power/cpu.c                          | 83 ++++++++++++++++---
>>  4 files changed, 121 insertions(+), 13 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 47d981a86e2f..f47eb33958c1 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -4090,6 +4090,14 @@
>>  			Run specified binary instead of /init from the ramdisk,
>>  			used for early userspace startup. See initrd.
>>  
>> +	rdrand_force	[X86]
>> +			On certain AMD processors, the advertisement of the
>> +			RDRAND instruction has been disabled by the kernel
>> +			because of buggy BIOS support, specifically around the
>> +			suspend/resume path. This option allows for overriding
>> +			that decision if it is known that the BIOS support for
>> +			RDRAND is not buggy on the system.
>> +
>>  	rdt=		[HW,X86,RDT]
>>  			Turn on/off individual RDT features. List is:
>>  			cmt, mbmtotal, mbmlocal, l3cat, l3cdp, l2cat, l2cdp,
>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>> index 6b4fc2788078..29ae2b66b9e9 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -381,6 +381,7 @@
>>  #define MSR_AMD64_PATCH_LEVEL		0x0000008b
>>  #define MSR_AMD64_TSC_RATIO		0xc0000104
>>  #define MSR_AMD64_NB_CFG		0xc001001f
>> +#define MSR_AMD64_CPUID_FN_00000001	0xc0011004
> 
> I know the PPR has all the 0s but let's write it
> 
> MSR_AMD64_CPUID_FN_1
> 
> so that it is readable in the kernel.

Ok, will do.

> 
>>  #define MSR_AMD64_PATCH_LOADER		0xc0010020
>>  #define MSR_AMD64_OSVW_ID_LENGTH	0xc0010140
>>  #define MSR_AMD64_OSVW_STATUS		0xc0010141
>> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
>> index 3afe07d602dd..86ff1464302b 100644
>> --- a/arch/x86/kernel/cpu/amd.c
>> +++ b/arch/x86/kernel/cpu/amd.c
>> @@ -804,6 +804,40 @@ static void init_amd_ln(struct cpuinfo_x86 *c)
>>  	msr_set_bit(MSR_AMD64_DE_CFG, 31);
>>  }
>>  
>> +static bool rdrand_force;
>> +
>> +static int __init rdrand_force_cmdline(char *str)
>> +{
>> +	rdrand_force = true;
>> +
>> +	return 0;
>> +}
>> +early_param("rdrand_force", rdrand_force_cmdline);
> 
> Let's make this a more generic param:
> 
> 	rdrand=force[, ...]
> 
> in case we wanna add some more opts here later.

Sure, I can do that. Do we want to tie this into the nordrand option and
add rdrand=off or keep that separate?

> 
>> +
>> +static void init_hide_rdrand(struct cpuinfo_x86 *c)
> 
> clear_rdrand_cpuid_bit()
> 
> is what this function does.

Ok.

> 
>> +{
>> +	/*
>> +	 * The nordrand option can clear X86_FEATURE_RDRAND, so check for
>> +	 * RDRAND support using the CPUID function directly.
>> +	 */
>> +	if (!(cpuid_ecx(1) & BIT(30)) || rdrand_force)
>> +		return;
>> +
>> +	msr_clear_bit(MSR_AMD64_CPUID_FN_00000001, 62);
>> +	clear_cpu_cap(c, X86_FEATURE_RDRAND);
>> +	pr_info_once("hiding RDRAND via CPUID\n");
> 
> No need for that I guess - that's visible in /proc/cpuinfo.

I think this is a clearer indication that the action has taken place.

> 
>> +}
>> +
>> +static void init_amd_jg(struct cpuinfo_x86 *c)
>> +{
>> +	/*
>> +	 * Some BIOS implementations do not restore proper RDRAND support
>> +	 * across suspend and resume. Check on whether to hide the RDRAND
>> +	 * instruction support via CPUID.
>> +	 */
>> +	init_hide_rdrand(c);
>> +}
>> +
>>  static void init_amd_bd(struct cpuinfo_x86 *c)
>>  {
>>  	u64 value;
>> @@ -818,6 +852,13 @@ static void init_amd_bd(struct cpuinfo_x86 *c)
>>  			wrmsrl_safe(MSR_F15H_IC_CFG, value);
>>  		}
>>  	}
>> +
>> +	/*
>> +	 * Some BIOS implementations do not restore proper RDRAND support
>> +	 * across suspend and resume. Check on whether to hide the RDRAND
>> +	 * instruction support via CPUID.
>> +	 */
>> +	init_hide_rdrand(c);
>>  }
>>  
>>  static void init_amd_zn(struct cpuinfo_x86 *c)
>> @@ -860,6 +901,7 @@ static void init_amd(struct cpuinfo_x86 *c)
>>  	case 0x10: init_amd_gh(c); break;
>>  	case 0x12: init_amd_ln(c); break;
>>  	case 0x15: init_amd_bd(c); break;
>> +	case 0x16: init_amd_jg(c); break;
>>  	case 0x17: init_amd_zn(c); break;
>>  	}
>> diff --git a/arch/x86/power/cpu.c b/arch/x86/power/cpu.c
>> index 1c58d8982728..146c4fd90c3d 100644
>> --- a/arch/x86/power/cpu.c
>> +++ b/arch/x86/power/cpu.c
>> @@ -12,6 +12,7 @@
>>  #include <linux/smp.h>
>>  #include <linux/perf_event.h>
>>  #include <linux/tboot.h>
>> +#include <linux/dmi.h>
>>  
>>  #include <asm/pgtable.h>
>>  #include <asm/proto.h>
>> @@ -23,7 +24,7 @@
>>  #include <asm/debugreg.h>
>>  #include <asm/cpu.h>
>>  #include <asm/mmu_context.h>
>> -#include <linux/dmi.h>
>> +#include <asm/cpu_device_id.h>
>>  
>>  #ifdef CONFIG_X86_32
>>  __visible unsigned long saved_context_ebx;
>> @@ -393,15 +394,14 @@ static int __init bsp_pm_check_init(void)
>>  
>>  core_initcall(bsp_pm_check_init);
>>  
>> -static int msr_init_context(const u32 *msr_id, const int total_num)
>> +static int msr_build_context(const u32 *msr_id, const int num)
>>  {
>> -	int i = 0;
>> +	struct saved_msrs *saved_msrs = &saved_context.saved_msrs;
>>  	struct saved_msr *msr_array;
>> +	int total_num;
>> +	int i, j;
>>  
>> -	if (saved_context.saved_msrs.array || saved_context.saved_msrs.num > 0) {
>> -		pr_err("x86/pm: MSR quirk already applied, please check your DMI match table.\n");
>> -		return -EINVAL;
>> -	}
>> +	total_num = saved_msrs->num + num;
>>  
>>  	msr_array = kmalloc_array(total_num, sizeof(struct saved_msr), GFP_KERNEL);
>>  	if (!msr_array) {
>> @@ -409,19 +409,27 @@ static int msr_init_context(const u32 *msr_id, const int total_num)
>>  		return -ENOMEM;
>>  	}
>>  
>> -	for (i = 0; i < total_num; i++) {
>> -		msr_array[i].info.msr_no	= msr_id[i];
>> +	if (saved_msrs->array) {
>> +		/* Copy previous MSR save requests */
>> +		memcpy(msr_array, saved_msrs->array,
>> +		       sizeof(struct saved_msr) * saved_msrs->num);
> 
> Why do you need to copy those? Why can't you use the infrastructure like
> msr_initialize_bdw() does?

Not sure what you mean. We can't use the DMI stuff for this. So now, with
the x86 family checks, if anyone adds some DMI stuff or x86 family stuff
in the future that matches both the DMI and x86 family checks, this will
be called more than once and so you need to copy any previous settings and
add the new ones.

> 
>> +		kfree(saved_msrs->array);
>> +	}
>> +
>> +	for (i = saved_msrs->num, j = 0; i < total_num; i++, j++) {
>> +		msr_array[i].info.msr_no	= msr_id[j];
>>  		msr_array[i].valid		= false;
>>  		msr_array[i].info.reg.q		= 0;
>>  	}
>> -	saved_context.saved_msrs.num	= total_num;
>> -	saved_context.saved_msrs.array	= msr_array;
>> +	saved_msrs->num   = total_num;
>> +	saved_msrs->array = msr_array;
>>  
>>  	return 0;
>>  }
>>  
>>  /*
>> - * The following section is a quirk framework for problematic BIOSen:
>> + * The following sections are a quirk framework for problematic BIOSen:
>>   * Sometimes MSRs are modified by the BIOSen after suspended to
>>   * RAM, this might cause unexpected behavior after wakeup.
>>   * Thus we save/restore these specified MSRs across suspend/resume
>> @@ -436,7 +444,7 @@ static int msr_initialize_bdw(const struct dmi_system_id *d)
>>  	u32 bdw_msr_id[] = { MSR_IA32_THERM_CONTROL };
>>  
>>  	pr_info("x86/pm: %s detected, MSR saving is needed during suspending.\n", d->ident);
>> -	return msr_init_context(bdw_msr_id, ARRAY_SIZE(bdw_msr_id));
>> +	return msr_build_context(bdw_msr_id, ARRAY_SIZE(bdw_msr_id));
>>  }
>>  
>>  static const struct dmi_system_id msr_save_dmi_table[] = {
>> @@ -451,9 +459,58 @@ static const struct dmi_system_id msr_save_dmi_table[] = {
>>  	{}
>>  };
>>  
>> +static int msr_save_cpuid_features(const struct x86_cpu_id *c)
>> +{
>> +	u32 cpuid_msr_id[] = {
>> +		MSR_AMD64_CPUID_FN_00000001,
>> +	};
>> +
>> +	pr_info("x86/pm: family %#hx cpu detected, MSR saving is needed during suspending.\n",
>> +		c->family);
>> +
>> +	return msr_build_context(cpuid_msr_id, ARRAY_SIZE(cpuid_msr_id));
>> +}
>> +
>> +static const struct x86_cpu_id msr_save_cpu_table[] = {
>> +	{
>> +		.vendor = X86_VENDOR_AMD,
>> +		.family = 0x15,
>> +		.model = X86_MODEL_ANY,
>> +		.feature = X86_FEATURE_ANY,
>> +		.driver_data = (kernel_ulong_t)msr_save_cpuid_features,
>> +	},
>> +	{
>> +		.vendor = X86_VENDOR_AMD,
>> +		.family = 0x16,
>> +		.model = X86_MODEL_ANY,
>> +		.feature = X86_FEATURE_ANY,
>> +		.driver_data = (kernel_ulong_t)msr_save_cpuid_features,
>> +	},
>> +	{}
> 
> I think you can make that table a single entry by setting
> 
> 	.vendor  = X86_VENDOR_AMD,
> 	...
> 	.feature = X86_FEATURE_RDRAND,
> 
> and then checking family in msr_save_cpuid_features().

Except that X86_FEATURE_RDRAND isn't set anymore. I could create a new
software feature that is set when the CPUID bit is cleared if that's
preferred.

Thanks,
Tom

> 
> Thx.
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Non-random RDRAND Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
  2019-08-14 23:24 ` Non-random RDRAND " Pavel Machek
  2019-08-14 23:38   ` Pavel Machek
  2019-08-15 13:01   ` Lendacky, Thomas
@ 2019-08-15 15:12   ` Theodore Y. Ts'o
  2019-08-16  9:07     ` Pavel Machek
  2019-08-16 14:42     ` Neil Horman
  2 siblings, 2 replies; 19+ messages in thread
From: Theodore Y. Ts'o @ 2019-08-15 15:12 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Lendacky, Thomas, nhorman, linux-kernel, linux-doc, linux-pm,
	x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Rafael J . Wysocki, Chen Yu, Jonathan Corbet

On Thu, Aug 15, 2019 at 01:24:35AM +0200, Pavel Machek wrote:
> Burn it with fire!
> 
> I mean... people were afraid RDRAND would be backdoored, and you now
> confirm ... it indeed _is_ backdoored? /., here's news for you!

To be fair to AMD, I wouldn't call it a backdoor.  Hanlon's razor is
applicable here:

	"Never attribute to malice that which can be adequately
	explained by neglect."

(Sometimes other words are used instead of neglect, but i'm trying to
be nice.)

					- Ted

P.S.   Also applicable:

	https://www.youtube.com/watch?v=XZxzJGgox_E

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
  2019-08-15 13:47   ` Lendacky, Thomas
@ 2019-08-15 15:34     ` Borislav Petkov
  2019-08-15 20:14       ` Thomas Gleixner
  0 siblings, 1 reply; 19+ messages in thread
From: Borislav Petkov @ 2019-08-15 15:34 UTC (permalink / raw)
  To: Lendacky, Thomas
  Cc: linux-kernel, linux-doc, linux-pm, x86, Thomas Gleixner,
	Ingo Molnar, Rafael J . Wysocki, Pavel Machek, Chen Yu,
	Jonathan Corbet

On Thu, Aug 15, 2019 at 01:47:24PM +0000, Lendacky, Thomas wrote:
> Sure, I can do that. Do we want to tie this into the nordrand option and
> add rdrand=off or keep that separate?

Yeah, I was looking at that this morning and I'd say keep 'em separate
because if you have to tie, you need to export functions and then
there's

	setup_clear_cpu_cap(X86_FEATURE_RDSEED);

in the nordrand callback but then F15h and F16h don't have RDSEED and
people would wonder, why clear RDSEED on AMD, blabla... so keeping them
separate saves us all that.

> I think this is a clearer indication that the action has taken place.

Yeah, but what does that bring us? You wanna know this now, while
testing. Once that whole effort is done, it is a useless printing of
info which you have in cpuinfo already.

> Not sure what you mean. We can't use the DMI stuff for this. So now, with
> the x86 family checks, if anyone adds some DMI stuff or x86 family stuff
> in the future that matches both the DMI and x86 family checks, this will
> be called more than once and so you need to copy any previous settings and
> add the new ones.

I had a suspicion that it was something like that. Ok, this is not a
big structure currently so I guess it is fine but if it keeps growing,
it would need a proper redesign like making it a list and callbacks
doing list_add_tail() for MSRs which get added. It would avoid that
kmalloc and copying which is silly. Please put a comment ontop why we're
copying.

> Except that X86_FEATURE_RDRAND isn't set anymore. I could create a new
> software feature that is set when the CPUID bit is cleared if that's
> preferred.

Nah, let's leave it like you had it.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
  2019-08-15 15:34     ` Borislav Petkov
@ 2019-08-15 20:14       ` Thomas Gleixner
  0 siblings, 0 replies; 19+ messages in thread
From: Thomas Gleixner @ 2019-08-15 20:14 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Lendacky, Thomas, linux-kernel, linux-doc, linux-pm, x86,
	Ingo Molnar, Rafael J . Wysocki, Pavel Machek, Chen Yu,
	Jonathan Corbet

On Thu, 15 Aug 2019, Borislav Petkov wrote:
> On Thu, Aug 15, 2019 at 01:47:24PM +0000, Lendacky, Thomas wrote:
> > I think this is a clearer indication that the action has taken place.
> 
> Yeah, but what does that bring us? You wanna know this now, while
> testing. Once that whole effort is done, it is a useless printing of
> info which you have in cpuinfo already.

No. Print something useful in dmesg, telling the user that and also why
this has been disabled.

That avoids stupid questions and if they come up nevertheless we can reduce
the answer to LMGT4Y.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
  2019-08-14 21:17 [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h Lendacky, Thomas
  2019-08-14 23:24 ` Non-random RDRAND " Pavel Machek
  2019-08-15  7:21 ` Borislav Petkov
@ 2019-08-15 20:59 ` Andrew Cooper
  2019-08-15 21:04   ` Thomas Gleixner
  2019-08-15 21:05   ` Borislav Petkov
  2019-08-16 15:19 ` Andy Lutomirski
  3 siblings, 2 replies; 19+ messages in thread
From: Andrew Cooper @ 2019-08-15 20:59 UTC (permalink / raw)
  To: Lendacky, Thomas, linux-kernel, linux-doc, linux-pm, x86
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Rafael J . Wysocki, Pavel Machek, Chen Yu, Jonathan Corbet

On 14/08/2019 22:17, Lendacky, Thomas wrote:
> +static void init_hide_rdrand(struct cpuinfo_x86 *c)
> +{
> +	/*
> +	 * The nordrand option can clear X86_FEATURE_RDRAND, so check for
> +	 * RDRAND support using the CPUID function directly.
> +	 */
> +	if (!(cpuid_ecx(1) & BIT(30)) || rdrand_force)
> +		return;
> +
> +	msr_clear_bit(MSR_AMD64_CPUID_FN_00000001, 62);
> +	clear_cpu_cap(c, X86_FEATURE_RDRAND);
> +	pr_info_once("hiding RDRAND via CPUID\n");

If you're virtualised, the write to MSR_AMD64_CPUID_FN_1 almost
certainly won't take effect, which means userspace will still be able to
see the bit.

Best to leave everything untouched if you can't actually clear the bit. 
All you can do is trust that your hypervisor knows what it is doing.

~Andrew

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
  2019-08-15 20:59 ` Andrew Cooper
@ 2019-08-15 21:04   ` Thomas Gleixner
  2019-08-15 21:05   ` Borislav Petkov
  1 sibling, 0 replies; 19+ messages in thread
From: Thomas Gleixner @ 2019-08-15 21:04 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Lendacky, Thomas, linux-kernel, linux-doc, linux-pm, x86,
	Ingo Molnar, Borislav Petkov, Rafael J . Wysocki, Pavel Machek,
	Chen Yu, Jonathan Corbet

[-- Attachment #1: Type: text/plain, Size: 1048 bytes --]

On Thu, 15 Aug 2019, Andrew Cooper wrote:

> On 14/08/2019 22:17, Lendacky, Thomas wrote:
> > +static void init_hide_rdrand(struct cpuinfo_x86 *c)
> > +{
> > +	/*
> > +	 * The nordrand option can clear X86_FEATURE_RDRAND, so check for
> > +	 * RDRAND support using the CPUID function directly.
> > +	 */
> > +	if (!(cpuid_ecx(1) & BIT(30)) || rdrand_force)
> > +		return;
> > +
> > +	msr_clear_bit(MSR_AMD64_CPUID_FN_00000001, 62);
> > +	clear_cpu_cap(c, X86_FEATURE_RDRAND);
> > +	pr_info_once("hiding RDRAND via CPUID\n");
> 
> If you're virtualised, the write to MSR_AMD64_CPUID_FN_1 almost
> certainly won't take effect, which means userspace will still be able to
> see the bit.
> 
> Best to leave everything untouched if you can't actually clear the bit. 
> All you can do is trust that your hypervisor knows what it is doing.

Well, we can read the CPUID entry again after writing that MSR bit. If it
still says RDRAND is available then we know that the hypervisor did not
allow the write and print something to that effect.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
  2019-08-15 20:59 ` Andrew Cooper
  2019-08-15 21:04   ` Thomas Gleixner
@ 2019-08-15 21:05   ` Borislav Petkov
  2019-08-15 21:25     ` Andrew Cooper
  1 sibling, 1 reply; 19+ messages in thread
From: Borislav Petkov @ 2019-08-15 21:05 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Lendacky, Thomas, linux-kernel, linux-doc, linux-pm, x86,
	Thomas Gleixner, Ingo Molnar, Rafael J . Wysocki, Pavel Machek,
	Chen Yu, Jonathan Corbet

On Thu, Aug 15, 2019 at 09:59:03PM +0100, Andrew Cooper wrote:
> If you're virtualised, the write to MSR_AMD64_CPUID_FN_1 almost
> certainly won't take effect, which means userspace will still be able to
> see the bit.

msr_clear_bit() has a return value. We should check it before
doing anything further. I hope the HV actually signals the write
success/failure properly so that we get a correct return value.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
  2019-08-15 21:05   ` Borislav Petkov
@ 2019-08-15 21:25     ` Andrew Cooper
  2019-08-17  8:44       ` Borislav Petkov
  0 siblings, 1 reply; 19+ messages in thread
From: Andrew Cooper @ 2019-08-15 21:25 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Lendacky, Thomas, linux-kernel, linux-doc, linux-pm, x86,
	Thomas Gleixner, Ingo Molnar, Rafael J . Wysocki, Pavel Machek,
	Chen Yu, Jonathan Corbet

On 15/08/2019 22:05, Borislav Petkov wrote:
> On Thu, Aug 15, 2019 at 09:59:03PM +0100, Andrew Cooper wrote:
>> If you're virtualised, the write to MSR_AMD64_CPUID_FN_1 almost
>> certainly won't take effect, which means userspace will still be able to
>> see the bit.
> msr_clear_bit() has a return value. We should check it before
> doing anything further. I hope the HV actually signals the write
> success/failure properly so that we get a correct return value.

I'm afraid that a number of hypervisors do write-discard, given the
propensity of OSes (certainly traditionally) to go poking at bits like
this without wrmsr_safe().

You either need to read the MSR back and observe that the bit has really
changed, or in this case as Thomas suggests, look at CPUID again (which
will likely be the faster option for the non-virtualised case).

~Andrew

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Non-random RDRAND Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
  2019-08-15 15:12   ` Theodore Y. Ts'o
@ 2019-08-16  9:07     ` Pavel Machek
  2019-08-16 14:42     ` Neil Horman
  1 sibling, 0 replies; 19+ messages in thread
From: Pavel Machek @ 2019-08-16  9:07 UTC (permalink / raw)
  To: Theodore Y. Ts'o, Lendacky, Thomas, nhorman, linux-kernel,
	linux-doc, linux-pm, x86, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, Rafael J . Wysocki, Chen Yu, Jonathan Corbet

[-- Attachment #1: Type: text/plain, Size: 1031 bytes --]

On Thu 2019-08-15 11:12:24, Theodore Y. Ts'o wrote:
> On Thu, Aug 15, 2019 at 01:24:35AM +0200, Pavel Machek wrote:
> > Burn it with fire!
> > 
> > I mean... people were afraid RDRAND would be backdoored, and you now
> > confirm ... it indeed _is_ backdoored? /., here's news for you!
> 
> To be fair to AMD, I wouldn't call it a backdoor.  Hanlon's razor is
> applicable here:
> 
> 	"Never attribute to malice that which can be adequately
> 	explained by neglect."

> (Sometimes other words are used instead of neglect, but i'm trying to
> be nice.)

You are right, I thought it was returning values with low entropy, and
it returns ~0 (so -- really really low entropy :-) and can't be
clasified as a backdoor.

Anyway, AMD is _not_ doing good job right now.

I'd expect:

a) CVE reference

b) real fix; if BIOS can init the rng, so can kernel

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Non-random RDRAND Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
  2019-08-15 15:12   ` Theodore Y. Ts'o
  2019-08-16  9:07     ` Pavel Machek
@ 2019-08-16 14:42     ` Neil Horman
  1 sibling, 0 replies; 19+ messages in thread
From: Neil Horman @ 2019-08-16 14:42 UTC (permalink / raw)
  To: Theodore Y. Ts'o, Pavel Machek, Lendacky, Thomas,
	linux-kernel, linux-doc, linux-pm, x86, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, Rafael J . Wysocki, Chen Yu,
	Jonathan Corbet

On Thu, Aug 15, 2019 at 11:12:24AM -0400, Theodore Y. Ts'o wrote:
> On Thu, Aug 15, 2019 at 01:24:35AM +0200, Pavel Machek wrote:
> > Burn it with fire!
> > 
> > I mean... people were afraid RDRAND would be backdoored, and you now
> > confirm ... it indeed _is_ backdoored? /., here's news for you!
> 
> To be fair to AMD, I wouldn't call it a backdoor.  Hanlon's razor is
> applicable here:
> 
> 	"Never attribute to malice that which can be adequately
> 	explained by neglect."
> 
> (Sometimes other words are used instead of neglect, but i'm trying to
> be nice.)
> 
Is it worth setting up a quirk for the Excavator era cpus, that triggers
a call to rdseed on resume?  Working under the assumption that calling
rdseed would kick the rdrand instruction back into gear.

Neil

> 
> 					- Ted
> 
> P.S.   Also applicable:
> 
> 	https://www.youtube.com/watch?v=XZxzJGgox_E
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
  2019-08-14 21:17 [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h Lendacky, Thomas
                   ` (2 preceding siblings ...)
  2019-08-15 20:59 ` Andrew Cooper
@ 2019-08-16 15:19 ` Andy Lutomirski
  3 siblings, 0 replies; 19+ messages in thread
From: Andy Lutomirski @ 2019-08-16 15:19 UTC (permalink / raw)
  To: Lendacky, Thomas, linux-kernel, linux-doc, linux-pm, x86
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Rafael J . Wysocki, Pavel Machek, Chen Yu, Jonathan Corbet,
	Andrew Cooper

On 8/14/19 2:17 PM, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> There have been reports of RDRAND issues after resuming from suspend on
> some AMD family 15h and family 16h systems. This issue stems from BIOS
> not performing the proper steps during resume to ensure RDRAND continues
> to function properly.

Can you or someone from AMD document *precisely* what goes wrong here? 
The APM is crystal clear:

Hardware modifies the CF flag to indicate whether the value returned in 
the destination register is valid. If CF = 1, the value is valid. If CF 
= 0, the value is invalid.

If BIOS screws up and somehow RDRAND starts failing and returning CF = 
0, then I think it's legitimate to call it a BIOS bug.  Some degree of 
documentation would be nice, as would a way for BIOS to indicate to the 
OS that it does not have this bug.

But, from the reports, it sounds like RDRAND starts failing, setting CF 
= 1, and returning 0xFFFF.... in the destination register.  If true, 
then this is, in my book, a severe CPU bug.  Software is supposed to be 
able to trust that, if RDRAND sets CF = 1, the result is a 
cryptographically secure random number, even if everything else in the 
system is actively malicious.  On a SEV-ES system, this should be 
considered a security hole -- even if the hypervisor and BIOS collude, 
RDRAND in the guest should work as defined by the manual.

So, can you clarify what is actually going on?  And, if there is an 
issue where the CPU does not behave as documented in the APM, and AMD 
issue an erratum?  And ideally also fix it in microcode or in a stepping 
and give an indication that the issue is fixed?

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
  2019-08-15 21:25     ` Andrew Cooper
@ 2019-08-17  8:44       ` Borislav Petkov
  2019-08-17 11:43         ` Andrew Cooper
  0 siblings, 1 reply; 19+ messages in thread
From: Borislav Petkov @ 2019-08-17  8:44 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Lendacky, Thomas, linux-kernel, linux-doc, linux-pm, x86,
	Thomas Gleixner, Ingo Molnar, Rafael J . Wysocki, Pavel Machek,
	Chen Yu, Jonathan Corbet

On Thu, Aug 15, 2019 at 10:25:24PM +0100, Andrew Cooper wrote:
> I'm afraid that a number of hypervisors do write-discard, given the
> propensity of OSes (certainly traditionally) to go poking at bits like
> this without wrmsr_safe().
> 
> You either need to read the MSR back and observe that the bit has really
> changed, or in this case as Thomas suggests, look at CPUID again (which
> will likely be the faster option for the non-virtualised case).

One thing I didn't think of when we talked about this: this happens only
after you resume the hypervisor. And the words "resume the hypervisor"
already means an improbable use case. Yeah, yeah, one can close the
laptop lid of her/his F15h or F16h machine while guests are running and
when the HV resumes, those guests won't get randomness but I can't seem
to find it in myself to say, uuh, that's an important use case...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
  2019-08-17  8:44       ` Borislav Petkov
@ 2019-08-17 11:43         ` Andrew Cooper
  2019-08-18 16:32           ` Thomas Gleixner
  0 siblings, 1 reply; 19+ messages in thread
From: Andrew Cooper @ 2019-08-17 11:43 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Lendacky, Thomas, linux-kernel, linux-doc, linux-pm, x86,
	Thomas Gleixner, Ingo Molnar, Rafael J . Wysocki, Pavel Machek,
	Chen Yu, Jonathan Corbet

On 17/08/2019 09:44, Borislav Petkov wrote:
> On Thu, Aug 15, 2019 at 10:25:24PM +0100, Andrew Cooper wrote:
>> I'm afraid that a number of hypervisors do write-discard, given the
>> propensity of OSes (certainly traditionally) to go poking at bits like
>> this without wrmsr_safe().
>>
>> You either need to read the MSR back and observe that the bit has really
>> changed, or in this case as Thomas suggests, look at CPUID again (which
>> will likely be the faster option for the non-virtualised case).
> One thing I didn't think of when we talked about this: this happens only
> after you resume the hypervisor.

:) It hadn't escaped my notice, hence the intervention on this thread.

> And the words "resume the hypervisor" already means an improbable use case.

Qubes and OpenXT are two laptop+hypervisor oriented distros where
suspend/resume is a big deal, and these will have to follow AMD's
recommendation here.

However, for servers which don't do S3/S4, we can reason about safely
leaving RDRAND enabled, irrespective of guest configuration.

~Andrew

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h
  2019-08-17 11:43         ` Andrew Cooper
@ 2019-08-18 16:32           ` Thomas Gleixner
  0 siblings, 0 replies; 19+ messages in thread
From: Thomas Gleixner @ 2019-08-18 16:32 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Borislav Petkov, Lendacky, Thomas, linux-kernel, linux-doc,
	linux-pm, x86, Ingo Molnar, Rafael J . Wysocki, Pavel Machek,
	Chen Yu, Jonathan Corbet

On Sat, 17 Aug 2019, Andrew Cooper wrote:
> On 17/08/2019 09:44, Borislav Petkov wrote:
> > On Thu, Aug 15, 2019 at 10:25:24PM +0100, Andrew Cooper wrote:
> >> I'm afraid that a number of hypervisors do write-discard, given the
> >> propensity of OSes (certainly traditionally) to go poking at bits like
> >> this without wrmsr_safe().
> >>
> >> You either need to read the MSR back and observe that the bit has really
> >> changed, or in this case as Thomas suggests, look at CPUID again (which
> >> will likely be the faster option for the non-virtualised case).
> > One thing I didn't think of when we talked about this: this happens only
> > after you resume the hypervisor.
> 
> :) It hadn't escaped my notice, hence the intervention on this thread.
> 
> > And the words "resume the hypervisor" already means an improbable use case.
> 
> Qubes and OpenXT are two laptop+hypervisor oriented distros where
> suspend/resume is a big deal, and these will have to follow AMD's
> recommendation here.
> 
> However, for servers which don't do S3/S4, we can reason about safely
> leaving RDRAND enabled, irrespective of guest configuration.

Let the administrator reason about it. Default is off for sanity sake.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2019-08-18 16:32 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-14 21:17 [PATCH] x86/CPU/AMD: Clear RDRAND CPUID bit on AMD family 15h/16h Lendacky, Thomas
2019-08-14 23:24 ` Non-random RDRAND " Pavel Machek
2019-08-14 23:38   ` Pavel Machek
2019-08-15 13:01   ` Lendacky, Thomas
2019-08-15 15:12   ` Theodore Y. Ts'o
2019-08-16  9:07     ` Pavel Machek
2019-08-16 14:42     ` Neil Horman
2019-08-15  7:21 ` Borislav Petkov
2019-08-15 13:47   ` Lendacky, Thomas
2019-08-15 15:34     ` Borislav Petkov
2019-08-15 20:14       ` Thomas Gleixner
2019-08-15 20:59 ` Andrew Cooper
2019-08-15 21:04   ` Thomas Gleixner
2019-08-15 21:05   ` Borislav Petkov
2019-08-15 21:25     ` Andrew Cooper
2019-08-17  8:44       ` Borislav Petkov
2019-08-17 11:43         ` Andrew Cooper
2019-08-18 16:32           ` Thomas Gleixner
2019-08-16 15:19 ` Andy Lutomirski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.