[PATCH][v2] vfio/type1: Sanitize user-supplied inputs to prevent undefined __ffs() behavior

[PATCH][v2] vfio/type1: Sanitize user-supplied inputs to prevent undefined __ffs() behavior

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

The __ffs() function expects a non-zero input. When passed 0, its return
value is undefined (garbage) but does not trigger a hardware fault.
Although downstream logic may eventually catch invalid derived values,
passing unchecked user inputs into __ffs() is a robust-ness and code
quality issue.

Fix this by validating user-supplied inputs early in the UNMAP_DMA and
DIRTY_PAGES ioctl paths before they reach any bit scan operations:

1. Reject an empty dirty.flags in vfio_iommu_type1_dirty_pages() to
   ensure the subsequent __ffs() / __fls() single-bit check is safe.
2. Ensure bitmap.pgsize and range.bitmap.pgsize are valid non-zero
   powers of two before calculating pgshift via __ffs().

This change improves the overall robustness of the VFIO type1 IOMMU
driver against erratic or malicious user-space inputs.

Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
Diff with v1: Add the two check in vfio_iommu_type1_dirty_pages
              and rewrite the commit message

 drivers/vfio/vfio_iommu_type1.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index c8151ba..b74f56c 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -38,6 +38,7 @@
 #include <linux/workqueue.h>
 #include <linux/mm_inline.h>
 #include <linux/overflow.h>
+#include <linux/log2.h>
 #include "vfio.h"
 
 #define DRIVER_VERSION  "0.2"
@@ -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;
+
 		pgshift = __ffs(bitmap.pgsize);
 		ret = verify_bitmap_size(unmap.size >> pgshift,
 					 bitmap.size);
@@ -2985,6 +2989,9 @@ static int vfio_iommu_type1_dirty_pages(struct vfio_iommu *iommu,
 	if (dirty.argsz < minsz || dirty.flags & ~mask)
 		return -EINVAL;
 
+	if (!dirty.flags)
+		return -EINVAL;
+
 	/* only one flag should be set at a time */
 	if (__ffs(dirty.flags) != __fls(dirty.flags))
 		return -EINVAL;
@@ -3039,6 +3046,9 @@ static int vfio_iommu_type1_dirty_pages(struct vfio_iommu *iommu,
 			       range.bitmap.size))
 			return -EINVAL;
 
+		if (unlikely(!is_power_of_2(range.bitmap.pgsize)))
+			return -EINVAL;
+
 		pgshift = __ffs(range.bitmap.pgsize);
 		ret = verify_bitmap_size(size >> pgshift,
 					 range.bitmap.size);
-- 
2.9.4

Re: [PATCH][v2] vfio/type1: Sanitize user-supplied inputs to prevent undefined __ffs() behavior

[David Laight]

On Wed, 17 Jun 2026 19:32:51 +0800
lirongqing <lirongqing@baidu.com> wrote:

> From: Li RongQing <lirongqing@baidu.com>
> 
> The __ffs() function expects a non-zero input. When passed 0, its return
> value is undefined (garbage) but does not trigger a hardware fault.
> Although downstream logic may eventually catch invalid derived values,
> passing unchecked user inputs into __ffs() is a robust-ness and code
> quality issue.
> 
> Fix this by validating user-supplied inputs early in the UNMAP_DMA and
> DIRTY_PAGES ioctl paths before they reach any bit scan operations:
> 
> 1. Reject an empty dirty.flags in vfio_iommu_type1_dirty_pages() to
>    ensure the subsequent __ffs() / __fls() single-bit check is safe.
> 2. Ensure bitmap.pgsize and range.bitmap.pgsize are valid non-zero
>    powers of two before calculating pgshift via __ffs().
> 
> This change improves the overall robustness of the VFIO type1 IOMMU
> driver against erratic or malicious user-space inputs.
> 
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
> Diff with v1: Add the two check in vfio_iommu_type1_dirty_pages
>               and rewrite the commit message
> 
>  drivers/vfio/vfio_iommu_type1.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index c8151ba..b74f56c 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -38,6 +38,7 @@
>  #include <linux/workqueue.h>
>  #include <linux/mm_inline.h>
>  #include <linux/overflow.h>
> +#include <linux/log2.h>
>  #include "vfio.h"
>  
>  #define DRIVER_VERSION  "0.2"
> @@ -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;

Unrelated to this change...
There's not a lot of point calling access_ok() there.
All it does is check that bitmap.data isn't a kernel address and then adds
a synchronising instruction (lfence on x86) because of possible speculative
accesses to the kernel address (in the unsave_get/put_user() that is
expected to follow).

The actual copies do use copy_to/from_user() and include the check.

	David



>  
> +		if (unlikely(!is_power_of_2(bitmap.pgsize)))
> +			return -EINVAL;
> +
>  		pgshift = __ffs(bitmap.pgsize);
>  		ret = verify_bitmap_size(unmap.size >> pgshift,
>  					 bitmap.size);
> @@ -2985,6 +2989,9 @@ static int vfio_iommu_type1_dirty_pages(struct vfio_iommu *iommu,
>  	if (dirty.argsz < minsz || dirty.flags & ~mask)
>  		return -EINVAL;
>  
> +	if (!dirty.flags)
> +		return -EINVAL;
> +
>  	/* only one flag should be set at a time */
>  	if (__ffs(dirty.flags) != __fls(dirty.flags))
>  		return -EINVAL;
> @@ -3039,6 +3046,9 @@ static int vfio_iommu_type1_dirty_pages(struct vfio_iommu *iommu,
>  			       range.bitmap.size))
>  			return -EINVAL;
>  
> +		if (unlikely(!is_power_of_2(range.bitmap.pgsize)))
> +			return -EINVAL;
> +
>  		pgshift = __ffs(range.bitmap.pgsize);
>  		ret = verify_bitmap_size(size >> pgshift,
>  					 range.bitmap.size);

Re: [PATCH][v2] vfio/type1: Sanitize user-supplied inputs to prevent undefined __ffs() behavior

[Alex Williamson]

On Wed, 17 Jun 2026 19:32:51 +0800
lirongqing <lirongqing@baidu.com> wrote:

> From: Li RongQing <lirongqing@baidu.com>
> 
> The __ffs() function expects a non-zero input. When passed 0, its return
> value is undefined (garbage) but does not trigger a hardware fault.
> Although downstream logic may eventually catch invalid derived values,
> passing unchecked user inputs into __ffs() is a robust-ness and code
> quality issue.
> 
> Fix this by validating user-supplied inputs early in the UNMAP_DMA and
> DIRTY_PAGES ioctl paths before they reach any bit scan operations:
> 
> 1. Reject an empty dirty.flags in vfio_iommu_type1_dirty_pages() to
>    ensure the subsequent __ffs() / __fls() single-bit check is safe.
> 2. Ensure bitmap.pgsize and range.bitmap.pgsize are valid non-zero
>    powers of two before calculating pgshift via __ffs().
> 
> This change improves the overall robustness of the VFIO type1 IOMMU
> driver against erratic or malicious user-space inputs.
> 
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
> Diff with v1: Add the two check in vfio_iommu_type1_dirty_pages
>               and rewrite the commit message
> 
>  drivers/vfio/vfio_iommu_type1.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index c8151ba..b74f56c 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -38,6 +38,7 @@
>  #include <linux/workqueue.h>
>  #include <linux/mm_inline.h>
>  #include <linux/overflow.h>
> +#include <linux/log2.h>
>  #include "vfio.h"
>  
>  #define DRIVER_VERSION  "0.2"
> @@ -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;
> +
>  		pgshift = __ffs(bitmap.pgsize);
>  		ret = verify_bitmap_size(unmap.size >> pgshift,
>  					 bitmap.size);
> @@ -2985,6 +2989,9 @@ static int vfio_iommu_type1_dirty_pages(struct vfio_iommu *iommu,
>  	if (dirty.argsz < minsz || dirty.flags & ~mask)
>  		return -EINVAL;
>  
> +	if (!dirty.flags)
> +		return -EINVAL;
> +
>  	/* only one flag should be set at a time */
>  	if (__ffs(dirty.flags) != __fls(dirty.flags))
>  		return -EINVAL;

If it can't be zero and only has one flag bit set, it seems like both
of these could be collapsed into another single !is_power_of_2() test.

Note that if (__ffs(0) == __fls(0)) this could present as an ABI change
where empty flags fall through as success, but that doesn't seem to be
the case for any of the usual suspects.  Probably worth a note in the
commit log though.

Also the unlikely() hint in the other chunks is inconsistent and
unnecessary, these aren't critical hot paths.  Thanks,

Alex


> @@ -3039,6 +3046,9 @@ static int vfio_iommu_type1_dirty_pages(struct vfio_iommu *iommu,
>  			       range.bitmap.size))
>  			return -EINVAL;
>  
> +		if (unlikely(!is_power_of_2(range.bitmap.pgsize)))
> +			return -EINVAL;
> +
>  		pgshift = __ffs(range.bitmap.pgsize);
>  		ret = verify_bitmap_size(size >> pgshift,
>  					 range.bitmap.size);

答复: [????] Re: [PATCH][v2] vfio/type1: Sanitize user-supplied inputs to prevent undefined __ffs() behavior

[Li,Rongqing]

> 
> > From: Li RongQing <lirongqing@baidu.com>
> >
> > The __ffs() function expects a non-zero input. When passed 0, its
> > return value is undefined (garbage) but does not trigger a hardware fault.
> > Although downstream logic may eventually catch invalid derived values,
> > passing unchecked user inputs into __ffs() is a robust-ness and code
> > quality issue.
> >
> > Fix this by validating user-supplied inputs early in the UNMAP_DMA and
> > DIRTY_PAGES ioctl paths before they reach any bit scan operations:
> >
> > 1. Reject an empty dirty.flags in vfio_iommu_type1_dirty_pages() to
> >    ensure the subsequent __ffs() / __fls() single-bit check is safe.
> > 2. Ensure bitmap.pgsize and range.bitmap.pgsize are valid non-zero
> >    powers of two before calculating pgshift via __ffs().
> >
> > This change improves the overall robustness of the VFIO type1 IOMMU
> > driver against erratic or malicious user-space inputs.
> >
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> > Diff with v1: Add the two check in vfio_iommu_type1_dirty_pages
> >               and rewrite the commit message
> >
> >  drivers/vfio/vfio_iommu_type1.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/vfio/vfio_iommu_type1.c
> > b/drivers/vfio/vfio_iommu_type1.c index c8151ba..b74f56c 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -38,6 +38,7 @@
> >  #include <linux/workqueue.h>
> >  #include <linux/mm_inline.h>
> >  #include <linux/overflow.h>
> > +#include <linux/log2.h>
> >  #include "vfio.h"
> >
> >  #define DRIVER_VERSION  "0.2"
> > @@ -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;
> > +
> >  		pgshift = __ffs(bitmap.pgsize);
> >  		ret = verify_bitmap_size(unmap.size >> pgshift,
> >  					 bitmap.size);
> > @@ -2985,6 +2989,9 @@ static int vfio_iommu_type1_dirty_pages(struct
> vfio_iommu *iommu,
> >  	if (dirty.argsz < minsz || dirty.flags & ~mask)
> >  		return -EINVAL;
> >
> > +	if (!dirty.flags)
> > +		return -EINVAL;
> > +
> >  	/* only one flag should be set at a time */
> >  	if (__ffs(dirty.flags) != __fls(dirty.flags))
> >  		return -EINVAL;
> 
> If it can't be zero and only has one flag bit set, it seems like both of these could be
> collapsed into another single !is_power_of_2() test.
> 
> Note that if (__ffs(0) == __fls(0)) this could present as an ABI change where empty
> flags fall through as success, but that doesn't seem to be the case for any of the
> usual suspects.  Probably worth a note in the commit log though.
> 
> Also the unlikely() hint in the other chunks is inconsistent and unnecessary,
> these aren't critical hot paths.  Thanks,
> 

Ok, I will send v3

Thanks

[Li,Rongqing] 


> Alex
> 
> 
> > @@ -3039,6 +3046,9 @@ static int vfio_iommu_type1_dirty_pages(struct
> vfio_iommu *iommu,
> >  			       range.bitmap.size))
> >  			return -EINVAL;
> >
> > +		if (unlikely(!is_power_of_2(range.bitmap.pgsize)))
> > +			return -EINVAL;
> > +
> >  		pgshift = __ffs(range.bitmap.pgsize);
> >  		ret = verify_bitmap_size(size >> pgshift,
> >  					 range.bitmap.size);