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

[Jean-Philippe Brucker <jpb@kernel.org>]

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
>