Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210823143028.649818-1-vkuznets@redhat.com> <20210823143028.649818-5-vkuznets@redhat.com>
 <20210823185841.ov7ejn2thwebcwqk@habkost.net> <87mtp7jowv.fsf@vitty.brq.redhat.com>
 <CAOpTY_ot8teH5x5vVS2HvuMx5LSKLPtyen_ZUM1p7ncci4LFbA@mail.gmail.com>
 <87k0kakip9.fsf@vitty.brq.redhat.com> <2df0b6d18115fb7f2701587b7937d8ddae38e36a.camel@redhat.com>
In-Reply-To: <2df0b6d18115fb7f2701587b7937d8ddae38e36a.camel@redhat.com>
From:   Nitesh Lal <nilal@redhat.com>
Date:   Mon, 30 Aug 2021 15:47:10 -0400
Message-ID: <CAFki+L=1UUbNVn21rh34FMk3sajb=6rUpsqSdrK82FyD+UKcTQ@mail.gmail.com>
Subject: Re: [PATCH v2 4/4] KVM: x86: Fix stack-out-of-bounds memory access
 from ioapic_write_indirect()
To:     Maxim Levitsky <mlevitsk@redhat.com>
Cc:     Vitaly Kuznetsov <vkuznets@redhat.com>,
        Eduardo Habkost <ehabkost@redhat.com>, 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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Aug 24, 2021 at 12:08 PM Maxim Levitsky <mlevitsk@redhat.com> wrote:
>
> On Tue, 2021-08-24 at 16:42 +0200, Vitaly Kuznetsov wrote:
> > 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]
> > > > > >

[...]

>
>
> I also don't like that ioapic_write_indirect calls the kvm_bitmap_or_dest_vcpus twice,
> and second time with 'old_dest_id'
>
> I am not 100%  sure why old_dest_id/old_dest_mode are needed as I don't see anything in the
> function changing them.
> I think only the guest can change them, so maybe the code deals with the guest changing them
> while the code is running from a different vcpu?
>
> The commit that introduced this code is 7ee30bc132c683d06a6d9e360e39e483e3990708
> Nitesh Narayan Lal, maybe you remember something about it?
>

Apologies for the delay in responding, I just got back from my PTO and
still clearing my inbox. Since you have reviewed this patch the only open
question is the above so I will try to answer that. Please let me know
in case I missed anything.

IIRC IOAPIC can be reconfigured while the previous interrupt is pending or
still processing. In this situation, ioapic_handeld_vectors may go out of
sync as it only records the recently passed configuration. Since with this
commit, we stopped generating requests for all vCPUs we need this chunk of
code to keep ioapic_handled_vectors in sync.

Having said that perhaps there could be a better way of handling this (?).

-- 
Thanks
Nitesh