diff mbox

[v4,02/20] virtio: pci-legacy: Validate queue pfn

Message ID 1531905547-25478-3-git-send-email-suzuki.poulose@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Suzuki K Poulose July 18, 2018, 9:18 a.m. UTC 
Legacy PCI over virtio uses 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@kernel.org>
Cc: Peter Maydel <peter.maydell@linaro.org>
Cc: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
Changes since v2:
 - Change errno to -E2BIG
---
 drivers/virtio/virtio_pci_legacy.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Michael S. Tsirkin July 22, 2018, 3:53 p.m. UTC | #1 
On Wed, Jul 18, 2018 at 10:18:45AM +0100, Suzuki K Poulose wrote:
> Legacy PCI over virtio uses 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@kernel.org>
> Cc: Peter Maydel <peter.maydell@linaro.org>
> Cc: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

I assume this will be merged through some other tree.

> ---
> Changes since v2:
>  - Change errno to -E2BIG
> ---
>  drivers/virtio/virtio_pci_legacy.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
> index 2780886..de062fb 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,17 @@ 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,
> +			"platform bug: legacy virtio-mmio must not be used with RAM above 0x%llxGB\n",
> +			0x1ULL << (32 + PAGE_SHIFT - 30));
> +		err = -E2BIG;
> +		goto out_del_vq;
> +	}
> +
>  	/* activate the queue */
> -	iowrite32(virtqueue_get_desc_addr(vq) >> VIRTIO_PCI_QUEUE_ADDR_SHIFT,
> -		  vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
> +	iowrite32(q_pfn, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
>  
>  	vq->priv = (void __force *)vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY;
>  
> @@ -160,6 +169,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
>  
>  out_deactivate:
>  	iowrite32(0, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
> +out_del_vq:
>  	vring_del_virtqueue(vq);
>  	return ERR_PTR(err);
>  }
> -- 
> 2.7.4
Suzuki K Poulose July 23, 2018, 9:44 a.m. UTC | #2 
On 07/22/2018 04:53 PM, Michael S. Tsirkin wrote:
> On Wed, Jul 18, 2018 at 10:18:45AM +0100, Suzuki K Poulose wrote:
>> Legacy PCI over virtio uses 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@kernel.org>
>> Cc: Peter Maydel <peter.maydell@linaro.org>
>> Cc: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>


Michael,

Thanks.

> 
> I assume this will be merged through some other tree.
>


As such these two virtio patches do not have any code dependencies with
the rest of the series. So, if you could pick this up it should be fine.
Otherwise, may be Marc can push it with the rest of the series.

Marc,

Are you OK with that ?

Suzuki
Marc Zyngier July 23, 2018, 12:54 p.m. UTC | #3 
On 23/07/18 10:44, Suzuki K Poulose wrote:
> On 07/22/2018 04:53 PM, Michael S. Tsirkin wrote:
>> On Wed, Jul 18, 2018 at 10:18:45AM +0100, Suzuki K Poulose wrote:
>>> Legacy PCI over virtio uses 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@kernel.org>
>>> Cc: Peter Maydel <peter.maydell@linaro.org>
>>> Cc: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>>
>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> 
> Michael,
> 
> Thanks.
> 
>>
>> I assume this will be merged through some other tree.
>>
> 
> 
> As such these two virtio patches do not have any code dependencies with
> the rest of the series. So, if you could pick this up it should be fine.
> Otherwise, may be Marc can push it with the rest of the series.
> 
> Marc,
> 
> Are you OK with that ?

Given that these two patches completely independent, I think their
natural path should be the virtio tree. But if Michael doesn't want to
pick them, I'll do it as part of this series.

Thanks,

	M.
Michael S. Tsirkin July 23, 2018, 2:20 p.m. UTC | #4 
On Mon, Jul 23, 2018 at 01:54:10PM +0100, Marc Zyngier wrote:
> On 23/07/18 10:44, Suzuki K Poulose wrote:
> > On 07/22/2018 04:53 PM, Michael S. Tsirkin wrote:
> >> On Wed, Jul 18, 2018 at 10:18:45AM +0100, Suzuki K Poulose wrote:
> >>> Legacy PCI over virtio uses 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@kernel.org>
> >>> Cc: Peter Maydel <peter.maydell@linaro.org>
> >>> Cc: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> >>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> >>
> >> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > 
> > Michael,
> > 
> > Thanks.
> > 
> >>
> >> I assume this will be merged through some other tree.
> >>
> > 
> > 
> > As such these two virtio patches do not have any code dependencies with
> > the rest of the series. So, if you could pick this up it should be fine.
> > Otherwise, may be Marc can push it with the rest of the series.
> > 
> > Marc,
> > 
> > Are you OK with that ?
> 
> Given that these two patches completely independent, I think their
> natural path should be the virtio tree. But if Michael doesn't want to
> pick them, I'll do it as part of this series.
> 
> Thanks,
> 
> 	M.

It's ok, I can pick them up.

> -- 
> Jazz is not dead. It just smells funny...
diff mbox

Patch

diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
index 2780886..de062fb 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,17 @@  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,
+			"platform bug: legacy virtio-mmio must not be used with RAM above 0x%llxGB\n",
+			0x1ULL << (32 + PAGE_SHIFT - 30));
+		err = -E2BIG;
+		goto out_del_vq;
+	}
+
 	/* activate the queue */
-	iowrite32(virtqueue_get_desc_addr(vq) >> VIRTIO_PCI_QUEUE_ADDR_SHIFT,
-		  vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
+	iowrite32(q_pfn, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
 
 	vq->priv = (void __force *)vp_dev->ioaddr + VIRTIO_PCI_QUEUE_NOTIFY;
 
@@ -160,6 +169,7 @@  static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 
 out_deactivate:
 	iowrite32(0, vp_dev->ioaddr + VIRTIO_PCI_QUEUE_PFN);
+out_del_vq:
 	vring_del_virtqueue(vq);
 	return ERR_PTR(err);
 }