| Message ID | 1583951685-202743-1-git-send-email-nitesh@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] KVM: x86: Initializing all kvm_lapic_irq fields | expand |
On 11/03/20 19:34, Nitesh Narayan Lal wrote: > Previously all fields of structure kvm_lapic_irq were not initialized > before it was passed to kvm_bitmap_or_dest_vcpus(). Which will cause > an issue when any of those fields are used for processing a request. > This patch initializes all the fields of kvm_lapic_irq based on the > values which are passed through the ioapic redirect_entry object. Can you explain better how the bug manifests itself? Thanks, Paolo > Fixes: 7ee30bc132c6("KVM: x86: deliver KVM IOAPIC scan request to target vCPUs") > Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com> > --- > arch/x86/kvm/ioapic.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c > index 7668fed..3a8467d 100644 > --- a/arch/x86/kvm/ioapic.c > +++ b/arch/x86/kvm/ioapic.c > @@ -378,12 +378,15 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val) > if (e->fields.delivery_mode == APIC_DM_FIXED) { > struct kvm_lapic_irq irq; > > - irq.shorthand = APIC_DEST_NOSHORT; > irq.vector = e->fields.vector; > irq.delivery_mode = e->fields.delivery_mode << 8; > - irq.dest_id = e->fields.dest_id; > irq.dest_mode = > kvm_lapic_irq_dest_mode(!!e->fields.dest_mode); > + irq.level = 1; > + irq.trig_mode = e->fields.trig_mode; > + irq.shorthand = APIC_DEST_NOSHORT; > + irq.dest_id = e->fields.dest_id; > + irq.msi_redir_hint = false; > bitmap_zero(&vcpu_bitmap, 16); > kvm_bitmap_or_dest_vcpus(ioapic->kvm, &irq, > &vcpu_bitmap); >
On 3/11/20 2:49 PM, Paolo Bonzini wrote: > On 11/03/20 19:34, Nitesh Narayan Lal wrote: >> Previously all fields of structure kvm_lapic_irq were not initialized >> before it was passed to kvm_bitmap_or_dest_vcpus(). Which will cause >> an issue when any of those fields are used for processing a request. >> This patch initializes all the fields of kvm_lapic_irq based on the >> values which are passed through the ioapic redirect_entry object. > Can you explain better how the bug manifests itself? For example not initializing the irq.msi_redir_hint field, could lead to a situation where it carries garbage (non-zero) value. This will lead to misbehavior of kvm_apic_map_get_dest_lapic() when it invokes the kvm_lowest_prio_delivery(), that will return true because of non-zero msi_redir_hint field. To be on the safe side, I thought of initializing other struct fields as well. If the above explanation makes sense, I can include it in the patch subject and send a second version of this patch? > > Thanks, > > Paolo > >> Fixes: 7ee30bc132c6("KVM: x86: deliver KVM IOAPIC scan request to target vCPUs") >> Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com> >> --- >> arch/x86/kvm/ioapic.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c >> index 7668fed..3a8467d 100644 >> --- a/arch/x86/kvm/ioapic.c >> +++ b/arch/x86/kvm/ioapic.c >> @@ -378,12 +378,15 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val) >> if (e->fields.delivery_mode == APIC_DM_FIXED) { >> struct kvm_lapic_irq irq; >> >> - irq.shorthand = APIC_DEST_NOSHORT; >> irq.vector = e->fields.vector; >> irq.delivery_mode = e->fields.delivery_mode << 8; >> - irq.dest_id = e->fields.dest_id; >> irq.dest_mode = >> kvm_lapic_irq_dest_mode(!!e->fields.dest_mode); >> + irq.level = 1; >> + irq.trig_mode = e->fields.trig_mode; >> + irq.shorthand = APIC_DEST_NOSHORT; >> + irq.dest_id = e->fields.dest_id; >> + irq.msi_redir_hint = false; >> bitmap_zero(&vcpu_bitmap, 16); >> kvm_bitmap_or_dest_vcpus(ioapic->kvm, &irq, >> &vcpu_bitmap); >>
diff --git a/arch/x86/kvm/ioapic.c b/arch/x86/kvm/ioapic.c index 7668fed..3a8467d 100644 --- a/arch/x86/kvm/ioapic.c +++ b/arch/x86/kvm/ioapic.c @@ -378,12 +378,15 @@ static void ioapic_write_indirect(struct kvm_ioapic *ioapic, u32 val) if (e->fields.delivery_mode == APIC_DM_FIXED) { struct kvm_lapic_irq irq; - irq.shorthand = APIC_DEST_NOSHORT; irq.vector = e->fields.vector; irq.delivery_mode = e->fields.delivery_mode << 8; - irq.dest_id = e->fields.dest_id; irq.dest_mode = kvm_lapic_irq_dest_mode(!!e->fields.dest_mode); + irq.level = 1; + irq.trig_mode = e->fields.trig_mode; + irq.shorthand = APIC_DEST_NOSHORT; + irq.dest_id = e->fields.dest_id; + irq.msi_redir_hint = false; bitmap_zero(&vcpu_bitmap, 16); kvm_bitmap_or_dest_vcpus(ioapic->kvm, &irq, &vcpu_bitmap);
Previously all fields of structure kvm_lapic_irq were not initialized before it was passed to kvm_bitmap_or_dest_vcpus(). Which will cause an issue when any of those fields are used for processing a request. This patch initializes all the fields of kvm_lapic_irq based on the values which are passed through the ioapic redirect_entry object. Fixes: 7ee30bc132c6("KVM: x86: deliver KVM IOAPIC scan request to target vCPUs") Signed-off-by: Nitesh Narayan Lal <nitesh@redhat.com> --- arch/x86/kvm/ioapic.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)