Re: [PATCH] vfio: Validate that bitmap.pgsize is a power-of-2 in vfio_iommu_type1_unmap_dma

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch correctly addresses undefined behavior by validating that user-provided bitmap.pgsize is a power-of-2 before passing it to __ffs() in vfio_iommu_type1_unmap_dma(). However, it completely misses identical vulnerabilities in vfio_iommu_type1_dirty_pages() where unvalidated user inputs are also passed to bit-scanning functions.
--

commit 6fe55d8211a4934baec28340932ad6ce53984ed4
Author: Li RongQing <lirongqing@baidu.com>

vfio: Validate that bitmap.pgsize is a power-of-2 in vfio_iommu_type1_unmap_dma

In vfio_iommu_type1_unmap_dma(), the user-supplied bitmap.pgsize is
passed directly to __ffs() without sufficient validation.

> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index c8151ba54de33..b05fde8d87c65 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
[ ... ]
> @@ -2949,6 +2950,9 @@ static int vfio_iommu_type1_unmap_dma(struct vfio_iommu *iommu,
>  		if (!access_ok((void __user *)bitmap.data, bitmap.size))
>  			return -EINVAL;
>  
> +		if (unlikely(!is_power_of_2(bitmap.pgsize)))
> +			return -EINVAL;
> +

[Severity: High]
This is a pre-existing issue, but while this check addresses the problem
here, does the exact same problem exist in vfio_iommu_type1_dirty_pages()
in the same file?

If userspace passes dirty.flags == 0, the invalid flag check is bypassed,
and both macros are invoked with 0, which triggers undefined behavior:

drivers/vfio/vfio_iommu_type1.c:vfio_iommu_type1_dirty_pages() {
    ...
	/* only one flag should be set at a time */
	if (__ffs(dirty.flags) != __fls(dirty.flags))
		return -EINVAL;
    ...
}

Similarly, when VFIO_IOMMU_DIRTY_PAGES_FLAG_GET_BITMAP is requested,
range.bitmap.pgsize is passed to __ffs() without checking for 0 or
non-power-of-2:

drivers/vfio/vfio_iommu_type1.c:vfio_iommu_type1_dirty_pages() {
    ...
		pgshift = __ffs(range.bitmap.pgsize);
		ret = verify_bitmap_size(size >> pgshift,
					 range.bitmap.size);
    ...
}

Since providing 0 triggers undefined behavior, and a non-power-of-2 value
distorts the subsequent verify_bitmap_size() logic exactly as described in
the commit message, would it be better to add the same validation to
vfio_iommu_type1_dirty_pages() as well?

>  		pgshift = __ffs(bitmap.pgsize);
>  		ret = verify_bitmap_size(unmap.size >> pgshift,
>  					 bitmap.size);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616111733.1810-1-lirongqing@baidu.com?part=1