[PATCH] iommu/virtio: reject short event buffers

[PATCH] iommu/virtio: reject short event buffers

[raoxu]

From: Xu Rao <raoxu@uniontech.com>

The event queue uses fixed-size buffers for struct viommu_event. The
device-reported used length is currently only checked for oversized
buffers, so a short used buffer can still be passed to the fault
handler.

In that case the handler parses fields that were not written by the
device for this event, potentially using stale data from a previous
event and reporting a bogus fault.

Reject any event buffer whose used length is not exactly the expected
event size. Still recycle the buffer back to the event queue so a bad
event does not permanently shrink the queue.

Signed-off-by: Xu Rao <raoxu@uniontech.com>
---
 drivers/iommu/virtio-iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 587fc13197f1..3ace4a6dd02a 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -635,7 +635,7 @@ static void viommu_event_handler(struct virtqueue *vq)
 	struct viommu_dev *viommu = vq->vdev->priv;

 	while ((evt = virtqueue_get_buf(vq, &len)) != NULL) {
-		if (len > sizeof(*evt)) {
+		if (len != sizeof(*evt)) {
 			dev_err(viommu->dev,
 				"invalid event buffer (len %u != %zu)\n",
 				len, sizeof(*evt));
--
2.50.1

Re: [PATCH] iommu/virtio: reject short event buffers

[Jean-Philippe Brucker]

On Mon, Jul 06, 2026 at 04:31:48PM +0800, raoxu wrote:
> From: Xu Rao <raoxu@uniontech.com>
> 
> The event queue uses fixed-size buffers for struct viommu_event. The
> device-reported used length is currently only checked for oversized
> buffers, so a short used buffer can still be passed to the fault
> handler.

Thank you for the patch. The buffer is allowed to be smaller than struct
virtio_iommu_fault, because valid fields depend on flags:

	struct virtio_iommu_fault {
		u8 reason;
		u8 reserved[3];
		le32 flags;
		le32 endpoint;
		le32 reserved1;
		le64 address;
	};
	
	#define VIRTIO_IOMMU_FAULT_F_READ	(1 << 0)
	#define VIRTIO_IOMMU_FAULT_F_WRITE      (1 << 1)
	#define VIRTIO_IOMMU_FAULT_F_ADDRESS    (1 << 8)

If F_ADDRESS isn't set, the address field is invalid. The spec (Virtio v1.3)
says in 5.13.6.9.2 "Device Requirements: Fault reporting"

  "The device MAY omit setting VIRTIO_IOMMU_FAULT_F_ADDRESS and writing
   address in any fault report, regardless of the reason."

I think the current check aims to catch newer implementations that would
use an extended virtio_iommu_fault to report recoverable faults, but I'm
not sure anymore.

Overall the driver assumes that the IOMMU device is well behaved. If we
want to add sanity checks, maybe viommu_fault_handler() should check len
depending on flags. But I suspect more work is needed if we change the
security model to not trust the device.

Thanks,
Jean

> 
> In that case the handler parses fields that were not written by the
> device for this event, potentially using stale data from a previous
> event and reporting a bogus fault.
> 
> Reject any event buffer whose used length is not exactly the expected
> event size. Still recycle the buffer back to the event queue so a bad
> event does not permanently shrink the queue.
> 
> Signed-off-by: Xu Rao <raoxu@uniontech.com>
> ---
>  drivers/iommu/virtio-iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 587fc13197f1..3ace4a6dd02a 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -635,7 +635,7 @@ static void viommu_event_handler(struct virtqueue *vq)
>  	struct viommu_dev *viommu = vq->vdev->priv;
> 
>  	while ((evt = virtqueue_get_buf(vq, &len)) != NULL) {
> -		if (len > sizeof(*evt)) {
> +		if (len != sizeof(*evt)) {
>  			dev_err(viommu->dev,
>  				"invalid event buffer (len %u != %zu)\n",
>  				len, sizeof(*evt));
> --
> 2.50.1
>