* [PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test
@ 2024-08-22 14:47 Jason Gunthorpe
2024-08-22 16:52 ` Joao Martins
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Jason Gunthorpe @ 2024-08-22 14:47 UTC (permalink / raw)
To: iommu, Joerg Roedel, Will Deacon
Cc: Joao Martins, Kevin Tian, Matt Ochs, patches
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test
2024-08-22 14:47 [PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test Jason Gunthorpe
@ 2024-08-22 16:52 ` Joao Martins
2024-08-22 16:59 ` Jason Gunthorpe
2024-08-26 6:10 ` Tian, Kevin
2024-08-27 12:47 ` Jason Gunthorpe
2 siblings, 1 reply; 7+ messages in thread
From: Joao Martins @ 2024-08-22 16:52 UTC (permalink / raw)
To: Jason Gunthorpe, iommu
Cc: Kevin Tian, Matt Ochs, patches, Will Deacon, Joerg Roedel
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>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test
2024-08-22 16:52 ` Joao Martins
@ 2024-08-22 16:59 ` Jason Gunthorpe
2024-08-22 17:03 ` Joao Martins
0 siblings, 1 reply; 7+ messages in thread
From: Jason Gunthorpe @ 2024-08-22 16:59 UTC (permalink / raw)
To: Joao Martins
Cc: iommu, Kevin Tian, Matt Ochs, patches, Will Deacon, Joerg Roedel
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test
2024-08-22 16:59 ` Jason Gunthorpe
@ 2024-08-22 17:03 ` Joao Martins
2024-08-22 17:06 ` Jason Gunthorpe
0 siblings, 1 reply; 7+ messages in thread
From: Joao Martins @ 2024-08-22 17:03 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: iommu, Kevin Tian, Matt Ochs, patches, Will Deacon, Joerg Roedel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test
2024-08-22 17:03 ` Joao Martins
@ 2024-08-22 17:06 ` Jason Gunthorpe
0 siblings, 0 replies; 7+ messages in thread
From: Jason Gunthorpe @ 2024-08-22 17:06 UTC (permalink / raw)
To: Joao Martins
Cc: iommu, Kevin Tian, Matt Ochs, patches, Will Deacon, Joerg Roedel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test
2024-08-22 14:47 [PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test Jason Gunthorpe
2024-08-22 16:52 ` Joao Martins
@ 2024-08-26 6:10 ` Tian, Kevin
2024-08-27 12:47 ` Jason Gunthorpe
2 siblings, 0 replies; 7+ messages in thread
From: Tian, Kevin @ 2024-08-26 6:10 UTC (permalink / raw)
To: Jason Gunthorpe, iommu@lists.linux.dev, Joerg Roedel, Will Deacon
Cc: Joao Martins, Matt Ochs, patches@lists.linux.dev
> 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>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test
2024-08-22 14:47 [PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test Jason Gunthorpe
2024-08-22 16:52 ` Joao Martins
2024-08-26 6:10 ` Tian, Kevin
@ 2024-08-27 12:47 ` Jason Gunthorpe
2 siblings, 0 replies; 7+ messages in thread
From: Jason Gunthorpe @ 2024-08-27 12:47 UTC (permalink / raw)
To: iommu, Joerg Roedel, Will Deacon
Cc: Joao Martins, Kevin Tian, Matt Ochs, patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-08-27 12:47 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-22 14:47 [PATCH] iommufd/selftest: Fix buffer read overrrun in the dirty test Jason Gunthorpe
2024-08-22 16:52 ` Joao Martins
2024-08-22 16:59 ` Jason Gunthorpe
2024-08-22 17:03 ` Joao Martins
2024-08-22 17:06 ` Jason Gunthorpe
2024-08-26 6:10 ` Tian, Kevin
2024-08-27 12:47 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox