[PATCH v2 1/2] KVM: Inject #GP on invalid write to APIC_SELF_IPI register

[PATCH v2 1/2] KVM: Inject #GP on invalid write to APIC_SELF_IPI register

[Venkatesh Srinivas]

From: Marc Orr <marcorr@google.com>

From: Venkatesh Srinivas <venkateshs@chromium.org>

The upper bytes of the x2APIC APIC_SELF_IPI register are reserved.
Inject a #GP into the guest if any of these reserved bits are set.

Signed-off-by: Marc Orr <marcorr@google.com>
Signed-off-by: Venkatesh Srinivas <venkateshs@chromium.org>
---
 arch/x86/kvm/lapic.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 21ab69db689b..6f8522e8c492 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2169,10 +2169,16 @@ 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;
+			break;
+		}
+		kvm_lapic_reg_write(apic, APIC_ICR,
+				    APIC_DEST_SELF | (val & APIC_VECTOR_MASK));
 		break;
 	default:
 		ret = 1;
-- 
2.36.1.124.g0e6072fb45-goog

[PATCH v2 2/2] KVM: Inject #GP on invalid writes to x2APIC registers

[Venkatesh Srinivas]

From: Marc Orr <marcorr@google.com>

From: Venkatesh Srinivas <venkateshs@chromium.org>

The upper bytes of any x2APIC register are reserved. Inject a #GP
into the guest if any of these reserved bits are set.

Signed-off-by: Marc Orr <marcorr@google.com>
Signed-off-by: Venkatesh Srinivas <venkateshs@chromium.org>
---
 arch/x86/kvm/lapic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 6f8522e8c492..617e4936c5cc 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2907,6 +2907,8 @@ int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 
 	if (!lapic_in_kernel(vcpu) || !apic_x2apic_mode(apic))
 		return 1;
+	else if (data >> 32)
+		return 1;
 
 	return kvm_lapic_msr_write(apic, reg, data);
 }
-- 
2.36.1.124.g0e6072fb45-goog

Re: [PATCH v2 2/2] KVM: Inject #GP on invalid writes to x2APIC registers

[Sean Christopherson]

On Wed, May 25, 2022, Venkatesh Srinivas wrote:
> From: Marc Orr <marcorr@google.com>
> 
> From: Venkatesh Srinivas <venkateshs@chromium.org>

Heh, something is wonky in your setup, only Marc should get an explicit From:

> The upper bytes of any x2APIC register are reserved. Inject a #GP
> into the guest if any of these reserved bits are set.
> 
> Signed-off-by: Marc Orr <marcorr@google.com>
> Signed-off-by: Venkatesh Srinivas <venkateshs@chromium.org>
> ---
>  arch/x86/kvm/lapic.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 6f8522e8c492..617e4936c5cc 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2907,6 +2907,8 @@ int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>  
>  	if (!lapic_in_kernel(vcpu) || !apic_x2apic_mode(apic))
>  		return 1;
> +	else if (data >> 32)
> +		return 1;

This is incorrect, ICR is a 64-bit value.  Marc's changelog for the internal patch
was wrong, although the code was correct.

The correct upstream change is:

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 39b805666a18..54d0f350acdf 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2892,6 +2892,9 @@ static int kvm_lapic_msr_write(struct kvm_lapic *apic, u32 reg, u64 data)
        if (reg == APIC_ICR)
                return kvm_x2apic_icr_write(apic, data);

+       if (data >> 32)
+               return 1;
+
        return kvm_lapic_reg_write(apic, reg, (u32)data);
 }

As penance for not testing, can you write a KUT testcase to hit all the registers?
You could use e.g. SIPI to test that a 64-bit ICR value is _not_ rejected.

One thought would be to base your testcase on top of similar changes to msr.c to
blast all of the MCE MSRs, e.g. add a path/helper to enable x2APIC and iterate over
all registers.

[*] https://lore.kernel.org/all/20220512233045.4125471-1-seanjc@google.com

Re: [PATCH v2 1/2] KVM: Inject #GP on invalid write to APIC_SELF_IPI register

[Sean Christopherson]

On Wed, May 25, 2022, Venkatesh Srinivas wrote:
> From: Marc Orr <marcorr@google.com>
> 
> From: Venkatesh Srinivas <venkateshs@chromium.org>

Only Marc should have an explicit From:.  git am processes only the first From:,
e.g. your From: line ends up in the changelog.

> The upper bytes of the x2APIC APIC_SELF_IPI register are reserved.

Uber nit, please be more precise than "upper bytes".  The comment about "Bits 7:0"
saved me from having to lookup up APIC_VECTOR_MASK, but it'd be nice to have that
in the changelog.

E.g.

  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.

> Inject a #GP into the guest if any of these reserved bits are set.
> 
> Signed-off-by: Marc Orr <marcorr@google.com>
> Signed-off-by: Venkatesh Srinivas <venkateshs@chromium.org>
> ---
>  arch/x86/kvm/lapic.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 21ab69db689b..6f8522e8c492 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2169,10 +2169,16 @@ 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;
> +			break;
> +		}
> +		kvm_lapic_reg_write(apic, APIC_ICR,
> +				    APIC_DEST_SELF | (val & APIC_VECTOR_MASK));
>  		break;
>  	default:
>  		ret = 1;
> -- 
> 2.36.1.124.g0e6072fb45-goog
> 

Re: [PATCH v2 1/2] KVM: Inject #GP on invalid write to APIC_SELF_IPI register

[Jim Mattson]

On Wed, May 25, 2022 at 10:40 AM Venkatesh Srinivas
<venkateshs@chromium.org> wrote:
>
> From: Marc Orr <marcorr@google.com>
>
> From: Venkatesh Srinivas <venkateshs@chromium.org>
>
> The upper bytes of the x2APIC APIC_SELF_IPI register are reserved.
> Inject a #GP into the guest if any of these reserved bits are set.
>
> Signed-off-by: Marc Orr <marcorr@google.com>
> Signed-off-by: Venkatesh Srinivas <venkateshs@chromium.org>
> ---
>  arch/x86/kvm/lapic.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 21ab69db689b..6f8522e8c492 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2169,10 +2169,16 @@ 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;
> +                       break;
> +               }
> +               kvm_lapic_reg_write(apic, APIC_ICR,
> +                                   APIC_DEST_SELF | (val & APIC_VECTOR_MASK));

Masking off the high bytes of 'val' is redundant here.

>                 break;
>         default:
>                 ret = 1;
> --
> 2.36.1.124.g0e6072fb45-goog
>