[PATCH 00/11] virtio: pci: Add and fix consistency checks

[PATCH 00/11] virtio: pci: Add and fix consistency checks

[Andrew Scull]

The virtio PCI drivers forgo a number of consistency checks,
particularly around pointer validation and bounds checking. This series
focuses on the modern driver to add those checks.

The start of the series adds and fixes some basic bounds checks. Later
patches ensure PCI addresses fall within the expected regions rather
than any arbitrary address.

The series applies atop v2022.04-rc4. I have been boot testing on the
AOSP cuttlefish virtualized device and protected KVM as part of the
Android Virtualization Framework (AVF).

Andrew Scull (11):
  virtio: pci: Fix discovery of device config length
  virtio: pci: Bounds check device config access
  virtio: pci: Bounds check notification writes
  virtio: pci: Check virtio common config size
  virtio: pci: Check virtio capability is in bounds
  virtio: pci: Read entire capability into memory
  virtio: pci: Check virtio configs are mapped
  pci: Check region ranges are addressable
  pci: Add function to validate PCI address range
  virtio: pci: Check mapped range is in a PCI region
  virtio: pci: Allow exclusion of legacy driver

 drivers/pci/pci-uclass.c           |  47 ++++++++-
 drivers/virtio/Kconfig             |   9 ++
 drivers/virtio/Makefile            |   3 +-
 drivers/virtio/virtio_pci_modern.c | 147 ++++++++++++++++++++++-------
 include/pci.h                      |  16 ++++
 5 files changed, 184 insertions(+), 38 deletions(-)

-- 
2.35.1.894.gb6a874cedc-goog

[PATCH 01/11] virtio: pci: Fix discovery of device config length

[Andrew Scull]

The length of the device config was erroneously being taken from the
notify capability. Correct this by finding the length in the device
capability.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 drivers/virtio/virtio_pci_modern.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index fd8a1f3fec..55d25cb81b 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -497,7 +497,7 @@ static int virtio_pci_probe(struct udevice *udev)
 	 */
 	device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG);
 	if (device) {
-		offset = notify + offsetof(struct virtio_pci_cap, length);
+		offset = device + offsetof(struct virtio_pci_cap, length);
 		dm_pci_read_config32(udev, offset, &priv->device_len);
 	}
 
-- 
2.35.1.894.gb6a874cedc-goog

[PATCH 02/11] virtio: pci: Bounds check device config access

[Andrew Scull]

The device config is optional, so check it was present and mapped before
trying to use the pointer. Bounds violations are an error, not just a
warning, so bail if the checks fail.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 drivers/virtio/virtio_pci_modern.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 55d25cb81b..bcf9f18997 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -114,7 +114,11 @@ static int virtio_pci_get_config(struct udevice *udev, unsigned int offset,
 	__le16 w;
 	__le32 l;
 
-	WARN_ON(offset + len > priv->device_len);
+	if (!priv->device)
+		return -ENOSYS;
+
+	if (offset + len > priv->device_len)
+		return -EINVAL;
 
 	switch (len) {
 	case 1:
@@ -136,7 +140,7 @@ static int virtio_pci_get_config(struct udevice *udev, unsigned int offset,
 		memcpy(buf + sizeof(l), &l, sizeof(l));
 		break;
 	default:
-		WARN_ON(true);
+		return -EINVAL;
 	}
 
 	return 0;
@@ -150,7 +154,11 @@ static int virtio_pci_set_config(struct udevice *udev, unsigned int offset,
 	__le16 w;
 	__le32 l;
 
-	WARN_ON(offset + len > priv->device_len);
+	if (!priv->device)
+		return -ENOSYS;
+
+	if (offset + len > priv->device_len)
+		return -EINVAL;
 
 	switch (len) {
 	case 1:
@@ -172,7 +180,7 @@ static int virtio_pci_set_config(struct udevice *udev, unsigned int offset,
 		iowrite32(le32_to_cpu(l), priv->device + offset + sizeof(l));
 		break;
 	default:
-		WARN_ON(true);
+		return -EINVAL;
 	}
 
 	return 0;
-- 
2.35.1.894.gb6a874cedc-goog

[PATCH 03/11] virtio: pci: Bounds check notification writes

[Andrew Scull]

Make sure virtio notifications are written within their allocated
buffer.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 drivers/virtio/virtio_pci_modern.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index bcf9f18997..60bdc53a6d 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -101,6 +101,7 @@
 struct virtio_pci_priv {
 	struct virtio_pci_common_cfg __iomem *common;
 	void __iomem *notify_base;
+	u32 notify_len;
 	void __iomem *device;
 	u32 device_len;
 	u32 notify_offset_multiplier;
@@ -372,12 +373,16 @@ static int virtio_pci_notify(struct udevice *udev, struct virtqueue *vq)
 	/* get offset of notification word for this vq */
 	off = ioread16(&priv->common->queue_notify_off);
 
+	/* Check the effective offset is in bounds */
+	off *= priv->notify_offset_multiplier;
+	if (off > priv->notify_len - sizeof(u16))
+		return -EIO;
+
 	/*
 	 * We write the queue's selector into the notification register
 	 * to signal the other end
 	 */
-	iowrite16(vq->index,
-		  priv->notify_base + off * priv->notify_offset_multiplier);
+	iowrite16(vq->index, priv->notify_base + off);
 
 	return 0;
 }
@@ -499,6 +504,9 @@ static int virtio_pci_probe(struct udevice *udev)
 		return -EINVAL;
 	}
 
+	offset = notify + offsetof(struct virtio_pci_cap, length);
+	dm_pci_read_config32(udev, offset, &priv->notify_len);
+
 	/*
 	 * Device capability is only mandatory for devices that have
 	 * device-specific configuration.
-- 
2.35.1.894.gb6a874cedc-goog

[PATCH 04/11] virtio: pci: Check virtio common config size

[Andrew Scull]

Check that the common config is at least as large as the struct it is
expected to contain. Only then is it safe to cast the pointer and be
safe from out-of-bounds accesses.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 drivers/virtio/virtio_pci_modern.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 60bdc53a6d..3403ff5cca 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -475,6 +475,7 @@ static int virtio_pci_probe(struct udevice *udev)
 	u16 subvendor;
 	u8 revision;
 	int common, notify, device;
+	u32 common_length;
 	int offset;
 
 	/* We only own devices >= 0x1040 and <= 0x107f: leave the rest. */
@@ -496,6 +497,13 @@ static int virtio_pci_probe(struct udevice *udev)
 		return -ENODEV;
 	}
 
+	offset = common + offsetof(struct virtio_pci_cap, length);
+	dm_pci_read_config32(udev, offset, &common_length);
+	if (common_length < sizeof(struct virtio_pci_common_cfg)) {
+		printf("(%s): virtio common config too small\n", udev->name);
+		return -EINVAL;
+	}
+
 	/* If common is there, notify should be too */
 	notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG);
 	if (!notify) {
-- 
2.35.1.894.gb6a874cedc-goog

[PATCH 05/11] virtio: pci: Check virtio capability is in bounds

[Andrew Scull]

Ensure the virtio PCI capabilities are contained within the bounds of
the device's configuration space. The expected size of the capability is
passed when searching for the capability to enforce this check.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 drivers/virtio/virtio_pci_modern.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 3403ff5cca..4b346be257 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -392,18 +392,30 @@ static int virtio_pci_notify(struct udevice *udev, struct virtqueue *vq)
  *
  * @udev:	the transport device
  * @cfg_type:	the VIRTIO_PCI_CAP_* value we seek
+ * @cap_size:	expected size of the capability
  *
  * Return: offset of the configuration structure
  */
-static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type)
+static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
+				      size_t cap_size)
 {
 	int pos;
 	int offset;
 	u8 type, bar;
 
+	if (cap_size < sizeof(struct virtio_pci_cap))
+		return 0;
+
+	if (cap_size > PCI_CFG_SPACE_SIZE)
+		return 0;
+
 	for (pos = dm_pci_find_capability(udev, PCI_CAP_ID_VNDR);
 	     pos > 0;
 	     pos = dm_pci_find_next_capability(udev, pos, PCI_CAP_ID_VNDR)) {
+		/* Ensure the capability is within bounds */
+		if (PCI_CFG_SPACE_SIZE - cap_size < pos)
+			return 0;
+
 		offset = pos + offsetof(struct virtio_pci_cap, cfg_type);
 		dm_pci_read_config8(udev, offset, &type);
 		offset = pos + offsetof(struct virtio_pci_cap, bar);
@@ -491,7 +503,8 @@ static int virtio_pci_probe(struct udevice *udev)
 	uc_priv->vendor = subvendor;
 
 	/* Check for a common config: if not, use legacy mode (bar 0) */
-	common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG);
+	common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG,
+					    sizeof(struct virtio_pci_cap));
 	if (!common) {
 		printf("(%s): leaving for legacy driver\n", udev->name);
 		return -ENODEV;
@@ -505,7 +518,8 @@ static int virtio_pci_probe(struct udevice *udev)
 	}
 
 	/* If common is there, notify should be too */
-	notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG);
+	notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG,
+					    sizeof(struct virtio_pci_notify_cap));
 	if (!notify) {
 		printf("(%s): missing capabilities %i/%i\n", udev->name,
 		       common, notify);
@@ -519,7 +533,8 @@ static int virtio_pci_probe(struct udevice *udev)
 	 * Device capability is only mandatory for devices that have
 	 * device-specific configuration.
 	 */
-	device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG);
+	device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG,
+					    sizeof(struct virtio_pci_cap));
 	if (device) {
 		offset = device + offsetof(struct virtio_pci_cap, length);
 		dm_pci_read_config32(udev, offset, &priv->device_len);
-- 
2.35.1.894.gb6a874cedc-goog

[PATCH 06/11] virtio: pci: Read entire capability into memory

[Andrew Scull]

Read the virtio PCI capability out of the device configuration space to
a struct rather than accessing fields directly from the configuration
space as they are needed. This both makes access to the fields easier
and avoids re-reading fields.

Re-reading fields could result in time-of-check to time-of-use problems,
should the value in the configuration space change. The range check of
the `bar` field and the later call to `dm_pci_read_bar32()` is an
example of where this could happen.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 drivers/virtio/virtio_pci_modern.c | 74 ++++++++++++++++--------------
 1 file changed, 40 insertions(+), 34 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 4b346be257..38a0da0a84 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -393,15 +393,16 @@ static int virtio_pci_notify(struct udevice *udev, struct virtqueue *vq)
  * @udev:	the transport device
  * @cfg_type:	the VIRTIO_PCI_CAP_* value we seek
  * @cap_size:	expected size of the capability
+ * @cap:	capability read from the config space
  *
  * Return: offset of the configuration structure
  */
 static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
-				      size_t cap_size)
+				      size_t cap_size,
+				      struct virtio_pci_cap *cap)
 {
 	int pos;
 	int offset;
-	u8 type, bar;
 
 	if (cap_size < sizeof(struct virtio_pci_cap))
 		return 0;
@@ -409,6 +410,9 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
 	if (cap_size > PCI_CFG_SPACE_SIZE)
 		return 0;
 
+	if (!cap)
+		return 0;
+
 	for (pos = dm_pci_find_capability(udev, PCI_CAP_ID_VNDR);
 	     pos > 0;
 	     pos = dm_pci_find_next_capability(udev, pos, PCI_CAP_ID_VNDR)) {
@@ -416,16 +420,26 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
 		if (PCI_CFG_SPACE_SIZE - cap_size < pos)
 			return 0;
 
+		offset = pos + offsetof(struct virtio_pci_cap, cap_vndr);
+		dm_pci_read_config8(udev, offset, &cap->cap_vndr);
+		offset = pos + offsetof(struct virtio_pci_cap, cap_next);
+		dm_pci_read_config8(udev, offset, &cap->cap_next);
+		offset = pos + offsetof(struct virtio_pci_cap, cap_len);
+		dm_pci_read_config8(udev, offset, &cap->cap_len);
 		offset = pos + offsetof(struct virtio_pci_cap, cfg_type);
-		dm_pci_read_config8(udev, offset, &type);
+		dm_pci_read_config8(udev, offset, &cap->cfg_type);
 		offset = pos + offsetof(struct virtio_pci_cap, bar);
-		dm_pci_read_config8(udev, offset, &bar);
+		dm_pci_read_config8(udev, offset, &cap->bar);
+		offset = pos + offsetof(struct virtio_pci_cap, offset);
+		dm_pci_read_config32(udev, offset, &cap->offset);
+		offset = pos + offsetof(struct virtio_pci_cap, length);
+		dm_pci_read_config32(udev, offset, &cap->length);
 
 		/* Ignore structures with reserved BAR values */
-		if (bar > 0x5)
+		if (cap->bar > 0x5)
 			continue;
 
-		if (type == cfg_type)
+		if (cap->cfg_type == cfg_type)
 			return pos;
 	}
 
@@ -436,33 +450,27 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
  * virtio_pci_map_capability - map base address of the capability
  *
  * @udev:	the transport device
- * @off:	offset of the configuration structure
+ * @cap:	capability to map
  *
  * Return: base address of the capability
  */
-static void __iomem *virtio_pci_map_capability(struct udevice *udev, int off)
+static void __iomem *virtio_pci_map_capability(struct udevice *udev,
+					       const struct virtio_pci_cap *cap)
 {
-	u8 bar;
-	u32 offset;
 	ulong base;
 	void __iomem *p;
 
-	if (!off)
+	if (!cap)
 		return NULL;
 
-	offset = off + offsetof(struct virtio_pci_cap, bar);
-	dm_pci_read_config8(udev, offset, &bar);
-	offset = off + offsetof(struct virtio_pci_cap, offset);
-	dm_pci_read_config32(udev, offset, &offset);
-
 	/*
 	 * TODO: adding 64-bit BAR support
 	 *
 	 * Per spec, the BAR is permitted to be either 32-bit or 64-bit.
 	 * For simplicity, only read the BAR address as 32-bit.
 	 */
-	base = dm_pci_read_bar32(udev, bar);
-	p = (void __iomem *)base + offset;
+	base = dm_pci_read_bar32(udev, cap->bar);
+	p = (void __iomem *)base + cap->offset;
 
 	return p;
 }
@@ -487,7 +495,7 @@ static int virtio_pci_probe(struct udevice *udev)
 	u16 subvendor;
 	u8 revision;
 	int common, notify, device;
-	u32 common_length;
+	struct virtio_pci_cap common_cap, notify_cap, device_cap;
 	int offset;
 
 	/* We only own devices >= 0x1040 and <= 0x107f: leave the rest. */
@@ -504,46 +512,44 @@ static int virtio_pci_probe(struct udevice *udev)
 
 	/* Check for a common config: if not, use legacy mode (bar 0) */
 	common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG,
-					    sizeof(struct virtio_pci_cap));
+					    sizeof(struct virtio_pci_cap),
+					    &common_cap);
 	if (!common) {
 		printf("(%s): leaving for legacy driver\n", udev->name);
 		return -ENODEV;
 	}
 
-	offset = common + offsetof(struct virtio_pci_cap, length);
-	dm_pci_read_config32(udev, offset, &common_length);
-	if (common_length < sizeof(struct virtio_pci_common_cfg)) {
+	if (common_cap.length < sizeof(struct virtio_pci_common_cfg)) {
 		printf("(%s): virtio common config too small\n", udev->name);
 		return -EINVAL;
 	}
 
 	/* If common is there, notify should be too */
 	notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG,
-					    sizeof(struct virtio_pci_notify_cap));
+					    sizeof(struct virtio_pci_notify_cap),
+					    &notify_cap);
 	if (!notify) {
 		printf("(%s): missing capabilities %i/%i\n", udev->name,
 		       common, notify);
 		return -EINVAL;
 	}
 
-	offset = notify + offsetof(struct virtio_pci_cap, length);
-	dm_pci_read_config32(udev, offset, &priv->notify_len);
+	priv->notify_len = notify_cap.length;
 
 	/*
 	 * Device capability is only mandatory for devices that have
 	 * device-specific configuration.
 	 */
 	device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG,
-					    sizeof(struct virtio_pci_cap));
-	if (device) {
-		offset = device + offsetof(struct virtio_pci_cap, length);
-		dm_pci_read_config32(udev, offset, &priv->device_len);
-	}
+					    sizeof(struct virtio_pci_cap),
+					    &device_cap);
+	if (device)
+		priv->device_len = device_cap.length;
 
 	/* Map configuration structures */
-	priv->common = virtio_pci_map_capability(udev, common);
-	priv->notify_base = virtio_pci_map_capability(udev, notify);
-	priv->device = virtio_pci_map_capability(udev, device);
+	priv->common = virtio_pci_map_capability(udev, &common_cap);
+	priv->notify_base = virtio_pci_map_capability(udev, &notify_cap);
+	priv->device = virtio_pci_map_capability(udev, &device_cap);
 	debug("(%p): common @ %p, notify base @ %p, device @ %p\n",
 	      udev, priv->common, priv->notify_base, priv->device);
 
-- 
2.35.1.894.gb6a874cedc-goog

[PATCH 07/11] virtio: pci: Check virtio configs are mapped

[Andrew Scull]

The calls to `virtio_pci_map_capability()` return NULL on error. If this
happens, later accesses to the pointers would be unsafe. Avoid this by
failing if the configs were not successfully mapped.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 drivers/virtio/virtio_pci_modern.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 38a0da0a84..2f1a1cedbc 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -534,7 +534,19 @@ static int virtio_pci_probe(struct udevice *udev)
 		return -EINVAL;
 	}
 
+	/* Map configuration structures */
+	priv->common = virtio_pci_map_capability(udev, &common_cap);
+	if (!priv->common) {
+		printf("(%s): could not map common config\n", udev->name);
+		return -EINVAL;
+	}
+
 	priv->notify_len = notify_cap.length;
+	priv->notify_base = virtio_pci_map_capability(udev, &notify_cap);
+	if (!priv->notify_base) {
+		printf("(%s): could not map notify config\n", udev->name);
+		return -EINVAL;
+	}
 
 	/*
 	 * Device capability is only mandatory for devices that have
@@ -543,13 +555,16 @@ static int virtio_pci_probe(struct udevice *udev)
 	device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG,
 					    sizeof(struct virtio_pci_cap),
 					    &device_cap);
-	if (device)
+	if (device) {
 		priv->device_len = device_cap.length;
+		priv->device = virtio_pci_map_capability(udev, &device_cap);
+		if (!priv->device) {
+			printf("(%s): could not map device config\n",
+			       udev->name);
+			return -EINVAL;
+		}
+	}
 
-	/* Map configuration structures */
-	priv->common = virtio_pci_map_capability(udev, &common_cap);
-	priv->notify_base = virtio_pci_map_capability(udev, &notify_cap);
-	priv->device = virtio_pci_map_capability(udev, &device_cap);
 	debug("(%p): common @ %p, notify base @ %p, device @ %p\n",
 	      udev, priv->common, priv->notify_base, priv->device);
 
-- 
2.35.1.894.gb6a874cedc-goog

[PATCH 08/11] pci: Check region ranges are addressable

[Andrew Scull]

When parsing the `ranges` DT node, check that both extremes of the
regions are addressable without overflow. This assumption can then be
safely made when processing the regions.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 drivers/pci/pci-uclass.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index 33dda00002..54a05d7567 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -1013,7 +1013,22 @@ static void decode_regions(struct pci_controller *hose, ofnode parent_node,
 
 		if (!IS_ENABLED(CONFIG_SYS_PCI_64BIT) &&
 		    type == PCI_REGION_MEM && upper_32_bits(pci_addr)) {
-			debug(" - beyond the 32-bit boundary, ignoring\n");
+			debug(" - pci_addr beyond the 32-bit boundary, ignoring\n");
+			continue;
+		}
+
+		if (!IS_ENABLED(CONFIG_PHYS_64BIT) && upper_32_bits(addr)) {
+			debug(" - addr beyond the 32-bit boundary, ignoring\n");
+			continue;
+		}
+
+		if (~((pci_addr_t)0) - pci_addr < size) {
+			debug(" - PCI range exceeds max address, ignoring\n");
+			continue;
+		}
+
+		if (~((phys_addr_t)0) - addr < size) {
+			debug(" - phys range exceeds max address, ignoring\n");
 			continue;
 		}
 
-- 
2.35.1.894.gb6a874cedc-goog

[PATCH 09/11] pci: Add function to validate PCI address range

[Andrew Scull]

Add a function to convert a PCI address range to a physical address
range. The address range is validated to ensure it is contained within
one of the PCI address regions and that the whole range can be indexed
from the resulting physical address.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 drivers/pci/pci-uclass.c | 30 ++++++++++++++++++++++++++++++
 include/pci.h            | 16 ++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index 54a05d7567..efab8916e2 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -1452,6 +1452,36 @@ phys_addr_t dm_pci_bus_to_phys(struct udevice *dev, pci_addr_t bus_addr,
 	return phys_addr;
 }
 
+phys_addr_t dm_pci_bus_range_to_phys(struct udevice *dev, pci_addr_t bus_addr,
+				     size_t size, unsigned long mask,
+				     unsigned long flags)
+{
+	struct udevice *ctlr = pci_get_controller(dev);
+	struct pci_controller *hose = dev_get_uclass_priv(ctlr);
+	struct pci_region *res;
+	size_t offset;
+	int i;
+
+	for (i = 0; i < hose->region_count; i++) {
+		res = &hose->regions[i];
+
+		if ((res->flags & mask) != flags)
+			continue;
+
+		if (bus_addr < res->bus_start)
+			continue;
+
+		offset = bus_addr - res->bus_start;
+		if (offset >= res->size || size > res->size - offset)
+			continue;
+
+		return res->phys_start + offset;
+	}
+
+	puts("pci_bus_range_to_phys: invalid address range\n");
+	return 0;
+}
+
 static int _dm_pci_phys_to_bus(struct udevice *dev, phys_addr_t phys_addr,
 			       unsigned long flags, unsigned long skip_mask,
 			       pci_addr_t *ba)
diff --git a/include/pci.h b/include/pci.h
index 673c95c6bb..15b3031c1f 100644
--- a/include/pci.h
+++ b/include/pci.h
@@ -1446,6 +1446,22 @@ u32 dm_pci_read_bar32(const struct udevice *dev, int barnum);
 phys_addr_t dm_pci_bus_to_phys(struct udevice *dev, pci_addr_t addr,
 			       unsigned long flags);
 
+/**
+ * dm_pci_bus_range_to_phys() - convert a PCI bus address range to a physical
+ *				address.
+ *
+ * @dev:	Device containing the PCI address
+ * @addr:	PCI address to convert
+ * @size:	Size of the addressrange
+ * @mask:	Mask of flags to match
+ * @flags:	Flags for the region type (PCI_REGION_...)
+ * Return: physical address corresponding to that PCI bus address range or 0 if
+ *	   the range could not be converted
+ */
+phys_addr_t dm_pci_bus_range_to_phys(struct udevice *dev, pci_addr_t bus_addr,
+				     size_t size, unsigned long mask,
+				     unsigned long flags);
+
 /**
  * dm_pci_phys_to_bus() - convert a physical address to a PCI bus address
  *
-- 
2.35.1.894.gb6a874cedc-goog

[PATCH 10/11] virtio: pci: Check mapped range is in a PCI region

[Andrew Scull]

The virtio PCI capabilities describe a region of memory that should be
mapped. Ensure those are valid PCI regions before accessing them.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 drivers/virtio/virtio_pci_modern.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 2f1a1cedbc..84750e2b27 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -457,8 +457,9 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
 static void __iomem *virtio_pci_map_capability(struct udevice *udev,
 					       const struct virtio_pci_cap *cap)
 {
-	ulong base;
-	void __iomem *p;
+	unsigned long mask, flags;
+	phys_addr_t phys_addr;
+	u32 base;
 
 	if (!cap)
 		return NULL;
@@ -470,9 +471,23 @@ static void __iomem *virtio_pci_map_capability(struct udevice *udev,
 	 * For simplicity, only read the BAR address as 32-bit.
 	 */
 	base = dm_pci_read_bar32(udev, cap->bar);
-	p = (void __iomem *)base + cap->offset;
 
-	return p;
+	if (U32_MAX - base < cap->offset)
+		return NULL;
+	base += cap->offset;
+
+	if (U32_MAX - base < cap->length)
+		return NULL;
+
+	/* Find the corresponding memory region that isn't system memory. */
+	mask = PCI_REGION_TYPE | PCI_REGION_SYS_MEMORY;
+	flags = PCI_REGION_MEM;
+	phys_addr = dm_pci_bus_range_to_phys(dev_get_parent(udev), base,
+					     cap->length, mask, flags);
+	if (!phys_addr)
+		return NULL;
+
+	return (void __iomem *)map_physmem(phys_addr, cap->length, MAP_NOCACHE);
 }
 
 static int virtio_pci_bind(struct udevice *udev)
-- 
2.35.1.894.gb6a874cedc-goog

[PATCH 11/11] virtio: pci: Allow exclusion of legacy driver

[Andrew Scull]

Add a new config to control whether the driver for legacy virtio PCI
devices is included in the build. VIRTIO_PCI_LEGACY is included by
default when VIRTIO_PCI is selected, but it can also be indepedently
toggled.

Signed-off-by: Andrew Scull <ascull@google.com>
---
 drivers/virtio/Kconfig  | 9 +++++++++
 drivers/virtio/Makefile | 3 ++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index 863c3fbe02..586263ec88 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -37,6 +37,15 @@ config VIRTIO_PCI
 	  This driver provides support for virtio based paravirtual device
 	  drivers over PCI.
 
+config VIRTIO_PCI_LEGACY
+	bool "PCI driver for legacy virtio devices"
+	depends on PCI
+	select VIRTIO
+	default VIRTIO_PCI
+	help
+	  This driver provides support for legacy virtio based paravirtual
+	  device drivers over PCI.
+
 config VIRTIO_SANDBOX
 	bool "Sandbox driver for virtio devices"
 	depends on SANDBOX
diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
index dc8880937a..4c63a6c690 100644
--- a/drivers/virtio/Makefile
+++ b/drivers/virtio/Makefile
@@ -5,7 +5,8 @@
 
 obj-y += virtio-uclass.o virtio_ring.o
 obj-$(CONFIG_VIRTIO_MMIO) += virtio_mmio.o
-obj-$(CONFIG_VIRTIO_PCI) += virtio_pci_legacy.o virtio_pci_modern.o
+obj-$(CONFIG_VIRTIO_PCI) += virtio_pci_modern.o
+obj-$(CONFIG_VIRTIO_PCI_LEGACY) += virtio_pci_legacy.o
 obj-$(CONFIG_VIRTIO_SANDBOX) += virtio_sandbox.o
 obj-$(CONFIG_VIRTIO_NET) += virtio_net.o
 obj-$(CONFIG_VIRTIO_BLK) += virtio_blk.o
-- 
2.35.1.894.gb6a874cedc-goog

Re: [PATCH 01/11] virtio: pci: Fix discovery of device config length

[Bin Meng]

On Sun, Mar 20, 2022 at 7:41 PM Andrew Scull <ascull@google.com> wrote:
>
> The length of the device config was erroneously being taken from the
> notify capability. Correct this by finding the length in the device
> capability.
>

Fixes: 550435edf810 ("virtio: pci: Support non-legacy PCI transport device")

> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  drivers/virtio/virtio_pci_modern.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 02/11] virtio: pci: Bounds check device config access

[Bin Meng]

On Sun, Mar 20, 2022 at 7:41 PM Andrew Scull <ascull@google.com> wrote:
>
> The device config is optional, so check it was present and mapped before
> trying to use the pointer. Bounds violations are an error, not just a
> warning, so bail if the checks fail.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  drivers/virtio/virtio_pci_modern.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 03/11] virtio: pci: Bounds check notification writes

[Bin Meng]

On Sun, Mar 20, 2022 at 7:41 PM Andrew Scull <ascull@google.com> wrote:
>
> Make sure virtio notifications are written within their allocated
> buffer.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  drivers/virtio/virtio_pci_modern.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index bcf9f18997..60bdc53a6d 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -101,6 +101,7 @@
>  struct virtio_pci_priv {
>         struct virtio_pci_common_cfg __iomem *common;
>         void __iomem *notify_base;
> +       u32 notify_len;
>         void __iomem *device;
>         u32 device_len;
>         u32 notify_offset_multiplier;
> @@ -372,12 +373,16 @@ static int virtio_pci_notify(struct udevice *udev, struct virtqueue *vq)
>         /* get offset of notification word for this vq */
>         off = ioread16(&priv->common->queue_notify_off);
>
> +       /* Check the effective offset is in bounds */
> +       off *= priv->notify_offset_multiplier;
> +       if (off > priv->notify_len - sizeof(u16))

This check may not work for devices offering VIRTIO_F_NOTIFICATION_DATA.

> +               return -EIO;
> +
>         /*
>          * We write the queue's selector into the notification register
>          * to signal the other end
>          */
> -       iowrite16(vq->index,
> -                 priv->notify_base + off * priv->notify_offset_multiplier);
> +       iowrite16(vq->index, priv->notify_base + off);
>
>         return 0;
>  }
> @@ -499,6 +504,9 @@ static int virtio_pci_probe(struct udevice *udev)
>                 return -EINVAL;
>         }
>
> +       offset = notify + offsetof(struct virtio_pci_cap, length);
> +       dm_pci_read_config32(udev, offset, &priv->notify_len);
> +
>         /*
>          * Device capability is only mandatory for devices that have
>          * device-specific configuration.

Regards,
Bin

Re: [PATCH 04/11] virtio: pci: Check virtio common config size

[Bin Meng]

On Sun, Mar 20, 2022 at 7:41 PM Andrew Scull <ascull@google.com> wrote:
>
> Check that the common config is at least as large as the struct it is
> expected to contain. Only then is it safe to cast the pointer and be
> safe from out-of-bounds accesses.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  drivers/virtio/virtio_pci_modern.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 05/11] virtio: pci: Check virtio capability is in bounds

[Bin Meng]

On Sun, Mar 20, 2022 at 7:41 PM Andrew Scull <ascull@google.com> wrote:
>
> Ensure the virtio PCI capabilities are contained within the bounds of
> the device's configuration space. The expected size of the capability is
> passed when searching for the capability to enforce this check.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  drivers/virtio/virtio_pci_modern.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 3403ff5cca..4b346be257 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -392,18 +392,30 @@ static int virtio_pci_notify(struct udevice *udev, struct virtqueue *vq)
>   *
>   * @udev:      the transport device
>   * @cfg_type:  the VIRTIO_PCI_CAP_* value we seek
> + * @cap_size:  expected size of the capability
>   *
>   * Return: offset of the configuration structure
>   */
> -static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type)
> +static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> +                                     size_t cap_size)
>  {
>         int pos;
>         int offset;
>         u8 type, bar;
>
> +       if (cap_size < sizeof(struct virtio_pci_cap))
> +               return 0;
> +
> +       if (cap_size > PCI_CFG_SPACE_SIZE)
> +               return 0;
> +

The above 2 checks are not necessary as this helper is local to this
driver, and we know the callers do things correctly.

>         for (pos = dm_pci_find_capability(udev, PCI_CAP_ID_VNDR);
>              pos > 0;
>              pos = dm_pci_find_next_capability(udev, pos, PCI_CAP_ID_VNDR)) {
> +               /* Ensure the capability is within bounds */
> +               if (PCI_CFG_SPACE_SIZE - cap_size < pos)
> +                       return 0;
> +
>                 offset = pos + offsetof(struct virtio_pci_cap, cfg_type);
>                 dm_pci_read_config8(udev, offset, &type);
>                 offset = pos + offsetof(struct virtio_pci_cap, bar);
> @@ -491,7 +503,8 @@ static int virtio_pci_probe(struct udevice *udev)
>         uc_priv->vendor = subvendor;
>
>         /* Check for a common config: if not, use legacy mode (bar 0) */
> -       common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG);
> +       common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG,
> +                                           sizeof(struct virtio_pci_cap));
>         if (!common) {
>                 printf("(%s): leaving for legacy driver\n", udev->name);
>                 return -ENODEV;
> @@ -505,7 +518,8 @@ static int virtio_pci_probe(struct udevice *udev)
>         }
>
>         /* If common is there, notify should be too */
> -       notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG);
> +       notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG,
> +                                           sizeof(struct virtio_pci_notify_cap));
>         if (!notify) {
>                 printf("(%s): missing capabilities %i/%i\n", udev->name,
>                        common, notify);
> @@ -519,7 +533,8 @@ static int virtio_pci_probe(struct udevice *udev)
>          * Device capability is only mandatory for devices that have
>          * device-specific configuration.
>          */
> -       device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG);
> +       device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG,
> +                                           sizeof(struct virtio_pci_cap));
>         if (device) {
>                 offset = device + offsetof(struct virtio_pci_cap, length);
>                 dm_pci_read_config32(udev, offset, &priv->device_len);
> --

Regards,
Bin

Re: [PATCH 03/11] virtio: pci: Bounds check notification writes

[Andrew Scull]

On Thu, 24 Mar 2022 at 14:19, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sun, Mar 20, 2022 at 7:41 PM Andrew Scull <ascull@google.com> wrote:
> >
> > Make sure virtio notifications are written within their allocated
> > buffer.
> >
> > Signed-off-by: Andrew Scull <ascull@google.com>
> > ---
> >  drivers/virtio/virtio_pci_modern.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > index bcf9f18997..60bdc53a6d 100644
> > --- a/drivers/virtio/virtio_pci_modern.c
> > +++ b/drivers/virtio/virtio_pci_modern.c
> > @@ -101,6 +101,7 @@
> >  struct virtio_pci_priv {
> >         struct virtio_pci_common_cfg __iomem *common;
> >         void __iomem *notify_base;
> > +       u32 notify_len;
> >         void __iomem *device;
> >         u32 device_len;
> >         u32 notify_offset_multiplier;
> > @@ -372,12 +373,16 @@ static int virtio_pci_notify(struct udevice *udev, struct virtqueue *vq)
> >         /* get offset of notification word for this vq */
> >         off = ioread16(&priv->common->queue_notify_off);
> >
> > +       /* Check the effective offset is in bounds */
> > +       off *= priv->notify_offset_multiplier;
> > +       if (off > priv->notify_len - sizeof(u16))
>
> This check may not work for devices offering VIRTIO_F_NOTIFICATION_DATA.

Quickly reading up on VIRTIO_F_NOTIFICATION_DATA (Section 2.7.23 of
the virtio v1.1, for my reference), it's a feature that can be
negotiated to allow the driver to pass more details in the
notification.

However, it isn't negotiated by the u-boot drivers and this is the
function that generates the notification so we know it's only going to
write a single 16-bit value. Were VIRTIO_F_NOTIFICATION_DATA support
to be added, this range check would have to be changed as the size of
the notification increases.

Does this match your understanding?

> > +               return -EIO;
> > +
> >         /*
> >          * We write the queue's selector into the notification register
> >          * to signal the other end
> >          */
> > -       iowrite16(vq->index,
> > -                 priv->notify_base + off * priv->notify_offset_multiplier);
> > +       iowrite16(vq->index, priv->notify_base + off);
> >
> >         return 0;
> >  }
> > @@ -499,6 +504,9 @@ static int virtio_pci_probe(struct udevice *udev)
> >                 return -EINVAL;
> >         }
> >
> > +       offset = notify + offsetof(struct virtio_pci_cap, length);
> > +       dm_pci_read_config32(udev, offset, &priv->notify_len);
> > +
> >         /*
> >          * Device capability is only mandatory for devices that have
> >          * device-specific configuration.
>
> Regards,
> Bin

Re: [PATCH 05/11] virtio: pci: Check virtio capability is in bounds

[Andrew Scull]

On Thu, 24 Mar 2022 at 15:24, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sun, Mar 20, 2022 at 7:41 PM Andrew Scull <ascull@google.com> wrote:
> >
> > Ensure the virtio PCI capabilities are contained within the bounds of
> > the device's configuration space. The expected size of the capability is
> > passed when searching for the capability to enforce this check.
> >
> > Signed-off-by: Andrew Scull <ascull@google.com>
> > ---
> >  drivers/virtio/virtio_pci_modern.c | 23 +++++++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > index 3403ff5cca..4b346be257 100644
> > --- a/drivers/virtio/virtio_pci_modern.c
> > +++ b/drivers/virtio/virtio_pci_modern.c
> > @@ -392,18 +392,30 @@ static int virtio_pci_notify(struct udevice *udev, struct virtqueue *vq)
> >   *
> >   * @udev:      the transport device
> >   * @cfg_type:  the VIRTIO_PCI_CAP_* value we seek
> > + * @cap_size:  expected size of the capability
> >   *
> >   * Return: offset of the configuration structure
> >   */
> > -static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type)
> > +static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> > +                                     size_t cap_size)
> >  {
> >         int pos;
> >         int offset;
> >         u8 type, bar;
> >
> > +       if (cap_size < sizeof(struct virtio_pci_cap))
> > +               return 0;
> > +
> > +       if (cap_size > PCI_CFG_SPACE_SIZE)
> > +               return 0;
> > +
>
> The above 2 checks are not necessary as this helper is local to this
> driver, and we know the callers do things correctly.

The checks aren't absolutely necessary but they do help to document
the constraints and catch any future problems. I'm not sure of the
philosophy the u-boot applies but I like the idea of the safeguards
being in place.

> >         for (pos = dm_pci_find_capability(udev, PCI_CAP_ID_VNDR);
> >              pos > 0;
> >              pos = dm_pci_find_next_capability(udev, pos, PCI_CAP_ID_VNDR)) {
> > +               /* Ensure the capability is within bounds */
> > +               if (PCI_CFG_SPACE_SIZE - cap_size < pos)
> > +                       return 0;
> > +
> >                 offset = pos + offsetof(struct virtio_pci_cap, cfg_type);
> >                 dm_pci_read_config8(udev, offset, &type);
> >                 offset = pos + offsetof(struct virtio_pci_cap, bar);
> > @@ -491,7 +503,8 @@ static int virtio_pci_probe(struct udevice *udev)
> >         uc_priv->vendor = subvendor;
> >
> >         /* Check for a common config: if not, use legacy mode (bar 0) */
> > -       common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG);
> > +       common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG,
> > +                                           sizeof(struct virtio_pci_cap));
> >         if (!common) {
> >                 printf("(%s): leaving for legacy driver\n", udev->name);
> >                 return -ENODEV;
> > @@ -505,7 +518,8 @@ static int virtio_pci_probe(struct udevice *udev)
> >         }
> >
> >         /* If common is there, notify should be too */
> > -       notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG);
> > +       notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG,
> > +                                           sizeof(struct virtio_pci_notify_cap));
> >         if (!notify) {
> >                 printf("(%s): missing capabilities %i/%i\n", udev->name,
> >                        common, notify);
> > @@ -519,7 +533,8 @@ static int virtio_pci_probe(struct udevice *udev)
> >          * Device capability is only mandatory for devices that have
> >          * device-specific configuration.
> >          */
> > -       device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG);
> > +       device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG,
> > +                                           sizeof(struct virtio_pci_cap));
> >         if (device) {
> >                 offset = device + offsetof(struct virtio_pci_cap, length);
> >                 dm_pci_read_config32(udev, offset, &priv->device_len);
> > --
>
> Regards,
> Bin

Re: [PATCH 05/11] virtio: pci: Check virtio capability is in bounds

[Bin Meng]

On Fri, Mar 25, 2022 at 12:27 AM Andrew Scull <ascull@google.com> wrote:
>
> On Thu, 24 Mar 2022 at 15:24, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Sun, Mar 20, 2022 at 7:41 PM Andrew Scull <ascull@google.com> wrote:
> > >
> > > Ensure the virtio PCI capabilities are contained within the bounds of
> > > the device's configuration space. The expected size of the capability is
> > > passed when searching for the capability to enforce this check.
> > >
> > > Signed-off-by: Andrew Scull <ascull@google.com>
> > > ---
> > >  drivers/virtio/virtio_pci_modern.c | 23 +++++++++++++++++++----
> > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > > index 3403ff5cca..4b346be257 100644
> > > --- a/drivers/virtio/virtio_pci_modern.c
> > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > @@ -392,18 +392,30 @@ static int virtio_pci_notify(struct udevice *udev, struct virtqueue *vq)
> > >   *
> > >   * @udev:      the transport device
> > >   * @cfg_type:  the VIRTIO_PCI_CAP_* value we seek
> > > + * @cap_size:  expected size of the capability
> > >   *
> > >   * Return: offset of the configuration structure
> > >   */
> > > -static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type)
> > > +static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> > > +                                     size_t cap_size)
> > >  {
> > >         int pos;
> > >         int offset;
> > >         u8 type, bar;
> > >
> > > +       if (cap_size < sizeof(struct virtio_pci_cap))
> > > +               return 0;
> > > +
> > > +       if (cap_size > PCI_CFG_SPACE_SIZE)
> > > +               return 0;
> > > +
> >
> > The above 2 checks are not necessary as this helper is local to this
> > driver, and we know the callers do things correctly.
>
> The checks aren't absolutely necessary but they do help to document
> the constraints and catch any future problems. I'm not sure of the
> philosophy the u-boot applies but I like the idea of the safeguards
> being in place.

I would use assert() for such case.

>
> > >         for (pos = dm_pci_find_capability(udev, PCI_CAP_ID_VNDR);
> > >              pos > 0;
> > >              pos = dm_pci_find_next_capability(udev, pos, PCI_CAP_ID_VNDR)) {
> > > +               /* Ensure the capability is within bounds */
> > > +               if (PCI_CFG_SPACE_SIZE - cap_size < pos)
> > > +                       return 0;
> > > +
> > >                 offset = pos + offsetof(struct virtio_pci_cap, cfg_type);
> > >                 dm_pci_read_config8(udev, offset, &type);
> > >                 offset = pos + offsetof(struct virtio_pci_cap, bar);
> > > @@ -491,7 +503,8 @@ static int virtio_pci_probe(struct udevice *udev)
> > >         uc_priv->vendor = subvendor;
> > >
> > >         /* Check for a common config: if not, use legacy mode (bar 0) */
> > > -       common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG);
> > > +       common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG,
> > > +                                           sizeof(struct virtio_pci_cap));
> > >         if (!common) {
> > >                 printf("(%s): leaving for legacy driver\n", udev->name);
> > >                 return -ENODEV;
> > > @@ -505,7 +518,8 @@ static int virtio_pci_probe(struct udevice *udev)
> > >         }
> > >
> > >         /* If common is there, notify should be too */
> > > -       notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG);
> > > +       notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG,
> > > +                                           sizeof(struct virtio_pci_notify_cap));
> > >         if (!notify) {
> > >                 printf("(%s): missing capabilities %i/%i\n", udev->name,
> > >                        common, notify);
> > > @@ -519,7 +533,8 @@ static int virtio_pci_probe(struct udevice *udev)
> > >          * Device capability is only mandatory for devices that have
> > >          * device-specific configuration.
> > >          */
> > > -       device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG);
> > > +       device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG,
> > > +                                           sizeof(struct virtio_pci_cap));
> > >         if (device) {
> > >                 offset = device + offsetof(struct virtio_pci_cap, length);
> > >                 dm_pci_read_config32(udev, offset, &priv->device_len);
> > > --

Regards,
Bin

Re: [PATCH 03/11] virtio: pci: Bounds check notification writes

[Bin Meng]

On Fri, Mar 25, 2022 at 12:24 AM Andrew Scull <ascull@google.com> wrote:
>
> On Thu, 24 Mar 2022 at 14:19, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Sun, Mar 20, 2022 at 7:41 PM Andrew Scull <ascull@google.com> wrote:
> > >
> > > Make sure virtio notifications are written within their allocated
> > > buffer.
> > >
> > > Signed-off-by: Andrew Scull <ascull@google.com>
> > > ---
> > >  drivers/virtio/virtio_pci_modern.c | 12 ++++++++++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > > index bcf9f18997..60bdc53a6d 100644
> > > --- a/drivers/virtio/virtio_pci_modern.c
> > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > @@ -101,6 +101,7 @@
> > >  struct virtio_pci_priv {
> > >         struct virtio_pci_common_cfg __iomem *common;
> > >         void __iomem *notify_base;
> > > +       u32 notify_len;
> > >         void __iomem *device;
> > >         u32 device_len;
> > >         u32 notify_offset_multiplier;
> > > @@ -372,12 +373,16 @@ static int virtio_pci_notify(struct udevice *udev, struct virtqueue *vq)
> > >         /* get offset of notification word for this vq */
> > >         off = ioread16(&priv->common->queue_notify_off);
> > >
> > > +       /* Check the effective offset is in bounds */
> > > +       off *= priv->notify_offset_multiplier;
> > > +       if (off > priv->notify_len - sizeof(u16))
> >
> > This check may not work for devices offering VIRTIO_F_NOTIFICATION_DATA.
>
> Quickly reading up on VIRTIO_F_NOTIFICATION_DATA (Section 2.7.23 of
> the virtio v1.1, for my reference), it's a feature that can be
> negotiated to allow the driver to pass more details in the
> notification.
>
> However, it isn't negotiated by the u-boot drivers and this is the
> function that generates the notification so we know it's only going to
> write a single 16-bit value. Were VIRTIO_F_NOTIFICATION_DATA support
> to be added, this range check would have to be changed as the size of
> the notification increases.
>
> Does this match your understanding?

Thanks for the pointers. Let's put additional comments there
mentioning that VIRTIO_F_NOTIFICATION_DATA is not used by U-Boot
driver.

>
> > > +               return -EIO;
> > > +
> > >         /*
> > >          * We write the queue's selector into the notification register
> > >          * to signal the other end
> > >          */
> > > -       iowrite16(vq->index,
> > > -                 priv->notify_base + off * priv->notify_offset_multiplier);
> > > +       iowrite16(vq->index, priv->notify_base + off);
> > >
> > >         return 0;
> > >  }
> > > @@ -499,6 +504,9 @@ static int virtio_pci_probe(struct udevice *udev)
> > >                 return -EINVAL;
> > >         }
> > >
> > > +       offset = notify + offsetof(struct virtio_pci_cap, length);
> > > +       dm_pci_read_config32(udev, offset, &priv->notify_len);
> > > +
> > >         /*
> > >          * Device capability is only mandatory for devices that have
> > >          * device-specific configuration.

Otherwise,
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 06/11] virtio: pci: Read entire capability into memory

[Bin Meng]

On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull@google.com> wrote:
>
> Read the virtio PCI capability out of the device configuration space to
> a struct rather than accessing fields directly from the configuration
> space as they are needed. This both makes access to the fields easier
> and avoids re-reading fields.
>
> Re-reading fields could result in time-of-check to time-of-use problems,
> should the value in the configuration space change. The range check of
> the `bar` field and the later call to `dm_pci_read_bar32()` is an
> example of where this could happen.

I don't see the need to avoid the time-of-check to time-of-use
problems, as it can only happen with the PCI configuration access
capability, which U-Boot driver does not touch.

Am I missing something?

>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  drivers/virtio/virtio_pci_modern.c | 74 ++++++++++++++++--------------
>  1 file changed, 40 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 4b346be257..38a0da0a84 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -393,15 +393,16 @@ static int virtio_pci_notify(struct udevice *udev, struct virtqueue *vq)
>   * @udev:      the transport device
>   * @cfg_type:  the VIRTIO_PCI_CAP_* value we seek
>   * @cap_size:  expected size of the capability
> + * @cap:       capability read from the config space
>   *
>   * Return: offset of the configuration structure
>   */
>  static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> -                                     size_t cap_size)
> +                                     size_t cap_size,
> +                                     struct virtio_pci_cap *cap)
>  {
>         int pos;
>         int offset;
> -       u8 type, bar;
>
>         if (cap_size < sizeof(struct virtio_pci_cap))
>                 return 0;
> @@ -409,6 +410,9 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
>         if (cap_size > PCI_CFG_SPACE_SIZE)
>                 return 0;
>
> +       if (!cap)
> +               return 0;
> +
>         for (pos = dm_pci_find_capability(udev, PCI_CAP_ID_VNDR);
>              pos > 0;
>              pos = dm_pci_find_next_capability(udev, pos, PCI_CAP_ID_VNDR)) {
> @@ -416,16 +420,26 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
>                 if (PCI_CFG_SPACE_SIZE - cap_size < pos)
>                         return 0;
>
> +               offset = pos + offsetof(struct virtio_pci_cap, cap_vndr);
> +               dm_pci_read_config8(udev, offset, &cap->cap_vndr);
> +               offset = pos + offsetof(struct virtio_pci_cap, cap_next);
> +               dm_pci_read_config8(udev, offset, &cap->cap_next);
> +               offset = pos + offsetof(struct virtio_pci_cap, cap_len);
> +               dm_pci_read_config8(udev, offset, &cap->cap_len);
>                 offset = pos + offsetof(struct virtio_pci_cap, cfg_type);
> -               dm_pci_read_config8(udev, offset, &type);
> +               dm_pci_read_config8(udev, offset, &cap->cfg_type);
>                 offset = pos + offsetof(struct virtio_pci_cap, bar);
> -               dm_pci_read_config8(udev, offset, &bar);
> +               dm_pci_read_config8(udev, offset, &cap->bar);
> +               offset = pos + offsetof(struct virtio_pci_cap, offset);
> +               dm_pci_read_config32(udev, offset, &cap->offset);
> +               offset = pos + offsetof(struct virtio_pci_cap, length);
> +               dm_pci_read_config32(udev, offset, &cap->length);
>
>                 /* Ignore structures with reserved BAR values */
> -               if (bar > 0x5)
> +               if (cap->bar > 0x5)
>                         continue;
>
> -               if (type == cfg_type)
> +               if (cap->cfg_type == cfg_type)
>                         return pos;
>         }
>
> @@ -436,33 +450,27 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
>   * virtio_pci_map_capability - map base address of the capability
>   *
>   * @udev:      the transport device
> - * @off:       offset of the configuration structure
> + * @cap:       capability to map
>   *
>   * Return: base address of the capability
>   */
> -static void __iomem *virtio_pci_map_capability(struct udevice *udev, int off)
> +static void __iomem *virtio_pci_map_capability(struct udevice *udev,
> +                                              const struct virtio_pci_cap *cap)
>  {
> -       u8 bar;
> -       u32 offset;
>         ulong base;
>         void __iomem *p;
>
> -       if (!off)
> +       if (!cap)
>                 return NULL;
>
> -       offset = off + offsetof(struct virtio_pci_cap, bar);
> -       dm_pci_read_config8(udev, offset, &bar);
> -       offset = off + offsetof(struct virtio_pci_cap, offset);
> -       dm_pci_read_config32(udev, offset, &offset);
> -
>         /*
>          * TODO: adding 64-bit BAR support
>          *
>          * Per spec, the BAR is permitted to be either 32-bit or 64-bit.
>          * For simplicity, only read the BAR address as 32-bit.
>          */
> -       base = dm_pci_read_bar32(udev, bar);
> -       p = (void __iomem *)base + offset;
> +       base = dm_pci_read_bar32(udev, cap->bar);
> +       p = (void __iomem *)base + cap->offset;
>
>         return p;
>  }
> @@ -487,7 +495,7 @@ static int virtio_pci_probe(struct udevice *udev)
>         u16 subvendor;
>         u8 revision;
>         int common, notify, device;
> -       u32 common_length;
> +       struct virtio_pci_cap common_cap, notify_cap, device_cap;
>         int offset;
>
>         /* We only own devices >= 0x1040 and <= 0x107f: leave the rest. */
> @@ -504,46 +512,44 @@ static int virtio_pci_probe(struct udevice *udev)
>
>         /* Check for a common config: if not, use legacy mode (bar 0) */
>         common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG,
> -                                           sizeof(struct virtio_pci_cap));
> +                                           sizeof(struct virtio_pci_cap),
> +                                           &common_cap);
>         if (!common) {
>                 printf("(%s): leaving for legacy driver\n", udev->name);
>                 return -ENODEV;
>         }
>
> -       offset = common + offsetof(struct virtio_pci_cap, length);
> -       dm_pci_read_config32(udev, offset, &common_length);
> -       if (common_length < sizeof(struct virtio_pci_common_cfg)) {
> +       if (common_cap.length < sizeof(struct virtio_pci_common_cfg)) {
>                 printf("(%s): virtio common config too small\n", udev->name);
>                 return -EINVAL;
>         }
>
>         /* If common is there, notify should be too */
>         notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG,
> -                                           sizeof(struct virtio_pci_notify_cap));
> +                                           sizeof(struct virtio_pci_notify_cap),
> +                                           &notify_cap);
>         if (!notify) {
>                 printf("(%s): missing capabilities %i/%i\n", udev->name,
>                        common, notify);
>                 return -EINVAL;
>         }
>
> -       offset = notify + offsetof(struct virtio_pci_cap, length);
> -       dm_pci_read_config32(udev, offset, &priv->notify_len);
> +       priv->notify_len = notify_cap.length;
>
>         /*
>          * Device capability is only mandatory for devices that have
>          * device-specific configuration.
>          */
>         device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG,
> -                                           sizeof(struct virtio_pci_cap));
> -       if (device) {
> -               offset = device + offsetof(struct virtio_pci_cap, length);
> -               dm_pci_read_config32(udev, offset, &priv->device_len);
> -       }
> +                                           sizeof(struct virtio_pci_cap),
> +                                           &device_cap);
> +       if (device)
> +               priv->device_len = device_cap.length;
>
>         /* Map configuration structures */
> -       priv->common = virtio_pci_map_capability(udev, common);
> -       priv->notify_base = virtio_pci_map_capability(udev, notify);
> -       priv->device = virtio_pci_map_capability(udev, device);
> +       priv->common = virtio_pci_map_capability(udev, &common_cap);
> +       priv->notify_base = virtio_pci_map_capability(udev, &notify_cap);
> +       priv->device = virtio_pci_map_capability(udev, &device_cap);
>         debug("(%p): common @ %p, notify base @ %p, device @ %p\n",
>               udev, priv->common, priv->notify_base, priv->device);
>
> --

Regards,
Bin

Re: [PATCH 07/11] virtio: pci: Check virtio configs are mapped

[Bin Meng]

On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull@google.com> wrote:
>
> The calls to `virtio_pci_map_capability()` return NULL on error. If this
> happens, later accesses to the pointers would be unsafe. Avoid this by
> failing if the configs were not successfully mapped.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  drivers/virtio/virtio_pci_modern.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 38a0da0a84..2f1a1cedbc 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -534,7 +534,19 @@ static int virtio_pci_probe(struct udevice *udev)
>                 return -EINVAL;
>         }
>
> +       /* Map configuration structures */
> +       priv->common = virtio_pci_map_capability(udev, &common_cap);
> +       if (!priv->common) {

This can't be NULL, as you did not pass a NULL capability pointer

> +               printf("(%s): could not map common config\n", udev->name);
> +               return -EINVAL;
> +       }
> +
>         priv->notify_len = notify_cap.length;
> +       priv->notify_base = virtio_pci_map_capability(udev, &notify_cap);
> +       if (!priv->notify_base) {
> +               printf("(%s): could not map notify config\n", udev->name);
> +               return -EINVAL;
> +       }
>
>         /*
>          * Device capability is only mandatory for devices that have
> @@ -543,13 +555,16 @@ static int virtio_pci_probe(struct udevice *udev)
>         device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG,
>                                             sizeof(struct virtio_pci_cap),
>                                             &device_cap);
> -       if (device)
> +       if (device) {
>                 priv->device_len = device_cap.length;
> +               priv->device = virtio_pci_map_capability(udev, &device_cap);
> +               if (!priv->device) {
> +                       printf("(%s): could not map device config\n",
> +                              udev->name);
> +                       return -EINVAL;
> +               }
> +       }
>
> -       /* Map configuration structures */
> -       priv->common = virtio_pci_map_capability(udev, &common_cap);
> -       priv->notify_base = virtio_pci_map_capability(udev, &notify_cap);
> -       priv->device = virtio_pci_map_capability(udev, &device_cap);
>         debug("(%p): common @ %p, notify base @ %p, device @ %p\n",
>               udev, priv->common, priv->notify_base, priv->device);
>

I don't think adding these checks is necessary.

Regards,
Bin

Re: [PATCH 06/11] virtio: pci: Read entire capability into memory

[Andrew Scull]

On Fri, 25 Mar 2022 at 04:31, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull@google.com> wrote:
> >
> > Read the virtio PCI capability out of the device configuration space to
> > a struct rather than accessing fields directly from the configuration
> > space as they are needed. This both makes access to the fields easier
> > and avoids re-reading fields.
> >
> > Re-reading fields could result in time-of-check to time-of-use problems,
> > should the value in the configuration space change. The range check of
> > the `bar` field and the later call to `dm_pci_read_bar32()` is an
> > example of where this could happen.
>
> I don't see the need to avoid the time-of-check to time-of-use
> problems, as it can only happen with the PCI configuration access
> capability, which U-Boot driver does not touch.
>
> Am I missing something?

U-Boot doesn't touch the configuration space but the device could
have, whether that be accidently or maliciously. Linux has taken
similar precautions [1] to add more safety checks and I'll be looking
to do the same in other parts of the u-boot virtio drivers.

[1] -- https://lwn.net/Articles/865216

> >
> > Signed-off-by: Andrew Scull <ascull@google.com>
> > ---
> >  drivers/virtio/virtio_pci_modern.c | 74 ++++++++++++++++--------------
> >  1 file changed, 40 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > index 4b346be257..38a0da0a84 100644
> > --- a/drivers/virtio/virtio_pci_modern.c
> > +++ b/drivers/virtio/virtio_pci_modern.c
> > @@ -393,15 +393,16 @@ static int virtio_pci_notify(struct udevice *udev, struct virtqueue *vq)
> >   * @udev:      the transport device
> >   * @cfg_type:  the VIRTIO_PCI_CAP_* value we seek
> >   * @cap_size:  expected size of the capability
> > + * @cap:       capability read from the config space
> >   *
> >   * Return: offset of the configuration structure
> >   */
> >  static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> > -                                     size_t cap_size)
> > +                                     size_t cap_size,
> > +                                     struct virtio_pci_cap *cap)
> >  {
> >         int pos;
> >         int offset;
> > -       u8 type, bar;
> >
> >         if (cap_size < sizeof(struct virtio_pci_cap))
> >                 return 0;
> > @@ -409,6 +410,9 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> >         if (cap_size > PCI_CFG_SPACE_SIZE)
> >                 return 0;
> >
> > +       if (!cap)
> > +               return 0;
> > +
> >         for (pos = dm_pci_find_capability(udev, PCI_CAP_ID_VNDR);
> >              pos > 0;
> >              pos = dm_pci_find_next_capability(udev, pos, PCI_CAP_ID_VNDR)) {
> > @@ -416,16 +420,26 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> >                 if (PCI_CFG_SPACE_SIZE - cap_size < pos)
> >                         return 0;
> >
> > +               offset = pos + offsetof(struct virtio_pci_cap, cap_vndr);
> > +               dm_pci_read_config8(udev, offset, &cap->cap_vndr);
> > +               offset = pos + offsetof(struct virtio_pci_cap, cap_next);
> > +               dm_pci_read_config8(udev, offset, &cap->cap_next);
> > +               offset = pos + offsetof(struct virtio_pci_cap, cap_len);
> > +               dm_pci_read_config8(udev, offset, &cap->cap_len);
> >                 offset = pos + offsetof(struct virtio_pci_cap, cfg_type);
> > -               dm_pci_read_config8(udev, offset, &type);
> > +               dm_pci_read_config8(udev, offset, &cap->cfg_type);
> >                 offset = pos + offsetof(struct virtio_pci_cap, bar);
> > -               dm_pci_read_config8(udev, offset, &bar);
> > +               dm_pci_read_config8(udev, offset, &cap->bar);
> > +               offset = pos + offsetof(struct virtio_pci_cap, offset);
> > +               dm_pci_read_config32(udev, offset, &cap->offset);
> > +               offset = pos + offsetof(struct virtio_pci_cap, length);
> > +               dm_pci_read_config32(udev, offset, &cap->length);
> >
> >                 /* Ignore structures with reserved BAR values */
> > -               if (bar > 0x5)
> > +               if (cap->bar > 0x5)
> >                         continue;
> >
> > -               if (type == cfg_type)
> > +               if (cap->cfg_type == cfg_type)
> >                         return pos;
> >         }
> >
> > @@ -436,33 +450,27 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> >   * virtio_pci_map_capability - map base address of the capability
> >   *
> >   * @udev:      the transport device
> > - * @off:       offset of the configuration structure
> > + * @cap:       capability to map
> >   *
> >   * Return: base address of the capability
> >   */
> > -static void __iomem *virtio_pci_map_capability(struct udevice *udev, int off)
> > +static void __iomem *virtio_pci_map_capability(struct udevice *udev,
> > +                                              const struct virtio_pci_cap *cap)
> >  {
> > -       u8 bar;
> > -       u32 offset;
> >         ulong base;
> >         void __iomem *p;
> >
> > -       if (!off)
> > +       if (!cap)
> >                 return NULL;
> >
> > -       offset = off + offsetof(struct virtio_pci_cap, bar);
> > -       dm_pci_read_config8(udev, offset, &bar);
> > -       offset = off + offsetof(struct virtio_pci_cap, offset);
> > -       dm_pci_read_config32(udev, offset, &offset);
> > -
> >         /*
> >          * TODO: adding 64-bit BAR support
> >          *
> >          * Per spec, the BAR is permitted to be either 32-bit or 64-bit.
> >          * For simplicity, only read the BAR address as 32-bit.
> >          */
> > -       base = dm_pci_read_bar32(udev, bar);
> > -       p = (void __iomem *)base + offset;
> > +       base = dm_pci_read_bar32(udev, cap->bar);
> > +       p = (void __iomem *)base + cap->offset;
> >
> >         return p;
> >  }
> > @@ -487,7 +495,7 @@ static int virtio_pci_probe(struct udevice *udev)
> >         u16 subvendor;
> >         u8 revision;
> >         int common, notify, device;
> > -       u32 common_length;
> > +       struct virtio_pci_cap common_cap, notify_cap, device_cap;
> >         int offset;
> >
> >         /* We only own devices >= 0x1040 and <= 0x107f: leave the rest. */
> > @@ -504,46 +512,44 @@ static int virtio_pci_probe(struct udevice *udev)
> >
> >         /* Check for a common config: if not, use legacy mode (bar 0) */
> >         common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG,
> > -                                           sizeof(struct virtio_pci_cap));
> > +                                           sizeof(struct virtio_pci_cap),
> > +                                           &common_cap);
> >         if (!common) {
> >                 printf("(%s): leaving for legacy driver\n", udev->name);
> >                 return -ENODEV;
> >         }
> >
> > -       offset = common + offsetof(struct virtio_pci_cap, length);
> > -       dm_pci_read_config32(udev, offset, &common_length);
> > -       if (common_length < sizeof(struct virtio_pci_common_cfg)) {
> > +       if (common_cap.length < sizeof(struct virtio_pci_common_cfg)) {
> >                 printf("(%s): virtio common config too small\n", udev->name);
> >                 return -EINVAL;
> >         }
> >
> >         /* If common is there, notify should be too */
> >         notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG,
> > -                                           sizeof(struct virtio_pci_notify_cap));
> > +                                           sizeof(struct virtio_pci_notify_cap),
> > +                                           &notify_cap);
> >         if (!notify) {
> >                 printf("(%s): missing capabilities %i/%i\n", udev->name,
> >                        common, notify);
> >                 return -EINVAL;
> >         }
> >
> > -       offset = notify + offsetof(struct virtio_pci_cap, length);
> > -       dm_pci_read_config32(udev, offset, &priv->notify_len);
> > +       priv->notify_len = notify_cap.length;
> >
> >         /*
> >          * Device capability is only mandatory for devices that have
> >          * device-specific configuration.
> >          */
> >         device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG,
> > -                                           sizeof(struct virtio_pci_cap));
> > -       if (device) {
> > -               offset = device + offsetof(struct virtio_pci_cap, length);
> > -               dm_pci_read_config32(udev, offset, &priv->device_len);
> > -       }
> > +                                           sizeof(struct virtio_pci_cap),
> > +                                           &device_cap);
> > +       if (device)
> > +               priv->device_len = device_cap.length;
> >
> >         /* Map configuration structures */
> > -       priv->common = virtio_pci_map_capability(udev, common);
> > -       priv->notify_base = virtio_pci_map_capability(udev, notify);
> > -       priv->device = virtio_pci_map_capability(udev, device);
> > +       priv->common = virtio_pci_map_capability(udev, &common_cap);
> > +       priv->notify_base = virtio_pci_map_capability(udev, &notify_cap);
> > +       priv->device = virtio_pci_map_capability(udev, &device_cap);
> >         debug("(%p): common @ %p, notify base @ %p, device @ %p\n",
> >               udev, priv->common, priv->notify_base, priv->device);
> >
> > --
>
> Regards,
> Bin

Re: [PATCH 07/11] virtio: pci: Check virtio configs are mapped

[Andrew Scull]

On Fri, 25 Mar 2022 at 04:38, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull@google.com> wrote:
> >
> > The calls to `virtio_pci_map_capability()` return NULL on error. If this
> > happens, later accesses to the pointers would be unsafe. Avoid this by
> > failing if the configs were not successfully mapped.
> >
> > Signed-off-by: Andrew Scull <ascull@google.com>
> > ---
> >  drivers/virtio/virtio_pci_modern.c | 25 ++++++++++++++++++++-----
> >  1 file changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > index 38a0da0a84..2f1a1cedbc 100644
> > --- a/drivers/virtio/virtio_pci_modern.c
> > +++ b/drivers/virtio/virtio_pci_modern.c
> > @@ -534,7 +534,19 @@ static int virtio_pci_probe(struct udevice *udev)
> >                 return -EINVAL;
> >         }
> >
> > +       /* Map configuration structures */
> > +       priv->common = virtio_pci_map_capability(udev, &common_cap);
> > +       if (!priv->common) {
>
> This can't be NULL, as you did not pass a NULL capability pointer
>
> > +               printf("(%s): could not map common config\n", udev->name);
> > +               return -EINVAL;
> > +       }
> > +
> >         priv->notify_len = notify_cap.length;
> > +       priv->notify_base = virtio_pci_map_capability(udev, &notify_cap);
> > +       if (!priv->notify_base) {
> > +               printf("(%s): could not map notify config\n", udev->name);
> > +               return -EINVAL;
> > +       }
> >
> >         /*
> >          * Device capability is only mandatory for devices that have
> > @@ -543,13 +555,16 @@ static int virtio_pci_probe(struct udevice *udev)
> >         device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG,
> >                                             sizeof(struct virtio_pci_cap),
> >                                             &device_cap);
> > -       if (device)
> > +       if (device) {
> >                 priv->device_len = device_cap.length;
> > +               priv->device = virtio_pci_map_capability(udev, &device_cap);
> > +               if (!priv->device) {
> > +                       printf("(%s): could not map device config\n",
> > +                              udev->name);
> > +                       return -EINVAL;
> > +               }
> > +       }
> >
> > -       /* Map configuration structures */
> > -       priv->common = virtio_pci_map_capability(udev, &common_cap);
> > -       priv->notify_base = virtio_pci_map_capability(udev, &notify_cap);
> > -       priv->device = virtio_pci_map_capability(udev, &device_cap);
> >         debug("(%p): common @ %p, notify base @ %p, device @ %p\n",
> >               udev, priv->common, priv->notify_base, priv->device);
> >
>
> I don't think adding these checks is necessary.

See later patches in the series that validate the address range is
within the declared PCI ranges and not an arbitrary range chosen,
accidently or maliciously, by the device. If those checks fail, the
memory will not have been mapped and the probe should fail.

> Regards,
> Bin

Re: [PATCH 08/11] pci: Check region ranges are addressable

[Bin Meng]

On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull@google.com> wrote:
>
> When parsing the `ranges` DT node, check that both extremes of the
> regions are addressable without overflow. This assumption can then be
> safely made when processing the regions.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  drivers/pci/pci-uclass.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 09/11] pci: Add function to validate PCI address range

[Bin Meng]

On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull@google.com> wrote:
>
> Add a function to convert a PCI address range to a physical address
> range. The address range is validated to ensure it is contained within
> one of the PCI address regions and that the whole range can be indexed
> from the resulting physical address.

I am not sure if we need to provide such an API given there is already
dm_pci_bus_to_phys() and friends. Not sure we really need this new API
to add a check against the range.

>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  drivers/pci/pci-uclass.c | 30 ++++++++++++++++++++++++++++++
>  include/pci.h            | 16 ++++++++++++++++
>  2 files changed, 46 insertions(+)
>
> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> index 54a05d7567..efab8916e2 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -1452,6 +1452,36 @@ phys_addr_t dm_pci_bus_to_phys(struct udevice *dev, pci_addr_t bus_addr,
>         return phys_addr;
>  }
>
> +phys_addr_t dm_pci_bus_range_to_phys(struct udevice *dev, pci_addr_t bus_addr,
> +                                    size_t size, unsigned long mask,
> +                                    unsigned long flags)
> +{
> +       struct udevice *ctlr = pci_get_controller(dev);
> +       struct pci_controller *hose = dev_get_uclass_priv(ctlr);
> +       struct pci_region *res;
> +       size_t offset;
> +       int i;
> +
> +       for (i = 0; i < hose->region_count; i++) {
> +               res = &hose->regions[i];
> +
> +               if ((res->flags & mask) != flags)
> +                       continue;
> +
> +               if (bus_addr < res->bus_start)
> +                       continue;
> +
> +               offset = bus_addr - res->bus_start;
> +               if (offset >= res->size || size > res->size - offset)
> +                       continue;
> +
> +               return res->phys_start + offset;
> +       }
> +
> +       puts("pci_bus_range_to_phys: invalid address range\n");
> +       return 0;
> +}

If we really need this, I believe we can do some refactoring on the
existing _dm_pci_bus_to_phys() to provide the new capability you
wanted.

> +
>  static int _dm_pci_phys_to_bus(struct udevice *dev, phys_addr_t phys_addr,
>                                unsigned long flags, unsigned long skip_mask,
>                                pci_addr_t *ba)
> diff --git a/include/pci.h b/include/pci.h
> index 673c95c6bb..15b3031c1f 100644
> --- a/include/pci.h
> +++ b/include/pci.h
> @@ -1446,6 +1446,22 @@ u32 dm_pci_read_bar32(const struct udevice *dev, int barnum);
>  phys_addr_t dm_pci_bus_to_phys(struct udevice *dev, pci_addr_t addr,
>                                unsigned long flags);
>
> +/**
> + * dm_pci_bus_range_to_phys() - convert a PCI bus address range to a physical
> + *                             address.
> + *
> + * @dev:       Device containing the PCI address
> + * @addr:      PCI address to convert
> + * @size:      Size of the addressrange

need a space before 'range'

> + * @mask:      Mask of flags to match
> + * @flags:     Flags for the region type (PCI_REGION_...)
> + * Return: physical address corresponding to that PCI bus address range or 0 if
> + *        the range could not be converted
> + */
> +phys_addr_t dm_pci_bus_range_to_phys(struct udevice *dev, pci_addr_t bus_addr,
> +                                    size_t size, unsigned long mask,
> +                                    unsigned long flags);
> +
>  /**
>   * dm_pci_phys_to_bus() - convert a physical address to a PCI bus address
>   *

Please add a test case in test/dm/pci.c to cover this new API.

Regards,
Bin

Re: [PATCH 10/11] virtio: pci: Check mapped range is in a PCI region

[Bin Meng]

On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull@google.com> wrote:
>
> The virtio PCI capabilities describe a region of memory that should be
> mapped. Ensure those are valid PCI regions before accessing them.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  drivers/virtio/virtio_pci_modern.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 2f1a1cedbc..84750e2b27 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -457,8 +457,9 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
>  static void __iomem *virtio_pci_map_capability(struct udevice *udev,
>                                                const struct virtio_pci_cap *cap)
>  {
> -       ulong base;
> -       void __iomem *p;
> +       unsigned long mask, flags;
> +       phys_addr_t phys_addr;
> +       u32 base;
>
>         if (!cap)
>                 return NULL;
> @@ -470,9 +471,23 @@ static void __iomem *virtio_pci_map_capability(struct udevice *udev,
>          * For simplicity, only read the BAR address as 32-bit.
>          */
>         base = dm_pci_read_bar32(udev, cap->bar);
> -       p = (void __iomem *)base + cap->offset;
>
> -       return p;
> +       if (U32_MAX - base < cap->offset)
> +               return NULL;
> +       base += cap->offset;
> +
> +       if (U32_MAX - base < cap->length)
> +               return NULL;
> +
> +       /* Find the corresponding memory region that isn't system memory. */
> +       mask = PCI_REGION_TYPE | PCI_REGION_SYS_MEMORY;
> +       flags = PCI_REGION_MEM;
> +       phys_addr = dm_pci_bus_range_to_phys(dev_get_parent(udev), base,
> +                                            cap->length, mask, flags);
> +       if (!phys_addr)
> +               return NULL;
> +
> +       return (void __iomem *)map_physmem(phys_addr, cap->length, MAP_NOCACHE);

Could we use dm_pci_map_bar() instead?

I understand the way the current patch does is to add some sanity
checks against buggy virtio device implementations, but is it really
necessary, or can we move such check somewhere else, like in the
probe()?

>  }
>
>  static int virtio_pci_bind(struct udevice *udev)
> --

Regards,
Bin

Re: [PATCH 11/11] virtio: pci: Allow exclusion of legacy driver

[Bin Meng]

On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull@google.com> wrote:
>
> Add a new config to control whether the driver for legacy virtio PCI
> devices is included in the build. VIRTIO_PCI_LEGACY is included by
> default when VIRTIO_PCI is selected, but it can also be indepedently

typo: independently

> toggled.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
>  drivers/virtio/Kconfig  | 9 +++++++++
>  drivers/virtio/Makefile | 3 ++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
>

Otherwise,
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH 07/11] virtio: pci: Check virtio configs are mapped

[Bin Meng]

On Fri, Mar 25, 2022 at 3:07 PM Andrew Scull <ascull@google.com> wrote:
>
> On Fri, 25 Mar 2022 at 04:38, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull@google.com> wrote:
> > >
> > > The calls to `virtio_pci_map_capability()` return NULL on error. If this
> > > happens, later accesses to the pointers would be unsafe. Avoid this by
> > > failing if the configs were not successfully mapped.
> > >
> > > Signed-off-by: Andrew Scull <ascull@google.com>
> > > ---
> > >  drivers/virtio/virtio_pci_modern.c | 25 ++++++++++++++++++++-----
> > >  1 file changed, 20 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > > index 38a0da0a84..2f1a1cedbc 100644
> > > --- a/drivers/virtio/virtio_pci_modern.c
> > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > @@ -534,7 +534,19 @@ static int virtio_pci_probe(struct udevice *udev)
> > >                 return -EINVAL;
> > >         }
> > >
> > > +       /* Map configuration structures */
> > > +       priv->common = virtio_pci_map_capability(udev, &common_cap);
> > > +       if (!priv->common) {
> >
> > This can't be NULL, as you did not pass a NULL capability pointer
> >
> > > +               printf("(%s): could not map common config\n", udev->name);
> > > +               return -EINVAL;
> > > +       }
> > > +
> > >         priv->notify_len = notify_cap.length;
> > > +       priv->notify_base = virtio_pci_map_capability(udev, &notify_cap);
> > > +       if (!priv->notify_base) {
> > > +               printf("(%s): could not map notify config\n", udev->name);
> > > +               return -EINVAL;
> > > +       }
> > >
> > >         /*
> > >          * Device capability is only mandatory for devices that have
> > > @@ -543,13 +555,16 @@ static int virtio_pci_probe(struct udevice *udev)
> > >         device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG,
> > >                                             sizeof(struct virtio_pci_cap),
> > >                                             &device_cap);
> > > -       if (device)
> > > +       if (device) {
> > >                 priv->device_len = device_cap.length;
> > > +               priv->device = virtio_pci_map_capability(udev, &device_cap);
> > > +               if (!priv->device) {
> > > +                       printf("(%s): could not map device config\n",
> > > +                              udev->name);
> > > +                       return -EINVAL;
> > > +               }
> > > +       }
> > >
> > > -       /* Map configuration structures */
> > > -       priv->common = virtio_pci_map_capability(udev, &common_cap);
> > > -       priv->notify_base = virtio_pci_map_capability(udev, &notify_cap);
> > > -       priv->device = virtio_pci_map_capability(udev, &device_cap);
> > >         debug("(%p): common @ %p, notify base @ %p, device @ %p\n",
> > >               udev, priv->common, priv->notify_base, priv->device);
> > >
> >
> > I don't think adding these checks is necessary.
>
> See later patches in the series that validate the address range is
> within the declared PCI ranges and not an arbitrary range chosen,
> accidently or maliciously, by the device. If those checks fail, the
> memory will not have been mapped and the probe should fail.

Yep, I see additional checks being added in patch 10, so the patch
order should be adjusted to let this patch come later.

Regards,
Bin

Re: [PATCH 06/11] virtio: pci: Read entire capability into memory

[Bin Meng]

On Fri, Mar 25, 2022 at 3:03 PM Andrew Scull <ascull@google.com> wrote:
>
> On Fri, 25 Mar 2022 at 04:31, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull@google.com> wrote:
> > >
> > > Read the virtio PCI capability out of the device configuration space to
> > > a struct rather than accessing fields directly from the configuration
> > > space as they are needed. This both makes access to the fields easier
> > > and avoids re-reading fields.
> > >
> > > Re-reading fields could result in time-of-check to time-of-use problems,
> > > should the value in the configuration space change. The range check of
> > > the `bar` field and the later call to `dm_pci_read_bar32()` is an
> > > example of where this could happen.
> >
> > I don't see the need to avoid the time-of-check to time-of-use
> > problems, as it can only happen with the PCI configuration access
> > capability, which U-Boot driver does not touch.
> >
> > Am I missing something?
>
> U-Boot doesn't touch the configuration space but the device could
> have, whether that be accidently or maliciously. Linux has taken
> similar precautions [1] to add more safety checks and I'll be looking
> to do the same in other parts of the u-boot virtio drivers.
>
> [1] -- https://lwn.net/Articles/865216
>

Got it. So basically the problem is that we don't trust the host that
implements the virtio device :)

I am curious that under such a guideline, probably lots of device
drivers need to be enhanced to do the sanity check, no?

Regards,
Bin

Re: [PATCH 06/11] virtio: pci: Read entire capability into memory

[Andrew Scull]

On Fri, 25 Mar 2022 at 07:51, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, Mar 25, 2022 at 3:03 PM Andrew Scull <ascull@google.com> wrote:
> >
> > On Fri, 25 Mar 2022 at 04:31, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull@google.com> wrote:
> > > >
> > > > Read the virtio PCI capability out of the device configuration space to
> > > > a struct rather than accessing fields directly from the configuration
> > > > space as they are needed. This both makes access to the fields easier
> > > > and avoids re-reading fields.
> > > >
> > > > Re-reading fields could result in time-of-check to time-of-use problems,
> > > > should the value in the configuration space change. The range check of
> > > > the `bar` field and the later call to `dm_pci_read_bar32()` is an
> > > > example of where this could happen.
> > >
> > > I don't see the need to avoid the time-of-check to time-of-use
> > > problems, as it can only happen with the PCI configuration access
> > > capability, which U-Boot driver does not touch.
> > >
> > > Am I missing something?
> >
> > U-Boot doesn't touch the configuration space but the device could
> > have, whether that be accidently or maliciously. Linux has taken
> > similar precautions [1] to add more safety checks and I'll be looking
> > to do the same in other parts of the u-boot virtio drivers.
> >
> > [1] -- https://lwn.net/Articles/865216
> >
>
> Got it. So basically the problem is that we don't trust the host that
> implements the virtio device :)
>
> I am curious that under such a guideline, probably lots of device
> drivers need to be enhanced to do the sanity check, no?

Absolutely, they do! My focus is going to be primarily on the modern
PCI driver, vring and block driver. This will lay a foundation for the
others but they will also need to be checked over carefully before
being relied on.

Re: [PATCH 06/11] virtio: pci: Read entire capability into memory

[Bin Meng]

On Fri, Mar 25, 2022 at 5:18 PM Andrew Scull <ascull@google.com> wrote:
>
> On Fri, 25 Mar 2022 at 07:51, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Fri, Mar 25, 2022 at 3:03 PM Andrew Scull <ascull@google.com> wrote:
> > >
> > > On Fri, 25 Mar 2022 at 04:31, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > >
> > > > On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull@google.com> wrote:
> > > > >
> > > > > Read the virtio PCI capability out of the device configuration space to
> > > > > a struct rather than accessing fields directly from the configuration
> > > > > space as they are needed. This both makes access to the fields easier
> > > > > and avoids re-reading fields.
> > > > >
> > > > > Re-reading fields could result in time-of-check to time-of-use problems,
> > > > > should the value in the configuration space change. The range check of
> > > > > the `bar` field and the later call to `dm_pci_read_bar32()` is an
> > > > > example of where this could happen.
> > > >
> > > > I don't see the need to avoid the time-of-check to time-of-use
> > > > problems, as it can only happen with the PCI configuration access
> > > > capability, which U-Boot driver does not touch.
> > > >
> > > > Am I missing something?
> > >
> > > U-Boot doesn't touch the configuration space but the device could
> > > have, whether that be accidently or maliciously. Linux has taken
> > > similar precautions [1] to add more safety checks and I'll be looking
> > > to do the same in other parts of the u-boot virtio drivers.
> > >
> > > [1] -- https://lwn.net/Articles/865216
> > >
> >
> > Got it. So basically the problem is that we don't trust the host that
> > implements the virtio device :)
> >
> > I am curious that under such a guideline, probably lots of device
> > drivers need to be enhanced to do the sanity check, no?
>
> Absolutely, they do! My focus is going to be primarily on the modern
> PCI driver, vring and block driver. This will lay a foundation for the
> others but they will also need to be checked over carefully before
> being relied on.

Do you know if the Linux kernel has a plan to apply such a guideline
to some other drivers than virtio, like usb or nvme? There are devices
that can do bad things too so we should not trust them?

Regards,
Bin

Re: [PATCH 09/11] pci: Add function to validate PCI address range

[Andrew Scull]

On Fri, 25 Mar 2022 at 07:14, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull@google.com> wrote:
> >
> > Add a function to convert a PCI address range to a physical address
> > range. The address range is validated to ensure it is contained within
> > one of the PCI address regions and that the whole range can be indexed
> > from the resulting physical address.
>
> I am not sure if we need to provide such an API given there is already
> dm_pci_bus_to_phys() and friends. Not sure we really need this new API
> to add a check against the range.
...
> If we really need this, I believe we can do some refactoring on the
> existing _dm_pci_bus_to_phys() to provide the new capability you
> wanted.

A big reason is to check the range fits in the region, not just a
single pointer. It looks like evolving the existing API might not be
too tricky for that.

The bit that really caused me trouble was that the PCI driver adds
system memory as identity mapped regions. But that system memory
hasn't been declared as part of the PCI bus so why is it being added?
I want to remove that but don't know the background, or have the
ability to test PCI will still work properly.

We can't allow a virtio device to claim it has buffers in the middle
of system memory and then have the driver blindly corrupt that region
of memory, so there'll need to be a way to prevent that.

> > + * @size:      Size of the addressrange
>
> need a space before 'range'

Ack.

> Please add a test case in test/dm/pci.c to cover this new API.

I was just learning about the sandbox testing, I'll take a look.

Re: [PATCH 06/11] virtio: pci: Read entire capability into memory

[Andrew Scull]

On Fri, 25 Mar 2022 at 10:26, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, Mar 25, 2022 at 5:18 PM Andrew Scull <ascull@google.com> wrote:
> >
> > On Fri, 25 Mar 2022 at 07:51, Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > On Fri, Mar 25, 2022 at 3:03 PM Andrew Scull <ascull@google.com> wrote:
> > > >
> > > > On Fri, 25 Mar 2022 at 04:31, Bin Meng <bmeng.cn@gmail.com> wrote:
> > > > >
> > > > > On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull@google.com> wrote:
> > > > > >
> > > > > > Read the virtio PCI capability out of the device configuration space to
> > > > > > a struct rather than accessing fields directly from the configuration
> > > > > > space as they are needed. This both makes access to the fields easier
> > > > > > and avoids re-reading fields.
> > > > > >
> > > > > > Re-reading fields could result in time-of-check to time-of-use problems,
> > > > > > should the value in the configuration space change. The range check of
> > > > > > the `bar` field and the later call to `dm_pci_read_bar32()` is an
> > > > > > example of where this could happen.
> > > > >
> > > > > I don't see the need to avoid the time-of-check to time-of-use
> > > > > problems, as it can only happen with the PCI configuration access
> > > > > capability, which U-Boot driver does not touch.
> > > > >
> > > > > Am I missing something?
> > > >
> > > > U-Boot doesn't touch the configuration space but the device could
> > > > have, whether that be accidently or maliciously. Linux has taken
> > > > similar precautions [1] to add more safety checks and I'll be looking
> > > > to do the same in other parts of the u-boot virtio drivers.
> > > >
> > > > [1] -- https://lwn.net/Articles/865216
> > > >
> > >
> > > Got it. So basically the problem is that we don't trust the host that
> > > implements the virtio device :)
> > >
> > > I am curious that under such a guideline, probably lots of device
> > > drivers need to be enhanced to do the sanity check, no?
> >
> > Absolutely, they do! My focus is going to be primarily on the modern
> > PCI driver, vring and block driver. This will lay a foundation for the
> > others but they will also need to be checked over carefully before
> > being relied on.
>
> Do you know if the Linux kernel has a plan to apply such a guideline
> to some other drivers than virtio, like usb or nvme? There are devices
> that can do bad things too so we should not trust them?

I don't know about those other subsystems, but I do know that virtio
has made assumptions about the other side being necessarily trusted
and that those assumptions are being broken by new applications, which
is leading to things being reworked.