| Message ID | 20210913154413.14322-7-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/pci: Fix MSIX table and PBA size allocation | expand |
On Mon, 13 Sep 2021 16:44:12 +0100 Alexandru Elisei <alexandru.elisei@arm.com> wrote: > Now that we keep track of the real size of MSIX table and PBA, print an > error when the guest tries to write to an offset which is not inside the > correct regions. What is the actual purpose of this message? Is there anything the user can do about it? Shouldn't we either abort the guest or inject an error condition back into KVM or the guest instead? Cheers, Andre > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > --- > vfio/pci.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/vfio/pci.c b/vfio/pci.c > index 7781868..a6d0408 100644 > --- a/vfio/pci.c > +++ b/vfio/pci.c > @@ -249,6 +249,11 @@ static void vfio_pci_msix_pba_access(struct kvm_cpu > *vcpu, u64 addr, u8 *data, u64 offset = addr - pba->guest_phys_addr; > struct vfio_device *vdev = container_of(pdev, struct > vfio_device, pci); > + if (offset >= pba->size) { > + vfio_dev_err(vdev, "access outside of the MSIX PBA"); > + return; > + } > + > if (is_write) > return; > > @@ -269,6 +274,10 @@ static void vfio_pci_msix_table_access(struct > kvm_cpu *vcpu, u64 addr, u8 *data, struct vfio_device *vdev = > container_of(pdev, struct vfio_device, pci); > u64 offset = addr - pdev->msix_table.guest_phys_addr; > + if (offset >= pdev->msix_table.size) { > + vfio_dev_err(vdev, "access outside of the MSI-X table"); > + return; > + } > > size_t vector = offset / PCI_MSIX_ENTRY_SIZE; > off_t field = offset % PCI_MSIX_ENTRY_SIZE;
Hi Andre, On Wed, Oct 06, 2021 at 04:11:28PM +0100, Andre Przywara wrote: > On Mon, 13 Sep 2021 16:44:12 +0100 > Alexandru Elisei <alexandru.elisei@arm.com> wrote: > > > Now that we keep track of the real size of MSIX table and PBA, print an > > error when the guest tries to write to an offset which is not inside the > > correct regions. > > What is the actual purpose of this message? Is there anything the user can > do about it? Shouldn't we either abort the guest or inject an error > condition back into KVM or the guest instead? The only way for the error message to trigger is in case of a programming error in kvmtool - when access to the PBA BAR is enabled by the guest, kvmtool will register the vfio_pci_msix_pba_access callback for the memory region [pba->guest_phys_addr, pba->guest_phys_addr + pba->size) in vfio_pci_bar_activate(). I believe kvmtool should definitely print something when it detects a programming error. The question now becomes if kvmtool should die or continue running the VM, as the VM can run just fine even if emulation for assigned devices is malfunctioning. I would rather give the user the chance to safely shutdown the VM instead of killing it outright. Thanks, Alex > > Cheers, > Andre > > > > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > > --- > > vfio/pci.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/vfio/pci.c b/vfio/pci.c > > index 7781868..a6d0408 100644 > > --- a/vfio/pci.c > > +++ b/vfio/pci.c > > @@ -249,6 +249,11 @@ static void vfio_pci_msix_pba_access(struct kvm_cpu > > *vcpu, u64 addr, u8 *data, u64 offset = addr - pba->guest_phys_addr; > > struct vfio_device *vdev = container_of(pdev, struct > > vfio_device, pci); > > + if (offset >= pba->size) { > > + vfio_dev_err(vdev, "access outside of the MSIX PBA"); > > + return; > > + } > > + > > if (is_write) > > return; > > > > @@ -269,6 +274,10 @@ static void vfio_pci_msix_table_access(struct > > kvm_cpu *vcpu, u64 addr, u8 *data, struct vfio_device *vdev = > > container_of(pdev, struct vfio_device, pci); > > u64 offset = addr - pdev->msix_table.guest_phys_addr; > > + if (offset >= pdev->msix_table.size) { > > + vfio_dev_err(vdev, "access outside of the MSI-X table"); > > + return; > > + } > > > > size_t vector = offset / PCI_MSIX_ENTRY_SIZE; > > off_t field = offset % PCI_MSIX_ENTRY_SIZE; >
diff --git a/vfio/pci.c b/vfio/pci.c index 7781868..a6d0408 100644 --- a/vfio/pci.c +++ b/vfio/pci.c @@ -249,6 +249,11 @@ static void vfio_pci_msix_pba_access(struct kvm_cpu *vcpu, u64 addr, u8 *data, u64 offset = addr - pba->guest_phys_addr; struct vfio_device *vdev = container_of(pdev, struct vfio_device, pci); + if (offset >= pba->size) { + vfio_dev_err(vdev, "access outside of the MSIX PBA"); + return; + } + if (is_write) return; @@ -269,6 +274,10 @@ static void vfio_pci_msix_table_access(struct kvm_cpu *vcpu, u64 addr, u8 *data, struct vfio_device *vdev = container_of(pdev, struct vfio_device, pci); u64 offset = addr - pdev->msix_table.guest_phys_addr; + if (offset >= pdev->msix_table.size) { + vfio_dev_err(vdev, "access outside of the MSI-X table"); + return; + } size_t vector = offset / PCI_MSIX_ENTRY_SIZE; off_t field = offset % PCI_MSIX_ENTRY_SIZE;
Now that we keep track of the real size of MSIX table and PBA, print an error when the guest tries to write to an offset which is not inside the correct regions. Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- vfio/pci.c | 9 +++++++++ 1 file changed, 9 insertions(+)