Re: [PATCH] iommu/virtio: bound the probe-property walk to the probe buffer

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Oct 08, 2026 at 09:56:03AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] iommu/virtio: unaligned 64-bit access due to unvalidated property length
> --
> 
> commit 1966676bc77a49f0f207bc2272bebf67d2bd8f06
> Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
> 
> iommu/virtio: bound the probe-property walk to the probe buffer
> 
> This commit prevents an out-of-bounds read in viommu_probe_endpoint() by
> checking if the property fits within the probe allocation before
> dereferencing it. It also moves the length check in viommu_add_resv_mem()
> ahead of the memory accesses to avoid reading past the end of the
> allocation.
> 
> > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> > index 587fc13197f12..dbc997a6e3568 100644
> > --- a/drivers/iommu/virtio-iommu.c
> > +++ b/drivers/iommu/virtio-iommu.c
> > @@ -489,6 +489,9 @@ static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
> >  	struct iommu_resv_region *region = NULL, *next;
> >  	unsigned long prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> >  
> > +	if (len < sizeof(*mem))
> > +		return -EINVAL;
> > +
> >  	start = start64 = le64_to_cpu(mem->start);
> >  	end = end64 = le64_to_cpu(mem->end);
> 
> [Severity: High]
> If a buggy or malicious device supplies an unaligned property length earlier
> in the parsing loop, could the mem pointer passed to viommu_add_resv_mem()
> be unaligned?
> 
> Because the virtio_iommu_probe_resv_mem structure is not packed, the
> compiler assumes natural alignment. On architectures with strict alignment
> requirements (such as ARM or SPARC), wouldn't these direct 64-bit
> dereferences via le64_to_cpu() trigger a hardware alignment fault and lead
> to a kernel panic?
> 
> [ ... ]
> 
> > @@ -563,8 +564,10 @@ static int viommu_probe_endpoint(struct viommu_dev *viommu, struct device *dev)
> >  	type = le16_to_cpu(prop->type) & VIRTIO_IOMMU_PROBE_T_MASK;
> >  
> >  	while (type != VIRTIO_IOMMU_PROBE_T_NONE &&
> > -	       cur < viommu->probe_size) {
> > +	       cur + sizeof(*prop) <= viommu->probe_size) {
> >  		len = le16_to_cpu(prop->length) + sizeof(*prop);
> > +		if (len > viommu->probe_size - cur)
> > +			break;
> 
> [Severity: High]
> The virtio-iommu specification requires property lengths to be 8-byte
> aligned,

Does it? Where in the spec?

> but this length validation does not enforce alignment.

it really shouldn't since it's not in the spec.

> Can an unaligned property length from the untrusted device advance the cur
> offset incorrectly?
> 
> viommu_probe_endpoint()
>     ...
>     cur += len;
> 
> Wouldn't this cause subsequent property structures to be mapped to unaligned
> memory addresses?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261008094240.1082031-1-yhlee@isslab.korea.ac.kr?part=1


Indeed, it would be cleaner to use unaligned APIs, or memcpy the
structure. Pre-existing and not part of this patch.

-- 
MST