On Mon, Jul 06, 2026 at 04:31:48PM +0800, raoxu wrote:
> From: Xu Rao <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
>
