Messages in this thread | | | Subject | Re: [PATCH v1 01/16] virtio: Validate queue pfn for 32bit transports | From | Suzuki K Poulose <> | Date | Wed, 10 Jan 2018 11:18:09 +0000 |
| |
On 10/01/18 11:06, Michael S. Tsirkin wrote: > On Wed, Jan 10, 2018 at 10:54:09AM +0000, Suzuki K Poulose wrote: >> On 09/01/18 23:29, Michael S. Tsirkin wrote: >>> On Tue, Jan 09, 2018 at 07:03:56PM +0000, Suzuki K Poulose wrote: >>>> virtio-mmio using virtio-v1 and virtio legacy pci use a 32bit PFN >>>> for the queue. If the queue pfn is too large to fit in 32bits, which >>>> we could hit on arm64 systems with 52bit physical addresses (even with >>>> 64K page size), we simply miss out a proper link to the other side of >>>> the queue. >>>> >>>> Add a check to validate the PFN, rather than silently breaking >>>> the devices. >>>> >>>> Cc: "Michael S. Tsirkin" <mst@redhat.com> >>>> Cc: Jason Wang <jasowang@redhat.com> >>>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>>> Cc: Christoffer Dall <cdall@linaro.org> >>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >>> >>> Could you guys please work on virtio 1 support in >>> for virtio mmio in qemu though? >>> It's not a lot of code. >> >> Did you mean kvmtool ? Qemu already supports virto-1. > > For virtio-mmio? I don't seem to see that code in > hw/virtio/virtio-mmio.c > For example I still see handling for VIRTIO_MMIO_QUEUE_PFN > there, and no handling for VIRTIO_MMIO_QUEUE_DESC_LOW > and such. > > What am I missing?
Nah, you're right. Its the PCI that uses QUEUE_DESC_LOW/HIGH. Btw, I can't work on Qemu.
> >>> >>>> --- >>>> drivers/virtio/virtio_mmio.c | 19 ++++++++++++++++--- >>>> drivers/virtio/virtio_pci_legacy.c | 11 +++++++++-- >>>> 2 files changed, 25 insertions(+), 5 deletions(-) >>> >>> I'd rather see this as 2 patches. >> >> OK, I will split them. >> >>> >>>> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c >>>> index a9192fe4f345..47109baf37f7 100644 >>>> --- a/drivers/virtio/virtio_mmio.c >>>> +++ b/drivers/virtio/virtio_mmio.c >>>> @@ -358,6 +358,7 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index, >>>> struct virtqueue *vq; >>>> unsigned long flags; >>>> unsigned int num; >>>> + u64 addr; >>>> int err; >>>> if (!name) >>>> @@ -394,16 +395,26 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index, >>>> goto error_new_virtqueue; >>>> } >>>> + addr = virtqueue_get_desc_addr(vq); >>>> + /* >>>> + * virtio-mmio v1 uses a 32bit QUEUE PFN. If we have something that >>>> + * doesn't fit in 32bit, fail the setup rather than pretending to >>>> + * be successful. >>>> + */ >>>> + if (vm_dev->version == 1 && (addr >> (PAGE_SHIFT + 32))) { >>>> + dev_err(&vdev->dev, "virtio-mmio: queue address too large\n"); >>>> + err = -ENOMEM; >>>> + goto error_bad_pfn; >>>> + } >>>> + >>> >>> Can you please move this below to where it's actually used? >>> >> >> The reason for keeping it here was to skip selecting the Queue number if we >> have a bad PFN. > > Why is selecting a problem if we don't program anything? >
I will be honest here, I don't know :-). The only reasoning was why do something that you are not going to use after all. I will move it down.
>> May be it doesn't make much difference as we write PFN = 0 anyway >> down.
>>>> --- a/drivers/virtio/virtio_pci_legacy.c >>>> +++ b/drivers/virtio/virtio_pci_legacy.c >>>> @@ -122,6 +122,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, >>>> struct virtqueue *vq; >>>> u16 num; >>>> int err; >>>> + u64 q_pfn; >>>> /* Select the queue we're interested in */ >>>> iowrite16(index, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_SEL); >>>> @@ -141,9 +142,15 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev, >>>> if (!vq) >>>> return ERR_PTR(-ENOMEM); >>>> + q_pfn = virtqueue_get_desc_addr(vq) >> VIRTIO_PCI_QUEUE_ADDR_SHIFT; >>>> + if (q_pfn >> 32) { >>>> + dev_err(&vp_dev->pci_dev->dev, "virtio-pci queue PFN too large\n"); >>>> + err = -ENOMEM; >>>> + goto out_deactivate; >>> >>> You never set up the address, it's cleaner to add another target >>> and not reset it. >> >> Thats right. However, the only thing we do is writing PFN=0, which would be a good >> thing to do to indicate the error to the host ? > > It isn't, a good way to indicate error is to set a bad status > which happens anyway I think. Writing PFN 0 resets the device > instead.
Ok, thats good to know. I will make the necessary changes. Thanks for the explanation.
Cheers Suzuki
| |