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

[sashiko-bot@kernel.org]

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, but this length validation does not enforce alignment.

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