All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Hyunwoo Kim <imv4bel@gmail.com>,
	pbonzini@redhat.com, tglx@kernel.org,  mingo@redhat.com,
	bp@alien8.de, dave.hansen@linux.intel.com, x86@kernel.org,
	 hpa@zytor.com, kvm@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH] KVM: x86: hyper-v: Bound the bank index in hv_is_vp_in_sparse_set()
Date: Mon, 8 Jun 2026 09:17:33 -0700	[thread overview]
Message-ID: <aibrHTY54o9ygVt6@google.com> (raw)
In-Reply-To: <87o6hlhuz5.fsf@redhat.com>

On Mon, Jun 08, 2026, Vitaly Kuznetsov wrote:
> Hyunwoo Kim <imv4bel@gmail.com> writes:
> 
> > hv_is_vp_in_sparse_set() uses valid_bit_nr, i.e. vp_id divided by
> > HV_VCPUS_PER_SPARSE_BANK, as the test_bit() index into
> > valid_bank_mask. valid_bank_mask is a single u64 and a sparse vCPU
> > set holds at most HV_MAX_SPARSE_VCPU_BANKS banks, so valid_bit_nr
> > must be less than HV_MAX_SPARSE_VCPU_BANKS.
> >
> > The caller in kvm_hv_send_ipi_to_many() passes kvm_hv_get_vpindex(),
> > which is below KVM_MAX_VCPUS and therefore always within that bound.
> > The L2 direct flush branch in kvm_hv_flush_tlb(), however, passes
> > hv_v->nested.vp_id, copied verbatim from the enlightened VMCS
> > without any bounds check, so valid_bit_nr can reach
> > HV_MAX_SPARSE_VCPU_BANKS or more and test_bit() then reads beyond
> > valid_bank_mask.
> >
> > Return false before the test_bit() when valid_bit_nr is not below
> > HV_MAX_SPARSE_VCPU_BANKS, since such a VP cannot be present in the
> > set.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: c58a318f6090 ("KVM: x86: hyper-v: L2 TLB flush")
> > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> > ---
> >  arch/x86/kvm/hyperv.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> > index 4438ecac9a89..d8782cb7ba02 100644
> > --- a/arch/x86/kvm/hyperv.c
> > +++ b/arch/x86/kvm/hyperv.c
> > @@ -1839,6 +1839,10 @@ static bool hv_is_vp_in_sparse_set(u32 vp_id, u64 valid_bank_mask, u64 sparse_ba
> >  	int valid_bit_nr = vp_id / HV_VCPUS_PER_SPARSE_BANK;
> >  	unsigned long sbank;
> >  
> > +	/* A bank index beyond the mask can't be set, the VP isn't in the set. */
> > +	if (valid_bit_nr >= HV_MAX_SPARSE_VCPU_BANKS)
> > +		return false;
> > +
> >  	if (!test_bit(valid_bit_nr, (unsigned long *)&valid_bank_mask))
> >  		return false;
> 
> I think the concern is valid, so

Yeah, easy to trigger with KASAN and:

diff --git tools/testing/selftests/kvm/x86/hyperv_evmcs.c tools/testing/selftests/kvm/x86/hyperv_evmcs.c
index c7fa114aee20..0cf5f891a20d 100644
--- tools/testing/selftests/kvm/x86/hyperv_evmcs.c
+++ tools/testing/selftests/kvm/x86/hyperv_evmcs.c
@@ -59,6 +59,10 @@ void l2_guest_code(void)
        vmcall();
        rdmsr_from_l2(MSR_GS_BASE); /* intercepted */
 
+       asm volatile ("movq %0, %%xmm0" :: "r"(-1ull));
+       hyperv_hypercall(HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE | HV_HYPERCALL_FAST_BIT, 0x0,
+                        HV_FLUSH_ALL_VIRTUAL_ADDRESS_SPACES);
+
        /* L2 TLB flush tests */
        hyperv_hypercall(HVCALL_FLUSH_VIRTUAL_ADDRESS_SPACE | HV_HYPERCALL_FAST_BIT, 0x0,
                         HV_FLUSH_ALL_VIRTUAL_ADDRESS_SPACES | HV_FLUSH_ALL_PROCESSORS);
@@ -117,7 +121,7 @@ void guest_code(struct vmx_pages *vmx_pages, struct hyperv_test_pages *hv_pages,
        current_evmcs->partition_assist_page = hv_pages->partition_assist_gpa;
        current_evmcs->hv_enlightenments_control.nested_flush_hypercall = 1;
        current_evmcs->hv_vm_id = 1;
-       current_evmcs->hv_vp_id = 1;
+       current_evmcs->hv_vp_id = -1;
        current_vp_assist->nested_control.features.directhypercall = 1;
        *(u32 *)(hv_pages->partition_assist) = 0;
 


> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> 
> what I'm not sure about if we should also deliberately crash the VM
> which does such a hypercall. This way it would be easier to find buggy
> L1s but given that they are most likely Windows, we need to do some
> tests to see if this is not actually happening today (e.g. Hyper-V usign
> VP_ID or '-1' for something). Let's have this as a future TODO item.

+1

      reply	other threads:[~2026-06-08 16:17 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-06 14:44 [PATCH] KVM: x86: hyper-v: Bound the bank index in hv_is_vp_in_sparse_set() Hyunwoo Kim
2026-06-08 16:12 ` Vitaly Kuznetsov
2026-06-08 16:17   ` Sean Christopherson [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aibrHTY54o9ygVt6@google.com \
    --to=seanjc@google.com \
    --cc=bp@alien8.de \
    --cc=dave.hansen@linux.intel.com \
    --cc=hpa@zytor.com \
    --cc=imv4bel@gmail.com \
    --cc=kvm@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=stable@vger.kernel.org \
    --cc=tglx@kernel.org \
    --cc=vkuznets@redhat.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.