[PATCH 0/6] KVM: x86: x2APIC reserved bits/regs fixes

[PATCH 0/6] KVM: x86: x2APIC reserved bits/regs fixes

[Sean Christopherson]

Fixes for edge cases where KVM mishandles reserved bits/regs checks when
the vCPU is in x2APIC mode.

The first two patches were previously posted[*], but both patches were
broken (as posted against upstream), hence I took full credit for doing
the work and changed Marc to a reporter.

The VMX APICv fixes are for bugs found when writing tests.  *sigh*
I didn't Cc those to stable as the odds of breaking something when touching
the MSR bitmaps seemed higher than someone caring about a 10 year old bug.

AMD x2AVIC support may or may not suffer similar interception bugs, but I
don't have hardware to test and this already snowballed further than
expected...

[*] https://lore.kernel.org/kvm/20220525173933.1611076-1-venkateshs@chromium.org

Sean Christopherson (6):
  KVM: x86: Inject #GP if WRMSR sets reserved bits in APIC Self-IPI
  KVM: x86: Inject #GP on x2APIC WRMSR that sets reserved bits 63:32
  KVM: x86: Mark x2APIC DFR reg as non-existent for x2APIC
  KVM: x86: Split out logic to generate "readable" APIC regs mask to
    helper
  KVM: VMX: Always intercept accesses to unsupported "extended" x2APIC
    regs
  KVM: VMX: Intercept reads to invalid and write-only x2APIC registers

 arch/x86/kvm/lapic.c   | 55 ++++++++++++++++++++++++++----------------
 arch/x86/kvm/lapic.h   |  2 ++
 arch/x86/kvm/vmx/vmx.c | 40 +++++++++++++++---------------
 3 files changed, 57 insertions(+), 40 deletions(-)


base-commit: 91dc252b0dbb6879e4067f614df1e397fec532a1
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH 1/6] KVM: x86: Inject #GP if WRMSR sets reserved bits in APIC Self-IPI

[Sean Christopherson]

Inject a #GP if the guest attempts to set reserved bits in the x2APIC-only
Self-IPI register.  Bits 7:0 hold the vector, all other bits are reserved.

Reported-by: Marc Orr <marcorr@google.com>
Cc: Ben Gardon <bgardon@google.com>
Cc: Venkatesh Srinivas <venkateshs@chromium.org>
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 80f92cbc4029..f77da92c6ea6 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2315,10 +2315,14 @@ static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
 		break;
 
 	case APIC_SELF_IPI:
-		if (apic_x2apic_mode(apic))
-			kvm_apic_send_ipi(apic, APIC_DEST_SELF | (val & APIC_VECTOR_MASK), 0);
-		else
+		/*
+		 * Self-IPI exists only when x2APIC is enabled.  Bits 7:0 hold
+		 * the vector, everything else is reserved.
+		 */
+		if (!apic_x2apic_mode(apic) || (val & ~APIC_VECTOR_MASK))
 			ret = 1;
+		else
+			kvm_apic_send_ipi(apic, APIC_DEST_SELF | val, 0);
 		break;
 	default:
 		ret = 1;
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH 2/6] KVM: x86: Inject #GP on x2APIC WRMSR that sets reserved bits 63:32

[Sean Christopherson]

Reject attempts to set bits 63:32 for 32-bit x2APIC registers, i.e. all
x2APIC registers except ICR.  Per Intel's SDM:

  Non-zero writes (by WRMSR instruction) to reserved bits to these
  registers will raise a general protection fault exception

Opportunistically fix a typo in a nearby comment.

Reported-by: Marc Orr <marcorr@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index f77da92c6ea6..bf53e4752f30 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -3108,13 +3108,17 @@ static int kvm_lapic_msr_read(struct kvm_lapic *apic, u32 reg, u64 *data)
 static int kvm_lapic_msr_write(struct kvm_lapic *apic, u32 reg, u64 data)
 {
 	/*
-	 * ICR is a 64-bit register in x2APIC mode (and Hyper'v PV vAPIC) and
+	 * ICR is a 64-bit register in x2APIC mode (and Hyper-V PV vAPIC) and
 	 * can be written as such, all other registers remain accessible only
 	 * through 32-bit reads/writes.
 	 */
 	if (reg == APIC_ICR)
 		return kvm_x2apic_icr_write(apic, data);
 
+	/* Bits 63:32 are reserved in all other registers. */
+	if (data >> 32)
+		return 1;
+
 	return kvm_lapic_reg_write(apic, reg, (u32)data);
 }
 
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH 3/6] KVM: x86: Mark x2APIC DFR reg as non-existent for x2APIC

[Sean Christopherson]

Mark APIC_DFR as being invalid/non-existent in x2APIC mode instead of
handling it as a one-off check in kvm_x2apic_msr_read().  This will allow
reusing "valid_reg_mask" to generate VMX's interception bitmaps for
x2APIC.  Handling DFR in the common read path may also fix the Hyper-V
PV MSR interface, if that can coexist with x2APIC.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index bf53e4752f30..c49b13418638 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1541,7 +1541,6 @@ static int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
 		APIC_REG_MASK(APIC_TASKPRI) |
 		APIC_REG_MASK(APIC_PROCPRI) |
 		APIC_REG_MASK(APIC_LDR) |
-		APIC_REG_MASK(APIC_DFR) |
 		APIC_REG_MASK(APIC_SPIV) |
 		APIC_REGS_MASK(APIC_ISR, APIC_ISR_NR) |
 		APIC_REGS_MASK(APIC_TMR, APIC_ISR_NR) |
@@ -1562,12 +1561,13 @@ static int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
 		valid_reg_mask |= APIC_REG_MASK(APIC_LVTCMCI);
 
 	/*
-	 * ARBPRI and ICR2 are not valid in x2APIC mode.  WARN if KVM reads ICR
-	 * in x2APIC mode as it's an 8-byte register in x2APIC and needs to be
-	 * manually handled by the caller.
+	 * ARBPRI, DFR, and ICR2 are not valid in x2APIC mode.  WARN if KVM
+	 * reads ICR in x2APIC mode as it's an 8-byte register in x2APIC and
+	 * needs to be manually handled by the caller.
 	 */
 	if (!apic_x2apic_mode(apic))
 		valid_reg_mask |= APIC_REG_MASK(APIC_ARBPRI) |
+				  APIC_REG_MASK(APIC_DFR) |
 				  APIC_REG_MASK(APIC_ICR2);
 	else
 		WARN_ON_ONCE(offset == APIC_ICR);
@@ -3141,9 +3141,6 @@ int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
 	if (!lapic_in_kernel(vcpu) || !apic_x2apic_mode(apic))
 		return 1;
 
-	if (reg == APIC_DFR)
-		return 1;
-
 	return kvm_lapic_msr_read(apic, reg, data);
 }
 
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH 4/6] KVM: x86: Split out logic to generate "readable" APIC regs mask to helper

[Sean Christopherson]

Move the generation of the readable APIC regs bitmask to a standalone
helper so that VMX can use the mask for its MSR interception bitmaps.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/lapic.c | 34 +++++++++++++++++++++-------------
 arch/x86/kvm/lapic.h |  2 ++
 2 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index c49b13418638..19697fe9b2c7 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1529,12 +1529,9 @@ static inline struct kvm_lapic *to_lapic(struct kvm_io_device *dev)
 #define APIC_REGS_MASK(first, count) \
 	(APIC_REG_MASK(first) * ((1ull << (count)) - 1))
 
-static int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
-			      void *data)
+u64 kvm_lapic_readable_reg_mask(struct kvm_lapic *apic)
 {
-	unsigned char alignment = offset & 0xf;
-	u32 result;
-	/* this bitmask has a bit cleared for each reserved register */
+	/* Leave bits '0' for reserved and write-only registers. */
 	u64 valid_reg_mask =
 		APIC_REG_MASK(APIC_ID) |
 		APIC_REG_MASK(APIC_LVR) |
@@ -1560,22 +1557,33 @@ static int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
 	if (kvm_lapic_lvt_supported(apic, LVT_CMCI))
 		valid_reg_mask |= APIC_REG_MASK(APIC_LVTCMCI);
 
-	/*
-	 * ARBPRI, DFR, and ICR2 are not valid in x2APIC mode.  WARN if KVM
-	 * reads ICR in x2APIC mode as it's an 8-byte register in x2APIC and
-	 * needs to be manually handled by the caller.
-	 */
+	/* ARBPRI, DFR, and ICR2 are not valid in x2APIC mode. */
 	if (!apic_x2apic_mode(apic))
 		valid_reg_mask |= APIC_REG_MASK(APIC_ARBPRI) |
 				  APIC_REG_MASK(APIC_DFR) |
 				  APIC_REG_MASK(APIC_ICR2);
-	else
-		WARN_ON_ONCE(offset == APIC_ICR);
+
+	return valid_reg_mask;
+}
+EXPORT_SYMBOL_GPL(kvm_lapic_readable_reg_mask);
+
+static int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
+			      void *data)
+{
+	unsigned char alignment = offset & 0xf;
+	u32 result;
+
+	/*
+	 * WARN if KVM reads ICR in x2APIC mode, as it's an 8-byte register in
+	 * x2APIC and needs to be manually handled by the caller.
+	 */
+	WARN_ON_ONCE(apic_x2apic_mode(apic) && offset == APIC_ICR);
 
 	if (alignment + len > 4)
 		return 1;
 
-	if (offset > 0x3f0 || !(valid_reg_mask & APIC_REG_MASK(offset)))
+	if (offset > 0x3f0 ||
+	    !(kvm_lapic_readable_reg_mask(apic) & APIC_REG_MASK(offset)))
 		return 1;
 
 	result = __apic_read(apic, offset & ~0xf);
diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
index df316ede7546..0a0ea4b5dd8c 100644
--- a/arch/x86/kvm/lapic.h
+++ b/arch/x86/kvm/lapic.h
@@ -146,6 +146,8 @@ int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
 int kvm_lapic_set_pv_eoi(struct kvm_vcpu *vcpu, u64 data, unsigned long len);
 void kvm_lapic_exit(void);
 
+u64 kvm_lapic_readable_reg_mask(struct kvm_lapic *apic);
+
 #define VEC_POS(v) ((v) & (32 - 1))
 #define REG_POS(v) (((v) >> 5) << 4)
 
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH 5/6] KVM: VMX: Always intercept accesses to unsupported "extended" x2APIC regs

[Sean Christopherson]

Don't clear the "read" bits for x2APIC registers above SELF_IPI (APIC regs
0x400 - 0xff0, MSRs 0x840 - 0x8ff).  KVM doesn't emulate registers in that
space (there are a smattering of AMD-only extensions) and so should
intercept reads in order to inject #GP.  When APICv is fully enabled,
Intel hardware doesn't validate the registers on RDMSR and instead blindly
retrieves data from the vAPIC page, i.e. it's software's responsibility to
intercept reads to non-existent MSRs.

Fixes: 8d14695f9542 ("x86, apicv: add virtual x2apic support")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/vmx.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index c788aa382611..82c61c16f8f5 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4018,26 +4018,17 @@ void vmx_enable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
 		vmx_set_msr_bitmap_write(msr_bitmap, msr);
 }
 
-static void vmx_reset_x2apic_msrs(struct kvm_vcpu *vcpu, u8 mode)
-{
-	unsigned long *msr_bitmap = to_vmx(vcpu)->vmcs01.msr_bitmap;
-	unsigned long read_intercept;
-	int msr;
-
-	read_intercept = (mode & MSR_BITMAP_MODE_X2APIC_APICV) ? 0 : ~0;
-
-	for (msr = 0x800; msr <= 0x8ff; msr += BITS_PER_LONG) {
-		unsigned int read_idx = msr / BITS_PER_LONG;
-		unsigned int write_idx = read_idx + (0x800 / sizeof(long));
-
-		msr_bitmap[read_idx] = read_intercept;
-		msr_bitmap[write_idx] = ~0ul;
-	}
-}
-
 static void vmx_update_msr_bitmap_x2apic(struct kvm_vcpu *vcpu)
 {
+	/*
+	 * x2APIC indices for 64-bit accesses into the RDMSR and WRMSR halves
+	 * of the MSR bitmap.  KVM emulates APIC registers up through 0x3f0,
+	 * i.e. MSR 0x83f, and so only needs to dynamically manipulate 64 bits.
+	 */
+	const int read_idx = APIC_BASE_MSR / BITS_PER_LONG_LONG;
+	const int write_idx = read_idx + (0x800 / sizeof(u64));
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	u64 *msr_bitmap = (u64 *)vmx->vmcs01.msr_bitmap;
 	u8 mode;
 
 	if (!cpu_has_vmx_msr_bitmap())
@@ -4058,7 +4049,18 @@ static void vmx_update_msr_bitmap_x2apic(struct kvm_vcpu *vcpu)
 
 	vmx->x2apic_msr_bitmap_mode = mode;
 
-	vmx_reset_x2apic_msrs(vcpu, mode);
+	/*
+	 * Reset the bitmap for MSRs 0x800 - 0x83f.  Leave AMD's uber-extended
+	 * registers (0x840 and above) intercepted, KVM doesn't support them.
+	 * Intercept all writes by default and poke holes as needed.  Pass
+	 * through all reads by default in x2APIC+APICv mode, as all registers
+	 * except the current timer count are passed through for read.
+	 */
+	if (mode & MSR_BITMAP_MODE_X2APIC_APICV)
+		msr_bitmap[read_idx] = 0;
+	else
+		msr_bitmap[read_idx] = ~0ull;
+	msr_bitmap[write_idx] = ~0ull;
 
 	/*
 	 * TPR reads and writes can be virtualized even if virtual interrupt
-- 
2.39.0.314.g84b9a713c41-goog

[PATCH 6/6] KVM: VMX: Intercept reads to invalid and write-only x2APIC registers

[Sean Christopherson]

Intercept reads to invalid (non-existent) and write-only x2APIC registers
when configuring VMX's MSR bitmaps for x2APIC+APICv.  When APICv is fully
enabled, Intel hardware doesn't validate the registers on RDMSR and
instead blindly retrieves data from the vAPIC page, i.e. it's software's
responsibility to intercept reads to non-existent and write-only MSRs.

Fixes: 8d14695f9542 ("x86, apicv: add virtual x2apic support")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/vmx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 82c61c16f8f5..1be2bc7185be 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4031,7 +4031,7 @@ static void vmx_update_msr_bitmap_x2apic(struct kvm_vcpu *vcpu)
 	u64 *msr_bitmap = (u64 *)vmx->vmcs01.msr_bitmap;
 	u8 mode;
 
-	if (!cpu_has_vmx_msr_bitmap())
+	if (!cpu_has_vmx_msr_bitmap() || WARN_ON_ONCE(!lapic_in_kernel(vcpu)))
 		return;
 
 	if (cpu_has_secondary_exec_ctrls() &&
@@ -4053,11 +4053,11 @@ static void vmx_update_msr_bitmap_x2apic(struct kvm_vcpu *vcpu)
 	 * Reset the bitmap for MSRs 0x800 - 0x83f.  Leave AMD's uber-extended
 	 * registers (0x840 and above) intercepted, KVM doesn't support them.
 	 * Intercept all writes by default and poke holes as needed.  Pass
-	 * through all reads by default in x2APIC+APICv mode, as all registers
-	 * except the current timer count are passed through for read.
+	 * through reads for all valid registers by default in x2APIC+APICv
+	 * mode, only the current timer count needs on-demand emulation by KVM.
 	 */
 	if (mode & MSR_BITMAP_MODE_X2APIC_APICV)
-		msr_bitmap[read_idx] = 0;
+		msr_bitmap[read_idx] = ~kvm_lapic_readable_reg_mask(vcpu->arch.apic);
 	else
 		msr_bitmap[read_idx] = ~0ull;
 	msr_bitmap[write_idx] = ~0ull;
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH 1/6] KVM: x86: Inject #GP if WRMSR sets reserved bits in APIC Self-IPI

[Maxim Levitsky]

On Sat, 2023-01-07 at 01:10 +0000, Sean Christopherson wrote:
> Inject a #GP if the guest attempts to set reserved bits in the x2APIC-only
> Self-IPI register.  Bits 7:0 hold the vector, all other bits are reserved.
> 
> Reported-by: Marc Orr <marcorr@google.com>
> Cc: Ben Gardon <bgardon@google.com>
> Cc: Venkatesh Srinivas <venkateshs@chromium.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/lapic.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 80f92cbc4029..f77da92c6ea6 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2315,10 +2315,14 @@ static int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
>  		break;
>  
>  	case APIC_SELF_IPI:
> -		if (apic_x2apic_mode(apic))
> -			kvm_apic_send_ipi(apic, APIC_DEST_SELF | (val & APIC_VECTOR_MASK), 0);
> -		else
> +		/*
> +		 * Self-IPI exists only when x2APIC is enabled.  Bits 7:0 hold
> +		 * the vector, everything else is reserved.
> +		 */
> +		if (!apic_x2apic_mode(apic) || (val & ~APIC_VECTOR_MASK))
>  			ret = 1;
> +		else
> +			kvm_apic_send_ipi(apic, APIC_DEST_SELF | val, 0);
>  		break;
>  	default:
>  		ret = 1;

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

Re: [PATCH 2/6] KVM: x86: Inject #GP on x2APIC WRMSR that sets reserved bits 63:32

[Maxim Levitsky]

On Sat, 2023-01-07 at 01:10 +0000, Sean Christopherson wrote:
> Reject attempts to set bits 63:32 for 32-bit x2APIC registers, i.e. all
> x2APIC registers except ICR.  Per Intel's SDM:
> 
>   Non-zero writes (by WRMSR instruction) to reserved bits to these
>   registers will raise a general protection fault exception
> 
> Opportunistically fix a typo in a nearby comment.
> 
> Reported-by: Marc Orr <marcorr@google.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/lapic.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index f77da92c6ea6..bf53e4752f30 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -3108,13 +3108,17 @@ static int kvm_lapic_msr_read(struct kvm_lapic *apic, u32 reg, u64 *data)
>  static int kvm_lapic_msr_write(struct kvm_lapic *apic, u32 reg, u64 data)
>  {
>  	/*
> -	 * ICR is a 64-bit register in x2APIC mode (and Hyper'v PV vAPIC) and
> +	 * ICR is a 64-bit register in x2APIC mode (and Hyper-V PV vAPIC) and
>  	 * can be written as such, all other registers remain accessible only
>  	 * through 32-bit reads/writes.
>  	 */
>  	if (reg == APIC_ICR)
>  		return kvm_x2apic_icr_write(apic, data);
>  
> +	/* Bits 63:32 are reserved in all other registers. */
> +	if (data >> 32)
> +		return 1;
> +
>  	return kvm_lapic_reg_write(apic, reg, (u32)data);
>  }
>  
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

Re: [PATCH 3/6] KVM: x86: Mark x2APIC DFR reg as non-existent for x2APIC

[Maxim Levitsky]

On Sat, 2023-01-07 at 01:10 +0000, Sean Christopherson wrote:
> Mark APIC_DFR as being invalid/non-existent in x2APIC mode instead of
> handling it as a one-off check in kvm_x2apic_msr_read().  This will allow
> reusing "valid_reg_mask" to generate VMX's interception bitmaps for
> x2APIC.  Handling DFR in the common read path may also fix the Hyper-V
> PV MSR interface, if that can coexist with x2APIC.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/lapic.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index bf53e4752f30..c49b13418638 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1541,7 +1541,6 @@ static int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
>  		APIC_REG_MASK(APIC_TASKPRI) |
>  		APIC_REG_MASK(APIC_PROCPRI) |
>  		APIC_REG_MASK(APIC_LDR) |
> -		APIC_REG_MASK(APIC_DFR) |
>  		APIC_REG_MASK(APIC_SPIV) |
>  		APIC_REGS_MASK(APIC_ISR, APIC_ISR_NR) |
>  		APIC_REGS_MASK(APIC_TMR, APIC_ISR_NR) |
> @@ -1562,12 +1561,13 @@ static int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
>  		valid_reg_mask |= APIC_REG_MASK(APIC_LVTCMCI);
>  
>  	/*
> -	 * ARBPRI and ICR2 are not valid in x2APIC mode.  WARN if KVM reads ICR
> -	 * in x2APIC mode as it's an 8-byte register in x2APIC and needs to be
> -	 * manually handled by the caller.
> +	 * ARBPRI, DFR, and ICR2 are not valid in x2APIC mode.  WARN if KVM
> +	 * reads ICR in x2APIC mode as it's an 8-byte register in x2APIC and
> +	 * needs to be manually handled by the caller.
>  	 */
>  	if (!apic_x2apic_mode(apic))
>  		valid_reg_mask |= APIC_REG_MASK(APIC_ARBPRI) |
> +				  APIC_REG_MASK(APIC_DFR) |
>  				  APIC_REG_MASK(APIC_ICR2);
>  	else
>  		WARN_ON_ONCE(offset == APIC_ICR);
> @@ -3141,9 +3141,6 @@ int kvm_x2apic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
>  	if (!lapic_in_kernel(vcpu) || !apic_x2apic_mode(apic))
>  		return 1;
>  
> -	if (reg == APIC_DFR)
> -		return 1;
> -
>  	return kvm_lapic_msr_read(apic, reg, data);
>  }
>  


Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
Best regards,
	Maxim Levitsky

Re: [PATCH 4/6] KVM: x86: Split out logic to generate "readable" APIC regs mask to helper

[Maxim Levitsky]

On Sat, 2023-01-07 at 01:10 +0000, Sean Christopherson wrote:
> Move the generation of the readable APIC regs bitmask to a standalone
> helper so that VMX can use the mask for its MSR interception bitmaps.
> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/lapic.c | 34 +++++++++++++++++++++-------------
>  arch/x86/kvm/lapic.h |  2 ++
>  2 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index c49b13418638..19697fe9b2c7 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1529,12 +1529,9 @@ static inline struct kvm_lapic *to_lapic(struct kvm_io_device *dev)
>  #define APIC_REGS_MASK(first, count) \
>  	(APIC_REG_MASK(first) * ((1ull << (count)) - 1))
>  
> -static int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
> -			      void *data)
> +u64 kvm_lapic_readable_reg_mask(struct kvm_lapic *apic)
>  {
> -	unsigned char alignment = offset & 0xf;
> -	u32 result;
> -	/* this bitmask has a bit cleared for each reserved register */
> +	/* Leave bits '0' for reserved and write-only registers. */
>  	u64 valid_reg_mask =
>  		APIC_REG_MASK(APIC_ID) |
>  		APIC_REG_MASK(APIC_LVR) |
> @@ -1560,22 +1557,33 @@ static int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
>  	if (kvm_lapic_lvt_supported(apic, LVT_CMCI))
>  		valid_reg_mask |= APIC_REG_MASK(APIC_LVTCMCI);
>  
> -	/*
> -	 * ARBPRI, DFR, and ICR2 are not valid in x2APIC mode.  WARN if KVM
> -	 * reads ICR in x2APIC mode as it's an 8-byte register in x2APIC and
> -	 * needs to be manually handled by the caller.
> -	 */
> +	/* ARBPRI, DFR, and ICR2 are not valid in x2APIC mode. */
>  	if (!apic_x2apic_mode(apic))
>  		valid_reg_mask |= APIC_REG_MASK(APIC_ARBPRI) |
>  				  APIC_REG_MASK(APIC_DFR) |
>  				  APIC_REG_MASK(APIC_ICR2);
> -	else
> -		WARN_ON_ONCE(offset == APIC_ICR);
> +
> +	return valid_reg_mask;
> +}
> +EXPORT_SYMBOL_GPL(kvm_lapic_readable_reg_mask);
> +
> +static int kvm_lapic_reg_read(struct kvm_lapic *apic, u32 offset, int len,
> +			      void *data)
> +{
> +	unsigned char alignment = offset & 0xf;
> +	u32 result;
> +
> +	/*
> +	 * WARN if KVM reads ICR in x2APIC mode, as it's an 8-byte register in
> +	 * x2APIC and needs to be manually handled by the caller.
> +	 */
> +	WARN_ON_ONCE(apic_x2apic_mode(apic) && offset == APIC_ICR);
>  
>  	if (alignment + len > 4)
>  		return 1;
>  
> -	if (offset > 0x3f0 || !(valid_reg_mask & APIC_REG_MASK(offset)))
> +	if (offset > 0x3f0 ||
> +	    !(kvm_lapic_readable_reg_mask(apic) & APIC_REG_MASK(offset)))
>  		return 1;
>  
>  	result = __apic_read(apic, offset & ~0xf);
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index df316ede7546..0a0ea4b5dd8c 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -146,6 +146,8 @@ int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
>  int kvm_lapic_set_pv_eoi(struct kvm_vcpu *vcpu, u64 data, unsigned long len);
>  void kvm_lapic_exit(void);
>  
> +u64 kvm_lapic_readable_reg_mask(struct kvm_lapic *apic);
> +
>  #define VEC_POS(v) ((v) & (32 - 1))
>  #define REG_POS(v) (((v) >> 5) << 4)
>  

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

Re: [PATCH 5/6] KVM: VMX: Always intercept accesses to unsupported "extended" x2APIC regs

[Maxim Levitsky]

On Sat, 2023-01-07 at 01:10 +0000, Sean Christopherson wrote:
> Don't clear the "read" bits for x2APIC registers above SELF_IPI (APIC regs
> 0x400 - 0xff0, MSRs 0x840 - 0x8ff).  KVM doesn't emulate registers in that
> space (there are a smattering of AMD-only extensions) and so should
> intercept reads in order to inject #GP.  When APICv is fully enabled,
> Intel hardware doesn't validate the registers on RDMSR and instead blindly
> retrieves data from the vAPIC page, i.e. it's software's responsibility to
> intercept reads to non-existent MSRs.
> 
> Fixes: 8d14695f9542 ("x86, apicv: add virtual x2apic support")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index c788aa382611..82c61c16f8f5 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4018,26 +4018,17 @@ void vmx_enable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type)
>  		vmx_set_msr_bitmap_write(msr_bitmap, msr);
>  }
>  
> -static void vmx_reset_x2apic_msrs(struct kvm_vcpu *vcpu, u8 mode)
> -{
> -	unsigned long *msr_bitmap = to_vmx(vcpu)->vmcs01.msr_bitmap;
> -	unsigned long read_intercept;
> -	int msr;
> -
> -	read_intercept = (mode & MSR_BITMAP_MODE_X2APIC_APICV) ? 0 : ~0;
> -
> -	for (msr = 0x800; msr <= 0x8ff; msr += BITS_PER_LONG) {
> -		unsigned int read_idx = msr / BITS_PER_LONG;
> -		unsigned int write_idx = read_idx + (0x800 / sizeof(long));
> -
> -		msr_bitmap[read_idx] = read_intercept;
> -		msr_bitmap[write_idx] = ~0ul;
> -	}
> -}
> -
>  static void vmx_update_msr_bitmap_x2apic(struct kvm_vcpu *vcpu)
>  {
> +	/*
> +	 * x2APIC indices for 64-bit accesses into the RDMSR and WRMSR halves
> +	 * of the MSR bitmap.  KVM emulates APIC registers up through 0x3f0,
> +	 * i.e. MSR 0x83f, and so only needs to dynamically manipulate 64 bits.
> +	 */
The above comment is better to be placed down below, near the actual write,
otherwise it is confusing.

> +	const int read_idx = APIC_BASE_MSR / BITS_PER_LONG_LONG;
> +	const int write_idx = read_idx + (0x800 / sizeof(u64));
>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +	u64 *msr_bitmap = (u64 *)vmx->vmcs01.msr_bitmap;
>  	u8 mode;
>  
>  	if (!cpu_has_vmx_msr_bitmap())
> @@ -4058,7 +4049,18 @@ static void vmx_update_msr_bitmap_x2apic(struct kvm_vcpu *vcpu)
>  
>  	vmx->x2apic_msr_bitmap_mode = mode;
>  
> -	vmx_reset_x2apic_msrs(vcpu, mode);
> +	/*
> +	 * Reset the bitmap for MSRs 0x800 - 0x83f.  Leave AMD's uber-extended
> +	 * registers (0x840 and above) intercepted, KVM doesn't support them.

I don't think AMD calls them uber-extended. Just extended.

From a quick glance, these could have beeing very useful for VFIO passthrough of INT-X interrupts,
removing the need to mask the interrupt on per PCI device basis - instead you can just leave
the IRQ pending in ISR, while using SEOI and IER to ignore this pending bit for host.

I understand that the days of INT-X are long gone (and especially days of shared IRQ lines...)
and every sane device uses MSI/-X instead, but still.


> +	 * Intercept all writes by default and poke holes as needed.  Pass
> +	 * through all reads by default in x2APIC+APICv mode, as all registers
> +	 * except the current timer count are passed through for read.
> +	 */
> +	if (mode & MSR_BITMAP_MODE_X2APIC_APICV)
> +		msr_bitmap[read_idx] = 0;
> +	else
> +		msr_bitmap[read_idx] = ~0ull;
> +	msr_bitmap[write_idx] = ~0ull;
>  
>  	/*
>  	 * TPR reads and writes can be virtualized even if virtual interrupt

Other than the note about the comment,

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>


Best regards,
	Maxim Levitsky

Re: [PATCH 6/6] KVM: VMX: Intercept reads to invalid and write-only x2APIC registers

[Maxim Levitsky]

On Sat, 2023-01-07 at 01:10 +0000, Sean Christopherson wrote:
> Intercept reads to invalid (non-existent) and write-only x2APIC registers
> when configuring VMX's MSR bitmaps for x2APIC+APICv.  When APICv is fully
> enabled, Intel hardware doesn't validate the registers on RDMSR and
> instead blindly retrieves data from the vAPIC page, i.e. it's software's
> responsibility to intercept reads to non-existent and write-only MSRs.
> 
> Fixes: 8d14695f9542 ("x86, apicv: add virtual x2apic support")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 82c61c16f8f5..1be2bc7185be 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4031,7 +4031,7 @@ static void vmx_update_msr_bitmap_x2apic(struct kvm_vcpu *vcpu)
>  	u64 *msr_bitmap = (u64 *)vmx->vmcs01.msr_bitmap;
>  	u8 mode;
>  
> -	if (!cpu_has_vmx_msr_bitmap())
> +	if (!cpu_has_vmx_msr_bitmap() || WARN_ON_ONCE(!lapic_in_kernel(vcpu)))
>  		return;
>  
>  	if (cpu_has_secondary_exec_ctrls() &&
> @@ -4053,11 +4053,11 @@ static void vmx_update_msr_bitmap_x2apic(struct kvm_vcpu *vcpu)
>  	 * Reset the bitmap for MSRs 0x800 - 0x83f.  Leave AMD's uber-extended
>  	 * registers (0x840 and above) intercepted, KVM doesn't support them.
>  	 * Intercept all writes by default and poke holes as needed.  Pass
> -	 * through all reads by default in x2APIC+APICv mode, as all registers
> -	 * except the current timer count are passed through for read.
> +	 * through reads for all valid registers by default in x2APIC+APICv
> +	 * mode, only the current timer count needs on-demand emulation by KVM.
>  	 */
>  	if (mode & MSR_BITMAP_MODE_X2APIC_APICV)
> -		msr_bitmap[read_idx] = 0;
> +		msr_bitmap[read_idx] = ~kvm_lapic_readable_reg_mask(vcpu->arch.apic);
>  	else
>  		msr_bitmap[read_idx] = ~0ull;
>  	msr_bitmap[write_idx] = ~0ull;

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

Re: [PATCH 5/6] KVM: VMX: Always intercept accesses to unsupported "extended" x2APIC regs

[Sean Christopherson]

On Sun, Jan 08, 2023, Maxim Levitsky wrote:
> On Sat, 2023-01-07 at 01:10 +0000, Sean Christopherson wrote:
> >  static void vmx_update_msr_bitmap_x2apic(struct kvm_vcpu *vcpu)
> >  {
> > +	/*
> > +	 * x2APIC indices for 64-bit accesses into the RDMSR and WRMSR halves
> > +	 * of the MSR bitmap.  KVM emulates APIC registers up through 0x3f0,
> > +	 * i.e. MSR 0x83f, and so only needs to dynamically manipulate 64 bits.
> > +	 */
> The above comment is better to be placed down below, near the actual write,
> otherwise it is confusing.

Can you elaborate on why it's confusing?  The intent of this specific comment is
to capture why the index calculations use BITS_PER_LONG_LONG and sizeof(u64).

> > +	const int read_idx = APIC_BASE_MSR / BITS_PER_LONG_LONG;
> > +	const int write_idx = read_idx + (0x800 / sizeof(u64));
> >  	struct vcpu_vmx *vmx = to_vmx(vcpu);
> > +	u64 *msr_bitmap = (u64 *)vmx->vmcs01.msr_bitmap;
> >  	u8 mode;
> >  
> >  	if (!cpu_has_vmx_msr_bitmap())
> > @@ -4058,7 +4049,18 @@ static void vmx_update_msr_bitmap_x2apic(struct kvm_vcpu *vcpu)
> >  
> >  	vmx->x2apic_msr_bitmap_mode = mode;
> >  
> > -	vmx_reset_x2apic_msrs(vcpu, mode);
> > +	/*
> > +	 * Reset the bitmap for MSRs 0x800 - 0x83f.  Leave AMD's uber-extended
> > +	 * registers (0x840 and above) intercepted, KVM doesn't support them.
> 
> I don't think AMD calls them uber-extended. Just extended.

Yeah, I took some creative liberaties.  I want to avoid confusion with the more
common use of Extended APIC (x2APIC).

Re: [PATCH 5/6] KVM: VMX: Always intercept accesses to unsupported "extended" x2APIC regs

[Jim Mattson]

On Fri, Jan 6, 2023 at 5:10 PM Sean Christopherson <seanjc@google.com> wrote:
>
> Don't clear the "read" bits for x2APIC registers above SELF_IPI (APIC regs

Odd use of quotation marks in the shortlog  and here.

> 0x400 - 0xff0, MSRs 0x840 - 0x8ff).  KVM doesn't emulate registers in that
> space (there are a smattering of AMD-only extensions) and so should
> intercept reads in order to inject #GP.  When APICv is fully enabled,
> Intel hardware doesn't validate the registers on RDMSR and instead blindly
> retrieves data from the vAPIC page, i.e. it's software's responsibility to
> intercept reads to non-existent MSRs.
>
> Fixes: 8d14695f9542 ("x86, apicv: add virtual x2apic support")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>

Re: [PATCH 0/6] KVM: x86: x2APIC reserved bits/regs fixes

[Paolo Bonzini]

On 1/7/23 02:10, Sean Christopherson wrote:
> Fixes for edge cases where KVM mishandles reserved bits/regs checks when
> the vCPU is in x2APIC mode.
> 
> The first two patches were previously posted[*], but both patches were
> broken (as posted against upstream), hence I took full credit for doing
> the work and changed Marc to a reporter.
> 
> The VMX APICv fixes are for bugs found when writing tests.  *sigh*
> I didn't Cc those to stable as the odds of breaking something when touching
> the MSR bitmaps seemed higher than someone caring about a 10 year old bug.
> 
> AMD x2AVIC support may or may not suffer similar interception bugs, but I
> don't have hardware to test and this already snowballed further than
> expected...
> 
> [*] https://lore.kernel.org/kvm/20220525173933.1611076-1-venkateshs@chromium.org

Looks good; please feel free to start gathering this in your tree for 6.3.

Next week I'll go through Ben's series as well as Aaron's "Clean up the 
supported xfeatures" and others.

Let me know if you would like me to queue anything of these instead, and 
please remember to set up the tree in linux-next. :)

Thanks,

Paolo

> Sean Christopherson (6):
>    KVM: x86: Inject #GP if WRMSR sets reserved bits in APIC Self-IPI
>    KVM: x86: Inject #GP on x2APIC WRMSR that sets reserved bits 63:32
>    KVM: x86: Mark x2APIC DFR reg as non-existent for x2APIC
>    KVM: x86: Split out logic to generate "readable" APIC regs mask to
>      helper
>    KVM: VMX: Always intercept accesses to unsupported "extended" x2APIC
>      regs
>    KVM: VMX: Intercept reads to invalid and write-only x2APIC registers
> 
>   arch/x86/kvm/lapic.c   | 55 ++++++++++++++++++++++++++----------------
>   arch/x86/kvm/lapic.h   |  2 ++
>   arch/x86/kvm/vmx/vmx.c | 40 +++++++++++++++---------------
>   3 files changed, 57 insertions(+), 40 deletions(-)
> 
> 
> base-commit: 91dc252b0dbb6879e4067f614df1e397fec532a1

Re: [PATCH 0/6] KVM: x86: x2APIC reserved bits/regs fixes

[Sean Christopherson]

On Fri, Jan 13, 2023, Paolo Bonzini wrote:
> On 1/7/23 02:10, Sean Christopherson wrote:
> > Fixes for edge cases where KVM mishandles reserved bits/regs checks when
> > the vCPU is in x2APIC mode.
> > 
> > The first two patches were previously posted[*], but both patches were
> > broken (as posted against upstream), hence I took full credit for doing
> > the work and changed Marc to a reporter.
> > 
> > The VMX APICv fixes are for bugs found when writing tests.  *sigh*
> > I didn't Cc those to stable as the odds of breaking something when touching
> > the MSR bitmaps seemed higher than someone caring about a 10 year old bug.
> > 
> > AMD x2AVIC support may or may not suffer similar interception bugs, but I
> > don't have hardware to test and this already snowballed further than
> > expected...
> > 
> > [*] https://lore.kernel.org/kvm/20220525173933.1611076-1-venkateshs@chromium.org
> 
> Looks good; please feel free to start gathering this in your tree for 6.3.

Thanks!

> Next week I'll go through Ben's series as well as Aaron's "Clean up the
> supported xfeatures" and others.
> 
> Let me know if you would like me to queue anything of these instead, and
> please remember to set up the tree in linux-next. :)

Ya, next week is going to be dedicated to sorting out maintenance mechanics.

Re: [PATCH 0/6] KVM: x86: x2APIC reserved bits/regs fixes

[Sean Christopherson]

On Sat, 07 Jan 2023 01:10:19 +0000, Sean Christopherson wrote:
> Fixes for edge cases where KVM mishandles reserved bits/regs checks when
> the vCPU is in x2APIC mode.
> 
> The first two patches were previously posted[*], but both patches were
> broken (as posted against upstream), hence I took full credit for doing
> the work and changed Marc to a reporter.
> 
> [...]

Applied to kvm-x86 apic, thanks past me!

[1/6] KVM: x86: Inject #GP if WRMSR sets reserved bits in APIC Self-IPI
      https://github.com/kvm-x86/linux/commit/aeee623ea411
[2/6] KVM: x86: Inject #GP on x2APIC WRMSR that sets reserved bits 63:32
      https://github.com/kvm-x86/linux/commit/a927a2508121
[3/6] KVM: x86: Mark x2APIC DFR reg as non-existent for x2APIC
      https://github.com/kvm-x86/linux/commit/6d4719e1b5a2
[4/6] KVM: x86: Split out logic to generate "readable" APIC regs mask to helper
      https://github.com/kvm-x86/linux/commit/1088d5e5cf70
[5/6] KVM: VMX: Always intercept accesses to unsupported "extended" x2APIC regs
      https://github.com/kvm-x86/linux/commit/cbb3f75487a9
[6/6] KVM: VMX: Intercept reads to invalid and write-only x2APIC registers
      https://github.com/kvm-x86/linux/commit/7b205379c53d

--
https://github.com/kvm-x86/linux/tree/next
https://github.com/kvm-x86/linux/tree/fixes