On Thu, Oct 08, 2026 at 09:56:03AM +0000, [email protected] 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 <[email protected]> > > 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/[email protected]?part=1 Indeed, it would be cleaner to use unaligned APIs, or memcpy the structure. Pre-existing and not part of this patch. -- MST
