[PATCH 0/3] xen/pv: sanitize xen pv guest msr accesses

[PATCH 0/3] xen/pv: sanitize xen pv guest msr accesses

[Juergen Gross]

Historically when running as Xen PV guest all MSR accesses have been
silently swallowing any GP faults, even when the kernel was using not
the *msr_safe() access functions.

Change that by making the behavior controllable via kernel config and
via a boot parameter.

This will help finding paths where MSRs are being accessed under Xen
which are not emulated by the hypervisor.

Juergen Gross (3):
  xen/pv: allow pmu msr accesses to cause GP
  xen/pv: refactor msr access functions to support safe and unsafe
    accesses
  xen/pv: support selecting safe/unsafe msr accesses

 .../admin-guide/kernel-parameters.txt         |  6 ++
 arch/x86/xen/Kconfig                          |  9 ++
 arch/x86/xen/enlighten_pv.c                   | 97 +++++++++++++------
 arch/x86/xen/pmu.c                            | 44 +++++----
 4 files changed, 110 insertions(+), 46 deletions(-)

-- 
2.35.3

[PATCH 1/3] xen/pv: allow pmu msr accesses to cause GP

[Juergen Gross]

Today pmu_msr_read() and pmu_msr_write() fall back to the safe variants
of read/write MSR in case the MSR access isn't emulated via Xen. Allow
the caller to select the potentially faulting variant by passing NULL
for the error pointer.

Remove one level of indentation by restructuring the code a little bit.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/xen/pmu.c | 44 ++++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/arch/x86/xen/pmu.c b/arch/x86/xen/pmu.c
index 21ecbe754cb2..34b4144f6041 100644
--- a/arch/x86/xen/pmu.c
+++ b/arch/x86/xen/pmu.c
@@ -293,22 +293,24 @@ static bool xen_amd_pmu_emulate(unsigned int msr, u64 *val, bool is_read)
 bool pmu_msr_read(unsigned int msr, uint64_t *val, int *err)
 {
 	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) {
-		if (is_amd_pmu_msr(msr)) {
-			if (!xen_amd_pmu_emulate(msr, val, 1))
-				*val = native_read_msr_safe(msr, err);
-			return true;
+		if (!is_amd_pmu_msr(msr))
+			return false;
+		if (!xen_amd_pmu_emulate(msr, val, 1)) {
+			*val = err ? native_read_msr_safe(msr, err)
+				   : native_read_msr(msr);
 		}
+		return true;
 	} else {
 		int type, index;
 
-		if (is_intel_pmu_msr(msr, &type, &index)) {
-			if (!xen_intel_pmu_emulate(msr, val, type, index, 1))
-				*val = native_read_msr_safe(msr, err);
-			return true;
+		if (!is_intel_pmu_msr(msr, &type, &index))
+			return false;
+		if (!xen_intel_pmu_emulate(msr, val, type, index, 1)) {
+			*val = err ? native_read_msr_safe(msr, err)
+				   : native_read_msr(msr);
 		}
+		return true;
 	}
-
-	return false;
 }
 
 bool pmu_msr_write(unsigned int msr, uint32_t low, uint32_t high, int *err)
@@ -316,22 +318,28 @@ bool pmu_msr_write(unsigned int msr, uint32_t low, uint32_t high, int *err)
 	uint64_t val = ((uint64_t)high << 32) | low;
 
 	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) {
-		if (is_amd_pmu_msr(msr)) {
-			if (!xen_amd_pmu_emulate(msr, &val, 0))
+		if (!is_amd_pmu_msr(msr))
+			return false;
+		if (!xen_amd_pmu_emulate(msr, &val, 0)) {
+			if (err)
 				*err = native_write_msr_safe(msr, low, high);
-			return true;
+			else
+				native_write_msr(msr, low, high);
 		}
+		return true;
 	} else {
 		int type, index;
 
-		if (is_intel_pmu_msr(msr, &type, &index)) {
-			if (!xen_intel_pmu_emulate(msr, &val, type, index, 0))
+		if (!is_intel_pmu_msr(msr, &type, &index))
+			return false;
+		if (!xen_intel_pmu_emulate(msr, &val, type, index, 0)) {
+			if (err)
 				*err = native_write_msr_safe(msr, low, high);
-			return true;
+			else
+				native_write_msr(msr, low, high);
 		}
+		return true;
 	}
-
-	return false;
 }
 
 static unsigned long long xen_amd_read_pmc(int counter)
-- 
2.35.3

Re: [PATCH 1/3] xen/pv: allow pmu msr accesses to cause GP

[Jan Beulich]

On 26.09.2022 16:18, Juergen Gross wrote:
> Today pmu_msr_read() and pmu_msr_write() fall back to the safe variants
> of read/write MSR in case the MSR access isn't emulated via Xen. Allow
> the caller to select the potentially faulting variant by passing NULL
> for the error pointer.

Maybe make this "the sole caller" or some such? Because if there were
multiple, they might easily disagree on what the best meaning of passing
NULL is.

> --- a/arch/x86/xen/pmu.c
> +++ b/arch/x86/xen/pmu.c
> @@ -293,22 +293,24 @@ static bool xen_amd_pmu_emulate(unsigned int msr, u64 *val, bool is_read)
>  bool pmu_msr_read(unsigned int msr, uint64_t *val, int *err)
>  {
>  	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) {
> -		if (is_amd_pmu_msr(msr)) {
> -			if (!xen_amd_pmu_emulate(msr, val, 1))
> -				*val = native_read_msr_safe(msr, err);
> -			return true;
> +		if (!is_amd_pmu_msr(msr))
> +			return false;
> +		if (!xen_amd_pmu_emulate(msr, val, 1)) {
> +			*val = err ? native_read_msr_safe(msr, err)
> +				   : native_read_msr(msr);
>  		}
> +		return true;

Minor remark: Fold this and ...

>  	} else {
>  		int type, index;
>  
> -		if (is_intel_pmu_msr(msr, &type, &index)) {
> -			if (!xen_intel_pmu_emulate(msr, val, type, index, 1))
> -				*val = native_read_msr_safe(msr, err);
> -			return true;
> +		if (!is_intel_pmu_msr(msr, &type, &index))
> +			return false;
> +		if (!xen_intel_pmu_emulate(msr, val, type, index, 1)) {
> +			*val = err ? native_read_msr_safe(msr, err)
> +				   : native_read_msr(msr);
>  		}
> +		return true;

... this by moving them ...

>  	}
> -
> -	return false;
>  }

... above here? You might even de-duplicate the native_read_msr{,_safe}()
invocations by moving them out of the if/else ...

Jan

Re: [PATCH 1/3] xen/pv: allow pmu msr accesses to cause GP

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1954 bytes --]

On 26.09.22 17:29, Jan Beulich wrote:
> On 26.09.2022 16:18, Juergen Gross wrote:
>> Today pmu_msr_read() and pmu_msr_write() fall back to the safe variants
>> of read/write MSR in case the MSR access isn't emulated via Xen. Allow
>> the caller to select the potentially faulting variant by passing NULL
>> for the error pointer.
> 
> Maybe make this "the sole caller" or some such? Because if there were
> multiple, they might easily disagree on what the best meaning of passing
> NULL is.

Okay.

> 
>> --- a/arch/x86/xen/pmu.c
>> +++ b/arch/x86/xen/pmu.c
>> @@ -293,22 +293,24 @@ static bool xen_amd_pmu_emulate(unsigned int msr, u64 *val, bool is_read)
>>   bool pmu_msr_read(unsigned int msr, uint64_t *val, int *err)
>>   {
>>   	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) {
>> -		if (is_amd_pmu_msr(msr)) {
>> -			if (!xen_amd_pmu_emulate(msr, val, 1))
>> -				*val = native_read_msr_safe(msr, err);
>> -			return true;
>> +		if (!is_amd_pmu_msr(msr))
>> +			return false;
>> +		if (!xen_amd_pmu_emulate(msr, val, 1)) {
>> +			*val = err ? native_read_msr_safe(msr, err)
>> +				   : native_read_msr(msr);
>>   		}
>> +		return true;
> 
> Minor remark: Fold this and ...
> 
>>   	} else {
>>   		int type, index;
>>   
>> -		if (is_intel_pmu_msr(msr, &type, &index)) {
>> -			if (!xen_intel_pmu_emulate(msr, val, type, index, 1))
>> -				*val = native_read_msr_safe(msr, err);
>> -			return true;
>> +		if (!is_intel_pmu_msr(msr, &type, &index))
>> +			return false;
>> +		if (!xen_intel_pmu_emulate(msr, val, type, index, 1)) {
>> +			*val = err ? native_read_msr_safe(msr, err)
>> +				   : native_read_msr(msr);
>>   		}
>> +		return true;
> 
> ... this by moving them ...
> 
>>   	}
>> -
>> -	return false;
>>   }
> 
> ... above here? You might even de-duplicate the native_read_msr{,_safe}()
> invocations by moving them out of the if/else ...

Oh, nice idea!


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 1/3] xen/pv: allow pmu msr accesses to cause GP

[Boris Ostrovsky]

On 9/26/22 10:18 AM, Juergen Gross wrote:
>   bool pmu_msr_read(unsigned int msr, uint64_t *val, int *err)
>   {
>   	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) {
> -		if (is_amd_pmu_msr(msr)) {
> -			if (!xen_amd_pmu_emulate(msr, val, 1))
> -				*val = native_read_msr_safe(msr, err);
> -			return true;
> +		if (!is_amd_pmu_msr(msr))


You should be able to move vendor check inside is_<vendor>_pmu_msr().


-boris


> +			return false;
> +		if (!xen_amd_pmu_emulate(msr, val, 1)) {
> +			*val = err ? native_read_msr_safe(msr, err)
> +				   : native_read_msr(msr);
>   		}
> +		return true;
>   	} else {

Re: [PATCH 1/3] xen/pv: allow pmu msr accesses to cause GP

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 696 bytes --]

On 26.09.22 22:09, Boris Ostrovsky wrote:
> 
> On 9/26/22 10:18 AM, Juergen Gross wrote:
>>   bool pmu_msr_read(unsigned int msr, uint64_t *val, int *err)
>>   {
>>       if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) {
>> -        if (is_amd_pmu_msr(msr)) {
>> -            if (!xen_amd_pmu_emulate(msr, val, 1))
>> -                *val = native_read_msr_safe(msr, err);
>> -            return true;
>> +        if (!is_amd_pmu_msr(msr))
> 
> 
> You should be able to move vendor check inside is_<vendor>_pmu_msr().

I like that. Together with Jan's suggestion this makes the code much
more readable!


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[PATCH 2/3] xen/pv: refactor msr access functions to support safe and unsafe accesses

[Juergen Gross]

Refactor and rename xen_read_msr_safe() and xen_write_msr_safe() to
support both cases of MSR accesses, safe ones and potentially GP-fault
generating ones.

This will prepare to no longer swallow GPs silently in xen_read_msr()
and xen_write_msr().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/xen/enlighten_pv.c | 73 ++++++++++++++++++++++++++++---------
 1 file changed, 55 insertions(+), 18 deletions(-)

diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 0ed2e487a693..4e68e047df94 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -916,14 +916,18 @@ static void xen_write_cr4(unsigned long cr4)
 	native_write_cr4(cr4);
 }
 
-static u64 xen_read_msr_safe(unsigned int msr, int *err)
+static u64 xen_do_read_msr(unsigned int msr, int *err)
 {
 	u64 val;
 
 	if (pmu_msr_read(msr, &val, err))
 		return val;
 
-	val = native_read_msr_safe(msr, err);
+	if (err)
+		val = native_read_msr_safe(msr, err);
+	else
+		val = native_read_msr(msr);
+
 	switch (msr) {
 	case MSR_IA32_APICBASE:
 		val &= ~X2APIC_ENABLE;
@@ -932,23 +936,39 @@ static u64 xen_read_msr_safe(unsigned int msr, int *err)
 	return val;
 }
 
-static int xen_write_msr_safe(unsigned int msr, unsigned low, unsigned high)
+static void set_seg(unsigned int which, unsigned int low, unsigned int high,
+		    int *err)
 {
-	int ret;
-	unsigned int which;
-	u64 base;
+	u64 base = ((u64)high << 32) | low;
+
+	if (HYPERVISOR_set_segment_base(which, base) == 0)
+		return;
 
-	ret = 0;
+	if (err)
+		*err = -EIO;
+	else
+		WARN(1, "Xen set_segment_base(%u, %llx) failed\n", which, base);
+}
 
+/*
+ * Support write_msr_safe() and write_msr() semantics.
+ * With err == NULL write_msr() semantics are selected.
+ * Supplying an err pointer requires err to be pre-initialized with 0.
+ */
+static void xen_do_write_msr(unsigned int msr, unsigned int low,
+			     unsigned int high, int *err)
+{
 	switch (msr) {
-	case MSR_FS_BASE:		which = SEGBASE_FS; goto set;
-	case MSR_KERNEL_GS_BASE:	which = SEGBASE_GS_USER; goto set;
-	case MSR_GS_BASE:		which = SEGBASE_GS_KERNEL; goto set;
-
-	set:
-		base = ((u64)high << 32) | low;
-		if (HYPERVISOR_set_segment_base(which, base) != 0)
-			ret = -EIO;
+	case MSR_FS_BASE:
+		set_seg(SEGBASE_FS, low, high, err);
+		break;
+
+	case MSR_KERNEL_GS_BASE:
+		set_seg(SEGBASE_GS_USER, low, high, err);
+		break;
+
+	case MSR_GS_BASE:
+		set_seg(SEGBASE_GS_KERNEL, low, high, err);
 		break;
 
 	case MSR_STAR:
@@ -964,11 +984,28 @@ static int xen_write_msr_safe(unsigned int msr, unsigned low, unsigned high)
 		break;
 
 	default:
-		if (!pmu_msr_write(msr, low, high, &ret))
-			ret = native_write_msr_safe(msr, low, high);
+		if (!pmu_msr_write(msr, low, high, err)) {
+			if (err)
+				*err = native_write_msr_safe(msr, low, high);
+			else
+				native_write_msr(msr, low, high);
+		}
 	}
+}
+
+static u64 xen_read_msr_safe(unsigned int msr, int *err)
+{
+	return xen_do_read_msr(msr, err);
+}
+
+static int xen_write_msr_safe(unsigned int msr, unsigned int low,
+			      unsigned int high)
+{
+	int err = 0;
+
+	xen_do_write_msr(msr, low, high, &err);
 
-	return ret;
+	return err;
 }
 
 static u64 xen_read_msr(unsigned int msr)
-- 
2.35.3

[PATCH 3/3] xen/pv: support selecting safe/unsafe msr accesses

[Juergen Gross]

Instead of always doing the safe variants for reading and writing MSRs
in Xen PV guests, make the behavior controllable via Kconfig option
and a boot parameter.

The default will be the current behavior, which is to always use the
safe variant.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 .../admin-guide/kernel-parameters.txt         |  6 +++++
 arch/x86/xen/Kconfig                          |  9 +++++++
 arch/x86/xen/enlighten_pv.c                   | 24 +++++++++++--------
 3 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 426fa892d311..1bda9cf18fae 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6836,6 +6836,12 @@
 			Crash from Xen panic notifier, without executing late
 			panic() code such as dumping handler.
 
+	xen_msr_safe=	[X86,XEN]
+			Format: <bool>
+			Select whether to always use non-faulting (safe) MSR
+			access functions when running as Xen PV guest. The
+			default value is controlled by CONFIG_XEN_PV_MSR_SAFE.
+
 	xen_nopvspin	[X86,XEN]
 			Disables the qspinlock slowpath using Xen PV optimizations.
 			This parameter is obsoleted by "nopvspin" parameter, which
diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
index 85246dd9faa1..9b1ec5d8c99c 100644
--- a/arch/x86/xen/Kconfig
+++ b/arch/x86/xen/Kconfig
@@ -92,3 +92,12 @@ config XEN_DOM0
 	select X86_X2APIC if XEN_PVH && X86_64
 	help
 	  Support running as a Xen Dom0 guest.
+
+config XEN_PV_MSR_SAFE
+	bool "Always use safe MSR accesses in PV guests"
+	default y
+	depends on XEN_PV
+	help
+	  Use safe (not faulting) MSR access functions even if the MSR access
+	  should not fault anyway.
+	  The default can be changed by using the "xen_msr_safe" boot parameter.
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 4e68e047df94..6b0e5d4c485a 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -108,6 +108,16 @@ struct tls_descs {
  */
 static DEFINE_PER_CPU(struct tls_descs, shadow_tls_desc);
 
+static __read_mostly bool xen_msr_safe = IS_ENABLED(CONFIG_XEN_PV_MSR_SAFE);
+
+static int __init parse_xen_msr_safe(char *str)
+{
+	if (str)
+		return strtobool(str, &xen_msr_safe);
+	return -EINVAL;
+}
+early_param("xen_msr_safe", parse_xen_msr_safe);
+
 static void __init xen_pv_init_platform(void)
 {
 	/* PV guests can't operate virtio devices without grants. */
@@ -1010,22 +1020,16 @@ static int xen_write_msr_safe(unsigned int msr, unsigned int low,
 
 static u64 xen_read_msr(unsigned int msr)
 {
-	/*
-	 * This will silently swallow a #GP from RDMSR.  It may be worth
-	 * changing that.
-	 */
 	int err;
 
-	return xen_read_msr_safe(msr, &err);
+	return xen_do_read_msr(msr, xen_msr_safe ? &err : NULL);
 }
 
 static void xen_write_msr(unsigned int msr, unsigned low, unsigned high)
 {
-	/*
-	 * This will silently swallow a #GP from WRMSR.  It may be worth
-	 * changing that.
-	 */
-	xen_write_msr_safe(msr, low, high);
+	int err;
+
+	xen_do_write_msr(msr, low, high, xen_msr_safe ? &err : NULL);
 }
 
 /* This is called once we have the cpu_possible_mask */
-- 
2.35.3

Re: [PATCH 3/3] xen/pv: support selecting safe/unsafe msr accesses

[Jan Beulich]

On 26.09.2022 16:18, Juergen Gross wrote:
> --- a/arch/x86/xen/Kconfig
> +++ b/arch/x86/xen/Kconfig
> @@ -92,3 +92,12 @@ config XEN_DOM0
>  	select X86_X2APIC if XEN_PVH && X86_64
>  	help
>  	  Support running as a Xen Dom0 guest.
> +
> +config XEN_PV_MSR_SAFE
> +	bool "Always use safe MSR accesses in PV guests"
> +	default y

Is there any time line when this default will change, perhaps first to
DEBUG and later to N?

> @@ -1010,22 +1020,16 @@ static int xen_write_msr_safe(unsigned int msr, unsigned int low,
>  
>  static u64 xen_read_msr(unsigned int msr)
>  {
> -	/*
> -	 * This will silently swallow a #GP from RDMSR.  It may be worth
> -	 * changing that.
> -	 */
>  	int err;
>  
> -	return xen_read_msr_safe(msr, &err);
> +	return xen_do_read_msr(msr, xen_msr_safe ? &err : NULL);
>  }

When we were talking at the session, I think I said that at least there
is no uninitialized value being passed back. But I did look at
xen_read_msr_safe() only, which indeed is okay. Whereas
native_read_msr_safe() isn't (nor is native_read_msr() afaict), so I
think part of this series should be to also eliminate the undefined-
ness from this code path (possible now only when xen_msr_safe is true,
but as per above that'll be the default at least for some time), where
the caller has no way to know that it shouldn't look at the value.

Jan

Re: [PATCH 3/3] xen/pv: support selecting safe/unsafe msr accesses

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1720 bytes --]

On 26.09.22 17:23, Jan Beulich wrote:
> On 26.09.2022 16:18, Juergen Gross wrote:
>> --- a/arch/x86/xen/Kconfig
>> +++ b/arch/x86/xen/Kconfig
>> @@ -92,3 +92,12 @@ config XEN_DOM0
>>   	select X86_X2APIC if XEN_PVH && X86_64
>>   	help
>>   	  Support running as a Xen Dom0 guest.
>> +
>> +config XEN_PV_MSR_SAFE
>> +	bool "Always use safe MSR accesses in PV guests"
>> +	default y
> 
> Is there any time line when this default will change, perhaps first to
> DEBUG and later to N?

I'm not sure. I did an initial test with the safe variants disabled in dom0
and it just worked.

I'm not sure we want an intermediate step, as in critical cases the user can
still use the boot parameter.

> 
>> @@ -1010,22 +1020,16 @@ static int xen_write_msr_safe(unsigned int msr, unsigned int low,
>>   
>>   static u64 xen_read_msr(unsigned int msr)
>>   {
>> -	/*
>> -	 * This will silently swallow a #GP from RDMSR.  It may be worth
>> -	 * changing that.
>> -	 */
>>   	int err;
>>   
>> -	return xen_read_msr_safe(msr, &err);
>> +	return xen_do_read_msr(msr, xen_msr_safe ? &err : NULL);
>>   }
> 
> When we were talking at the session, I think I said that at least there
> is no uninitialized value being passed back. But I did look at
> xen_read_msr_safe() only, which indeed is okay. Whereas
> native_read_msr_safe() isn't (nor is native_read_msr() afaict), so I
> think part of this series should be to also eliminate the undefined-
> ness from this code path (possible now only when xen_msr_safe is true,
> but as per above that'll be the default at least for some time), where
> the caller has no way to know that it shouldn't look at the value.

I can add that.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]