Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755020AbeAJKyg (ORCPT + 1 other); Wed, 10 Jan 2018 05:54:36 -0500 Received: from foss.arm.com ([217.140.101.70]:39642 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753621AbeAJKyO (ORCPT ); Wed, 10 Jan 2018 05:54:14 -0500 Subject: Re: [PATCH v1 01/16] virtio: Validate queue pfn for 32bit transports To: "Michael S. Tsirkin" Cc: linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu, christoffer.dall@linaro.org, marc.zyngier@arm.com, linux-kernel@vger.kernel.org, kristina.martsenko@arm.com, peter.maydell@linaro.org, pbonzini@redhat.com, rkrcmar@redhat.com, will.deacon@arm.com, ard.biesheuvel@linaro.org, mark.rutland@arm.com, catalin.marinas@arm.com, Jason Wang , Christoffer Dall References: <20180109190414.4017-1-suzuki.poulose@arm.com> <20180109190414.4017-2-suzuki.poulose@arm.com> <20180110012429-mutt-send-email-mst@kernel.org> From: Suzuki K Poulose Message-ID: Date: Wed, 10 Jan 2018 10:54:09 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20180110012429-mutt-send-email-mst@kernel.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: 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" >> Cc: Jason Wang >> Cc: Marc Zyngier >> Cc: Christoffer Dall >> Signed-off-by: Suzuki K Poulose > > 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. > >> --- >> 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. May be it doesn't make much difference as we write PFN = 0 anyway down. >> /* Activate the queue */ >> writel(virtqueue_get_vring_size(vq), vm_dev->base + VIRTIO_MMIO_QUEUE_NUM); >> if (vm_dev->version == 1) { >> writel(PAGE_SIZE, vm_dev->base + VIRTIO_MMIO_QUEUE_ALIGN); >> - writel(virtqueue_get_desc_addr(vq) >> PAGE_SHIFT, >> + writel(addr >> PAGE_SHIFT, >> vm_dev->base + VIRTIO_MMIO_QUEUE_PFN); >> } else { >> - u64 addr; >> >> - addr = virtqueue_get_desc_addr(vq); >> writel((u32)addr, vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_LOW); >> writel((u32)(addr >> 32), >> vm_dev->base + VIRTIO_MMIO_QUEUE_DESC_HIGH); >> @@ -430,6 +441,8 @@ static struct virtqueue *vm_setup_vq(struct virtio_device *vdev, unsigned index, >> >> return vq; >> >> +error_bad_pfn: >> + vring_del_virtqueue(vq); >> error_new_virtqueue: >> if (vm_dev->version == 1) { >> writel(0, vm_dev->base + VIRTIO_MMIO_QUEUE_PFN); >> diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c >> index 2780886e8ba3..099d2cfb47b3 100644 >> --- 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 ? I could skip it if you think it is not needed. Thanks Suzuki