[PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test

[PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test

[Jason Gunthorpe]

test_bit() is used to read the memory storing the bitmap, however
test_bit() always uses a unsigned long 8 byte access.

If the bitmap is not an aligned size of 64 bits this will now trigger a
KASAN warning reading past the end of the buffer.

Properly round the buffer allocation to an unsigned long size. Continue to
copy_from_user() using a byte granularity.

Fixes: 9560393b830b ("iommufd/selftest: Fix iommufd_test_dirty() to handle <u8 bitmaps")
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommufd/selftest.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index b60687f57bef3b..c360d5a7675921 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -1342,7 +1342,7 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id,
 			      unsigned long page_size, void __user *uptr,
 			      u32 flags)
 {
-	unsigned long bitmap_size, i, max;
+	unsigned long i, max;
 	struct iommu_test_cmd *cmd = ucmd->cmd;
 	struct iommufd_hw_pagetable *hwpt;
 	struct mock_iommu_domain *mock;
@@ -1363,15 +1363,14 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id,
 	}
 
 	max = length / page_size;
-	bitmap_size = DIV_ROUND_UP(max, BITS_PER_BYTE);
-
-	tmp = kvzalloc(bitmap_size, GFP_KERNEL_ACCOUNT);
+	tmp = kvzalloc(DIV_ROUND_UP(max, BITS_PER_LONG) * sizeof(unsigned long),
+		       GFP_KERNEL_ACCOUNT);
 	if (!tmp) {
 		rc = -ENOMEM;
 		goto out_put;
 	}
 
-	if (copy_from_user(tmp, uptr, bitmap_size)) {
+	if (copy_from_user(tmp, uptr,DIV_ROUND_UP(max, BITS_PER_BYTE))) {
 		rc = -EFAULT;
 		goto out_free;
 	}

base-commit: f97d3fdab5c314e6afcd6cc27a4af6a7f8735566
-- 
2.46.0

Re: [PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test

[Joao Martins]

On 22/08/2024 15:47, Jason Gunthorpe wrote:
> diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
> index b60687f57bef3b..c360d5a7675921 100644
> --- a/drivers/iommu/iommufd/selftest.c
> +++ b/drivers/iommu/iommufd/selftest.c
> @@ -1342,7 +1342,7 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id,
>  			      unsigned long page_size, void __user *uptr,
>  			      u32 flags)
>  {
> -	unsigned long bitmap_size, i, max;
> +	unsigned long i, max;
>  	struct iommu_test_cmd *cmd = ucmd->cmd;
>  	struct iommufd_hw_pagetable *hwpt;
>  	struct mock_iommu_domain *mock;
> @@ -1363,15 +1363,14 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id,
>  	}
>  
>  	max = length / page_size;
> -	bitmap_size = DIV_ROUND_UP(max, BITS_PER_BYTE);
> -
> -	tmp = kvzalloc(bitmap_size, GFP_KERNEL_ACCOUNT);
> +	tmp = kvzalloc(DIV_ROUND_UP(max, BITS_PER_LONG) * sizeof(unsigned long),

If you keep bitmap_size then this gets to be a one-liner patch, but I assume you
want to remove bitmap_size anyways.

> +		       GFP_KERNEL_ACCOUNT);
>  	if (!tmp) {
>  		rc = -ENOMEM;
>  		goto out_put;
>  	}
>  
> -	if (copy_from_user(tmp, uptr, bitmap_size)) {
> +	if (copy_from_user(tmp, uptr,DIV_ROUND_UP(max, BITS_PER_BYTE))) {
                                     ^^ space here after the comma

>  		rc = -EFAULT;
>  		goto out_free;
>  	}
> 
Regardless of the first comment:

	Reviewed-by: Joao Martins <joao.m.martins@oracle.com>

Re: [PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test

[Jason Gunthorpe]

On Thu, Aug 22, 2024 at 05:52:26PM +0100, Joao Martins wrote:
> On 22/08/2024 15:47, Jason Gunthorpe wrote:
> > diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
> > index b60687f57bef3b..c360d5a7675921 100644
> > --- a/drivers/iommu/iommufd/selftest.c
> > +++ b/drivers/iommu/iommufd/selftest.c
> > @@ -1342,7 +1342,7 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id,
> >  			      unsigned long page_size, void __user *uptr,
> >  			      u32 flags)
> >  {
> > -	unsigned long bitmap_size, i, max;
> > +	unsigned long i, max;
> >  	struct iommu_test_cmd *cmd = ucmd->cmd;
> >  	struct iommufd_hw_pagetable *hwpt;
> >  	struct mock_iommu_domain *mock;
> > @@ -1363,15 +1363,14 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id,
> >  	}
> >  
> >  	max = length / page_size;
> > -	bitmap_size = DIV_ROUND_UP(max, BITS_PER_BYTE);
> > -
> > -	tmp = kvzalloc(bitmap_size, GFP_KERNEL_ACCOUNT);
> > +	tmp = kvzalloc(DIV_ROUND_UP(max, BITS_PER_LONG) * sizeof(unsigned long),
> 
> If you keep bitmap_size then this gets to be a one-liner patch, but I assume you
> want to remove bitmap_size anyways.

Then we would technically read past the end of the user buffer..

Thanks,
Jason

Re: [PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test

[Joao Martins]

On 22/08/2024 17:59, Jason Gunthorpe wrote:
> On Thu, Aug 22, 2024 at 05:52:26PM +0100, Joao Martins wrote:
>> On 22/08/2024 15:47, Jason Gunthorpe wrote:
>>> diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
>>> index b60687f57bef3b..c360d5a7675921 100644
>>> --- a/drivers/iommu/iommufd/selftest.c
>>> +++ b/drivers/iommu/iommufd/selftest.c
>>> @@ -1342,7 +1342,7 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id,
>>>  			      unsigned long page_size, void __user *uptr,
>>>  			      u32 flags)
>>>  {
>>> -	unsigned long bitmap_size, i, max;
>>> +	unsigned long i, max;
>>>  	struct iommu_test_cmd *cmd = ucmd->cmd;
>>>  	struct iommufd_hw_pagetable *hwpt;
>>>  	struct mock_iommu_domain *mock;
>>> @@ -1363,15 +1363,14 @@ static int iommufd_test_dirty(struct iommufd_ucmd *ucmd, unsigned int mockpt_id,
>>>  	}
>>>  
>>>  	max = length / page_size;
>>> -	bitmap_size = DIV_ROUND_UP(max, BITS_PER_BYTE);
>>> -
>>> -	tmp = kvzalloc(bitmap_size, GFP_KERNEL_ACCOUNT);
>>> +	tmp = kvzalloc(DIV_ROUND_UP(max, BITS_PER_LONG) * sizeof(unsigned long),
>>
>> If you keep bitmap_size then this gets to be a one-liner patch, but I assume you
>> want to remove bitmap_size anyways.
> 
> Then we would technically read past the end of the user buffer..

No, it's the same code?

I meant more that the next chunk in copy_from_user() can still use bitmap_size
variable. Because you are removing @bitmap_size which is set to
DIV_ROUND_UP(max, BITS_PER_BYTE) to replace with this equivalent below:

-	if (copy_from_user(tmp, uptr, bitmap_size)) {
+	if (copy_from_user(tmp, uptr,DIV_ROUND_UP(max, BITS_PER_BYTE))) {
 		rc = -EFAULT;
 		goto out_free;
 	}


But considering that's only one call side, then I assumed you just wanted removed.

Re: [PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test

[Jason Gunthorpe]

On Thu, Aug 22, 2024 at 06:03:17PM +0100, Joao Martins wrote:

> -	if (copy_from_user(tmp, uptr, bitmap_size)) {
> +	if (copy_from_user(tmp, uptr,DIV_ROUND_UP(max, BITS_PER_BYTE))) {
>  		rc = -EFAULT;
>  		goto out_free;
>  	}
> 
> 
> But considering that's only one call side, then I assumed you just wanted removed.

Oh, I see, yes, with only one user (and there is actually a #define
called bitmap_size in common headers too) it seemed best

Jason

RE: [PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, August 22, 2024 10:47 PM
> 
> test_bit() is used to read the memory storing the bitmap, however
> test_bit() always uses a unsigned long 8 byte access.
> 
> If the bitmap is not an aligned size of 64 bits this will now trigger a
> KASAN warning reading past the end of the buffer.
> 
> Properly round the buffer allocation to an unsigned long size. Continue to
> copy_from_user() using a byte granularity.
> 
> Fixes: 9560393b830b ("iommufd/selftest: Fix iommufd_test_dirty() to handle
> <u8 bitmaps")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test

[Jason Gunthorpe]

On Thu, Aug 22, 2024 at 11:47:09AM -0300, Jason Gunthorpe wrote:
> test_bit() is used to read the memory storing the bitmap, however
> test_bit() always uses a unsigned long 8 byte access.
> 
> If the bitmap is not an aligned size of 64 bits this will now trigger a
> KASAN warning reading past the end of the buffer.
> 
> Properly round the buffer allocation to an unsigned long size. Continue to
> copy_from_user() using a byte granularity.
> 
> Fixes: 9560393b830b ("iommufd/selftest: Fix iommufd_test_dirty() to handle <u8 bitmaps")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/iommufd/selftest.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)

Applied to for-next

Jason