LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Vitaly Kuznetsov <vkuznets@redhat.com>
To: Eduardo Habkost <ehabkost@redhat.com>
Cc: kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
	Sean Christopherson <seanjc@google.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
	Nitesh Narayan Lal <nitesh@redhat.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 4/4] KVM: x86: Fix stack-out-of-bounds memory access from ioapic_write_indirect()
Date: Tue, 24 Aug 2021 16:42:26 +0200	[thread overview]
Message-ID: <87k0kakip9.fsf@vitty.brq.redhat.com> (raw)
In-Reply-To: <CAOpTY_ot8teH5x5vVS2HvuMx5LSKLPtyen_ZUM1p7ncci4LFbA@mail.gmail.com>

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Tue, Aug 24, 2021 at 3:13 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>>
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>>
>> > On Mon, Aug 23, 2021 at 04:30:28PM +0200, Vitaly Kuznetsov wrote:
>> >> KASAN reports the following issue:
>> >>
>> >>  BUG: KASAN: stack-out-of-bounds in kvm_make_vcpus_request_mask+0x174/0x440 [kvm]
>> >>  Read of size 8 at addr ffffc9001364f638 by task qemu-kvm/4798
>> >>
>> >>  CPU: 0 PID: 4798 Comm: qemu-kvm Tainted: G               X --------- ---
>> >>  Hardware name: AMD Corporation DAYTONA_X/DAYTONA_X, BIOS RYM0081C 07/13/2020
>> >>  Call Trace:
>> >>   dump_stack+0xa5/0xe6
>> >>   print_address_description.constprop.0+0x18/0x130
>> >>   ? kvm_make_vcpus_request_mask+0x174/0x440 [kvm]
>> >>   __kasan_report.cold+0x7f/0x114
>> >>   ? kvm_make_vcpus_request_mask+0x174/0x440 [kvm]
>> >>   kasan_report+0x38/0x50
>> >>   kasan_check_range+0xf5/0x1d0
>> >>   kvm_make_vcpus_request_mask+0x174/0x440 [kvm]
>> >>   kvm_make_scan_ioapic_request_mask+0x84/0xc0 [kvm]
>> >>   ? kvm_arch_exit+0x110/0x110 [kvm]
>> >>   ? sched_clock+0x5/0x10
>> >>   ioapic_write_indirect+0x59f/0x9e0 [kvm]
>> >>   ? static_obj+0xc0/0xc0
>> >>   ? __lock_acquired+0x1d2/0x8c0
>> >>   ? kvm_ioapic_eoi_inject_work+0x120/0x120 [kvm]
>> >>
>> >> The problem appears to be that 'vcpu_bitmap' is allocated as a single long
>> >> on stack and it should really be KVM_MAX_VCPUS long. We also seem to clear
>> >> the lower 16 bits of it with bitmap_zero() for no particular reason (my
>> >> guess would be that 'bitmap' and 'vcpu_bitmap' variables in
>> >> kvm_bitmap_or_dest_vcpus() caused the confusion: while the later is indeed
>> >> 16-bit long, the later should accommodate all possible vCPUs).
>> >>
>> >> Fixes: 7ee30bc132c6 ("KVM: x86: deliver KVM IOAPIC scan request to target vCPUs")
>> >> Fixes: 9a2ae9f6b6bb ("KVM: x86: Zero the IOAPIC scan request dest vCPUs bitmap")
>> >> Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> >> ---
>> >>  arch/x86/kvm/ioapic.c | 10 +++++-----
>> >>  1 file changed, 5 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c
>> >> index ff005fe738a4..92cd4b02e9ba 100644
>> >> --- a/arch/x86/kvm/ioapic.c
>> >> +++ b/arch/x86/kvm/ioapic.c
>> >> @@ -319,7 +319,7 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val)
>> >>      unsigned index;
>> >>      bool mask_before, mask_after;
>> >>      union kvm_ioapic_redirect_entry *e;
>> >> -    unsigned long vcpu_bitmap;
>> >> +    unsigned long vcpu_bitmap[BITS_TO_LONGS(KVM_MAX_VCPUS)];
>> >
>> > Is there a way to avoid this KVM_MAX_VCPUS-sized variable on the
>> > stack?  This might hit us back when we increase KVM_MAX_VCPUS to
>> > a few thousand VCPUs (I was planning to submit a patch for that
>> > soon).
>>
>> What's the short- or mid-term target?
>
> Short term target is 2048 (which was already tested). Mid-term target
> (not tested yet) is 4096, maybe 8192.
>
>>
>> Note, we're allocating KVM_MAX_VCPUS bits (not bytes!) here, this means
>> that for e.g. 2048 vCPUs we need 256 bytes of the stack only. In case
>> the target much higher than that, we will need to either switch to
>> dynamic allocation or e.g. use pre-allocated per-CPU variables and make
>> this a preempt-disabled region. I, however, would like to understand if
>> the problem with allocating this from stack is real or not first.
>
> Is 256 bytes too much here, or would that be OK?
>

AFAIR, on x86_64 stack size (both reqular and irq) is 16k, eating 256
bytes of it is probably OK. I'd start worrying when we go to 1024 (8k
vCPUs) and above (but this is subjective of course).

-- 
Vitaly


  reply	other threads:[~2021-08-24 14:42 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-23 14:30 [PATCH v2 0/4] KVM: Various fixes and improvements around kicking vCPUs Vitaly Kuznetsov
2021-08-23 14:30 ` [PATCH v2 1/4] KVM: Clean up benign vcpu->cpu data races when " Vitaly Kuznetsov
2021-08-23 14:30 ` [PATCH v2 2/4] KVM: Guard cpusmask NULL check with CONFIG_CPUMASK_OFFSTACK Vitaly Kuznetsov
2021-08-23 14:30 ` [PATCH v2 3/4] KVM: Optimize kvm_make_vcpus_request_mask() a bit Vitaly Kuznetsov
2021-08-23 14:30 ` [PATCH v2 4/4] KVM: x86: Fix stack-out-of-bounds memory access from ioapic_write_indirect() Vitaly Kuznetsov
2021-08-23 18:58   ` Eduardo Habkost
2021-08-24  7:13     ` Vitaly Kuznetsov
2021-08-24 14:23       ` Eduardo Habkost
2021-08-24 14:42         ` Vitaly Kuznetsov [this message]
2021-08-24 16:07           ` Maxim Levitsky
2021-08-24 17:40             ` Sean Christopherson
2021-08-25  8:26               ` Vitaly Kuznetsov
2021-08-25  8:21             ` Vitaly Kuznetsov
2021-08-25  9:11               ` Maxim Levitsky
2021-08-25  9:43                 ` Vitaly Kuznetsov
2021-08-25 10:41                   ` Maxim Levitsky
2021-08-25 13:19                   ` Eduardo Habkost
2021-08-26 12:40                     ` Vitaly Kuznetsov
2021-08-26 14:52                       ` Eduardo Habkost
2021-08-26 18:01                         ` Sean Christopherson
2021-08-26 18:13                           ` Eduardo Habkost
2021-08-26 19:27             ` Eduardo Habkost
2021-08-30 19:47             ` Nitesh Lal

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=87k0kakip9.fsf@vitty.brq.redhat.com \
    --to=vkuznets@redhat.com \
    --cc=dgilbert@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=jmattson@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nitesh@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=seanjc@google.com \
    --cc=wanpengli@tencent.com \
    --subject='Re: [PATCH v2 4/4] KVM: x86: Fix stack-out-of-bounds memory access from ioapic_write_indirect()' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).