[PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Steven Price]

panfrost_copy_in_sync() takes the number of fences from user space
(in_sync_count) and used to kvmalloc() an array to hold that number of
fences before processing them. This provides an easy method for user
space to trigger the OOM killer (by temporarily allocating large amounts
of kernel memory) or hit the WARN_ONCE() added by 7661809d493b ("mm:
don't allow oversized kvmalloc() calls").

Since we don't expect there to be a large number of fences we can
instead iterate over the fences one-by-one and avoid the temporary
allocation altogether. This also makes the code simpler.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 44 ++++++++-----------------
 1 file changed, 14 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 96bb5a465627..12ab55e5782c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -186,47 +186,31 @@ panfrost_copy_in_sync(struct drm_device *dev,
 		  struct drm_panfrost_submit *args,
 		  struct panfrost_job *job)
 {
-	u32 *handles;
-	int ret = 0;
-	int i, in_fence_count;
-
-	in_fence_count = args->in_sync_count;
-
-	if (!in_fence_count)
-		return 0;
-
-	handles = kvmalloc_array(in_fence_count, sizeof(u32), GFP_KERNEL);
-	if (!handles) {
-		ret = -ENOMEM;
-		DRM_DEBUG("Failed to allocate incoming syncobj handles\n");
-		goto fail;
-	}
+	int i;
+	u32 __user *user_handles = u64_to_user_ptr(args->in_syncs);
 
-	if (copy_from_user(handles,
-			   (void __user *)(uintptr_t)args->in_syncs,
-			   in_fence_count * sizeof(u32))) {
-		ret = -EFAULT;
-		DRM_DEBUG("Failed to copy in syncobj handles\n");
-		goto fail;
-	}
-
-	for (i = 0; i < in_fence_count; i++) {
+	for (i = 0; i < args->in_sync_count; i++) {
 		struct dma_fence *fence;
+		u32 handle;
+		int ret;
+
+		ret = copy_from_user(&handle, user_handles + i,
+				     sizeof(handle));
+		if (ret)
+			return -EFAULT;
 
-		ret = drm_syncobj_find_fence(file_priv, handles[i], 0, 0,
+		ret = drm_syncobj_find_fence(file_priv, handle, 0, 0,
 					     &fence);
 		if (ret)
-			goto fail;
+			return ret;
 
 		ret = drm_sched_job_add_dependency(&job->base, fence);
 
 		if (ret)
-			goto fail;
+			return ret;
 	}
 
-fail:
-	kvfree(handles);
-	return ret;
+	return 0;
 }
 
 static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
-- 
2.30.2

[PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Steven Price]

panfrost_copy_in_sync() takes the number of fences from user space
(in_sync_count) and used to kvmalloc() an array to hold that number of
fences before processing them. This provides an easy method for user
space to trigger the OOM killer (by temporarily allocating large amounts
of kernel memory) or hit the WARN_ONCE() added by 7661809d493b ("mm:
don't allow oversized kvmalloc() calls").

Since we don't expect there to be a large number of fences we can
instead iterate over the fences one-by-one and avoid the temporary
allocation altogether. This also makes the code simpler.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 44 ++++++++-----------------
 1 file changed, 14 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 96bb5a465627..12ab55e5782c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -186,47 +186,31 @@ panfrost_copy_in_sync(struct drm_device *dev,
 		  struct drm_panfrost_submit *args,
 		  struct panfrost_job *job)
 {
-	u32 *handles;
-	int ret = 0;
-	int i, in_fence_count;
-
-	in_fence_count = args->in_sync_count;
-
-	if (!in_fence_count)
-		return 0;
-
-	handles = kvmalloc_array(in_fence_count, sizeof(u32), GFP_KERNEL);
-	if (!handles) {
-		ret = -ENOMEM;
-		DRM_DEBUG("Failed to allocate incoming syncobj handles\n");
-		goto fail;
-	}
+	int i;
+	u32 __user *user_handles = u64_to_user_ptr(args->in_syncs);
 
-	if (copy_from_user(handles,
-			   (void __user *)(uintptr_t)args->in_syncs,
-			   in_fence_count * sizeof(u32))) {
-		ret = -EFAULT;
-		DRM_DEBUG("Failed to copy in syncobj handles\n");
-		goto fail;
-	}
-
-	for (i = 0; i < in_fence_count; i++) {
+	for (i = 0; i < args->in_sync_count; i++) {
 		struct dma_fence *fence;
+		u32 handle;
+		int ret;
+
+		ret = copy_from_user(&handle, user_handles + i,
+				     sizeof(handle));
+		if (ret)
+			return -EFAULT;
 
-		ret = drm_syncobj_find_fence(file_priv, handles[i], 0, 0,
+		ret = drm_syncobj_find_fence(file_priv, handle, 0, 0,
 					     &fence);
 		if (ret)
-			goto fail;
+			return ret;
 
 		ret = drm_sched_job_add_dependency(&job->base, fence);
 
 		if (ret)
-			goto fail;
+			return ret;
 	}
 
-fail:
-	kvfree(handles);
-	return ret;
+	return 0;
 }
 
 static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
-- 
2.30.2

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Rob Herring]

On Thu, Dec 16, 2021 at 10:16 AM Steven Price <steven.price@arm.com> wrote:
>
> panfrost_copy_in_sync() takes the number of fences from user space
> (in_sync_count) and used to kvmalloc() an array to hold that number of
> fences before processing them. This provides an easy method for user
> space to trigger the OOM killer (by temporarily allocating large amounts
> of kernel memory) or hit the WARN_ONCE() added by 7661809d493b ("mm:
> don't allow oversized kvmalloc() calls").
>
> Since we don't expect there to be a large number of fences we can
> instead iterate over the fences one-by-one and avoid the temporary
> allocation altogether. This also makes the code simpler.

Doesn't the BO lookup suffer from the same issue?

Rob

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Rob Herring]

On Thu, Dec 16, 2021 at 10:16 AM Steven Price <steven.price@arm.com> wrote:
>
> panfrost_copy_in_sync() takes the number of fences from user space
> (in_sync_count) and used to kvmalloc() an array to hold that number of
> fences before processing them. This provides an easy method for user
> space to trigger the OOM killer (by temporarily allocating large amounts
> of kernel memory) or hit the WARN_ONCE() added by 7661809d493b ("mm:
> don't allow oversized kvmalloc() calls").
>
> Since we don't expect there to be a large number of fences we can
> instead iterate over the fences one-by-one and avoid the temporary
> allocation altogether. This also makes the code simpler.

Doesn't the BO lookup suffer from the same issue?

Rob

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Alyssa Rosenzweig]

> This provides an easy method for user
> space to trigger the OOM killer (by temporarily allocating large amounts
> of kernel memory)

panfrost user space has a lot of easy ways to trigger to the OOM killer
unfortunately .... if this is something we want to fix there are a lot
more patches coming :(

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Alyssa Rosenzweig]

> This provides an easy method for user
> space to trigger the OOM killer (by temporarily allocating large amounts
> of kernel memory)

panfrost user space has a lot of easy ways to trigger to the OOM killer
unfortunately .... if this is something we want to fix there are a lot
more patches coming :(

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Dan Carpenter]

On Thu, Dec 16, 2021 at 12:49:53PM -0500, Alyssa Rosenzweig wrote:
> > This provides an easy method for user
> > space to trigger the OOM killer (by temporarily allocating large amounts
> > of kernel memory)
> 
> panfrost user space has a lot of easy ways to trigger to the OOM killer
> unfortunately .... if this is something we want to fix there are a lot
> more patches coming :(

My static checker is only looking at the new rule that kvmalloc() can't
allocate more than 2GB.  But eventually syzbot will figure out how to
trigger the other OOMs so fixing that is going to be a requirement at
some point.

regards,
dan carpenter

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Dan Carpenter]

On Thu, Dec 16, 2021 at 12:49:53PM -0500, Alyssa Rosenzweig wrote:
> > This provides an easy method for user
> > space to trigger the OOM killer (by temporarily allocating large amounts
> > of kernel memory)
> 
> panfrost user space has a lot of easy ways to trigger to the OOM killer
> unfortunately .... if this is something we want to fix there are a lot
> more patches coming :(

My static checker is only looking at the new rule that kvmalloc() can't
allocate more than 2GB.  But eventually syzbot will figure out how to
trigger the other OOMs so fixing that is going to be a requirement at
some point.

regards,
dan carpenter

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Tomeu Vizoso]

On 12/16/21 6:49 PM, Alyssa Rosenzweig wrote:
>> This provides an easy method for user
>> space to trigger the OOM killer (by temporarily allocating large amounts
>> of kernel memory)
> 
> panfrost user space has a lot of easy ways to trigger to the OOM killer
> unfortunately .... if this is something we want to fix there are a lot
> more patches coming :(

What are you thinking of, Alyssa?

My understanding is that the problem are kernel allocations that aren't 
accounted per userspace process. I would expect shmem-backed BOs to be 
taken into account by the OOM killer, so the offending process would be 
terminated without affecting the other processes in the system.

Cheers,

Tomeu

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Tomeu Vizoso]

On 12/16/21 6:49 PM, Alyssa Rosenzweig wrote:
>> This provides an easy method for user
>> space to trigger the OOM killer (by temporarily allocating large amounts
>> of kernel memory)
> 
> panfrost user space has a lot of easy ways to trigger to the OOM killer
> unfortunately .... if this is something we want to fix there are a lot
> more patches coming :(

What are you thinking of, Alyssa?

My understanding is that the problem are kernel allocations that aren't 
accounted per userspace process. I would expect shmem-backed BOs to be 
taken into account by the OOM killer, so the offending process would be 
terminated without affecting the other processes in the system.

Cheers,

Tomeu

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Steven Price]

On 16/12/2021 17:12, Rob Herring wrote:
> On Thu, Dec 16, 2021 at 10:16 AM Steven Price <steven.price@arm.com> wrote:
>>
>> panfrost_copy_in_sync() takes the number of fences from user space
>> (in_sync_count) and used to kvmalloc() an array to hold that number of
>> fences before processing them. This provides an easy method for user
>> space to trigger the OOM killer (by temporarily allocating large amounts
>> of kernel memory) or hit the WARN_ONCE() added by 7661809d493b ("mm:
>> don't allow oversized kvmalloc() calls").
>>
>> Since we don't expect there to be a large number of fences we can
>> instead iterate over the fences one-by-one and avoid the temporary
>> allocation altogether. This also makes the code simpler.
> 
> Doesn't the BO lookup suffer from the same issue?

Ah! Yes it does - I'd looked at this and seen the call to
drm_gem_objects_lookup() and assumed that since it's a DRM function (not
Panfrost specific) it must already handle this gracefully. However
clearly it doesn't, and a quick grep confirms that Panfrost is actually
the only user of the function anyway.

However this one is harder to fix without setting an arbitrary cap on
the number of BOs during a sumbit. I'm not sure how other drivers handle
this - the ones I've looked at so far all have the same issue. There's
obviously the list that Dan already sent, but e.g. msm has the same bug
in msm_gem_submit.c:submit_create() with an amusing bug where the check
for (sz > SIZE_MAX) will never hit, although the call is to kzalloc() so
large allocations are going to fail anyway.

Has anyone got any views on how this should be handled generally? I'm
somewhat wary of setting arbitrary limits, especially as it's
effectively an ABI change.

Steve

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Steven Price]

On 16/12/2021 17:12, Rob Herring wrote:
> On Thu, Dec 16, 2021 at 10:16 AM Steven Price <steven.price@arm.com> wrote:
>>
>> panfrost_copy_in_sync() takes the number of fences from user space
>> (in_sync_count) and used to kvmalloc() an array to hold that number of
>> fences before processing them. This provides an easy method for user
>> space to trigger the OOM killer (by temporarily allocating large amounts
>> of kernel memory) or hit the WARN_ONCE() added by 7661809d493b ("mm:
>> don't allow oversized kvmalloc() calls").
>>
>> Since we don't expect there to be a large number of fences we can
>> instead iterate over the fences one-by-one and avoid the temporary
>> allocation altogether. This also makes the code simpler.
> 
> Doesn't the BO lookup suffer from the same issue?

Ah! Yes it does - I'd looked at this and seen the call to
drm_gem_objects_lookup() and assumed that since it's a DRM function (not
Panfrost specific) it must already handle this gracefully. However
clearly it doesn't, and a quick grep confirms that Panfrost is actually
the only user of the function anyway.

However this one is harder to fix without setting an arbitrary cap on
the number of BOs during a sumbit. I'm not sure how other drivers handle
this - the ones I've looked at so far all have the same issue. There's
obviously the list that Dan already sent, but e.g. msm has the same bug
in msm_gem_submit.c:submit_create() with an amusing bug where the check
for (sz > SIZE_MAX) will never hit, although the call is to kzalloc() so
large allocations are going to fail anyway.

Has anyone got any views on how this should be handled generally? I'm
somewhat wary of setting arbitrary limits, especially as it's
effectively an ABI change.

Steve

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Steven Price]

On 16/12/2021 17:49, Alyssa Rosenzweig wrote:
>> This provides an easy method for user
>> space to trigger the OOM killer (by temporarily allocating large amounts
>> of kernel memory)
> 
> panfrost user space has a lot of easy ways to trigger to the OOM killer
> unfortunately .... if this is something we want to fix there are a lot
> more patches coming :(
> 

Sorry I should have been a bit clearer in my wording. The issue isn't
triggering the OOM killer as such - it's triggering the OOM killer
without allocating memory accounted to the process. So e.g. allocating
large numbers of BOs should be ok as the OOM killer will come for the
process that allocated those BOs (so this is not much different from
allocating normal user space memory). However in this path there's no
user space allocation - so we can have a tiny process that triggers a
large kernel allocation and the OOM killer will go after every other
process first.

As Dan points out syzbot can have fun with this sort of thing (if it can
figure out the ioctl).

Thanks,

Steve

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Steven Price]

On 16/12/2021 17:49, Alyssa Rosenzweig wrote:
>> This provides an easy method for user
>> space to trigger the OOM killer (by temporarily allocating large amounts
>> of kernel memory)
> 
> panfrost user space has a lot of easy ways to trigger to the OOM killer
> unfortunately .... if this is something we want to fix there are a lot
> more patches coming :(
> 

Sorry I should have been a bit clearer in my wording. The issue isn't
triggering the OOM killer as such - it's triggering the OOM killer
without allocating memory accounted to the process. So e.g. allocating
large numbers of BOs should be ok as the OOM killer will come for the
process that allocated those BOs (so this is not much different from
allocating normal user space memory). However in this path there's no
user space allocation - so we can have a tiny process that triggers a
large kernel allocation and the OOM killer will go after every other
process first.

As Dan points out syzbot can have fun with this sort of thing (if it can
figure out the ioctl).

Thanks,

Steve

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Dan Carpenter]

On Fri, Dec 17, 2021 at 08:55:50AM +0000, Steven Price wrote:
> However this one is harder to fix without setting an arbitrary cap on
> the number of BOs during a sumbit. I'm not sure how other drivers handle
> this - the ones I've looked at so far all have the same issue. There's
> obviously the list that Dan already sent, but e.g. msm has the same bug
> in msm_gem_submit.c:submit_create() with an amusing bug where the check
> for (sz > SIZE_MAX) will never hit, although the call is to kzalloc() so
> large allocations are going to fail anyway.

sz is u64 and SIZE_MAX is ULONG_MAX so the (sz > SIZE_MAX) condition
does work to prevent an integer overflow on 32bit systems.  But it's not
beautiful.

regards,
dan carpenter

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Dan Carpenter]

On Fri, Dec 17, 2021 at 08:55:50AM +0000, Steven Price wrote:
> However this one is harder to fix without setting an arbitrary cap on
> the number of BOs during a sumbit. I'm not sure how other drivers handle
> this - the ones I've looked at so far all have the same issue. There's
> obviously the list that Dan already sent, but e.g. msm has the same bug
> in msm_gem_submit.c:submit_create() with an amusing bug where the check
> for (sz > SIZE_MAX) will never hit, although the call is to kzalloc() so
> large allocations are going to fail anyway.

sz is u64 and SIZE_MAX is ULONG_MAX so the (sz > SIZE_MAX) condition
does work to prevent an integer overflow on 32bit systems.  But it's not
beautiful.

regards,
dan carpenter

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Steven Price]

On 17/12/2021 09:10, Dan Carpenter wrote:
> On Fri, Dec 17, 2021 at 08:55:50AM +0000, Steven Price wrote:
>> However this one is harder to fix without setting an arbitrary cap on
>> the number of BOs during a sumbit. I'm not sure how other drivers handle
>> this - the ones I've looked at so far all have the same issue. There's
>> obviously the list that Dan already sent, but e.g. msm has the same bug
>> in msm_gem_submit.c:submit_create() with an amusing bug where the check
>> for (sz > SIZE_MAX) will never hit, although the call is to kzalloc() so
>> large allocations are going to fail anyway.
> 
> sz is u64 and SIZE_MAX is ULONG_MAX so the (sz > SIZE_MAX) condition
> does work to prevent an integer overflow on 32bit systems.  But it's not
> beautiful.

sz is the result of struct_size() which returns a size_t, and SIZE_MAX
in case of an overflow. However the check is *greater than* SIZE_MAX
which will never occur even on 32 bit systems.

However the chances of kzalloc() allocating SIZE_MAX are 0 so I don't
see it's an exploitable bug.

Steve

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Steven Price]

On 17/12/2021 09:10, Dan Carpenter wrote:
> On Fri, Dec 17, 2021 at 08:55:50AM +0000, Steven Price wrote:
>> However this one is harder to fix without setting an arbitrary cap on
>> the number of BOs during a sumbit. I'm not sure how other drivers handle
>> this - the ones I've looked at so far all have the same issue. There's
>> obviously the list that Dan already sent, but e.g. msm has the same bug
>> in msm_gem_submit.c:submit_create() with an amusing bug where the check
>> for (sz > SIZE_MAX) will never hit, although the call is to kzalloc() so
>> large allocations are going to fail anyway.
> 
> sz is u64 and SIZE_MAX is ULONG_MAX so the (sz > SIZE_MAX) condition
> does work to prevent an integer overflow on 32bit systems.  But it's not
> beautiful.

sz is the result of struct_size() which returns a size_t, and SIZE_MAX
in case of an overflow. However the check is *greater than* SIZE_MAX
which will never occur even on 32 bit systems.

However the chances of kzalloc() allocating SIZE_MAX are 0 so I don't
see it's an exploitable bug.

Steve

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Dan Carpenter]

On Fri, Dec 17, 2021 at 09:16:19AM +0000, Steven Price wrote:
> On 17/12/2021 09:10, Dan Carpenter wrote:
> > On Fri, Dec 17, 2021 at 08:55:50AM +0000, Steven Price wrote:
> >> However this one is harder to fix without setting an arbitrary cap on
> >> the number of BOs during a sumbit. I'm not sure how other drivers handle
> >> this - the ones I've looked at so far all have the same issue. There's
> >> obviously the list that Dan already sent, but e.g. msm has the same bug
> >> in msm_gem_submit.c:submit_create() with an amusing bug where the check
> >> for (sz > SIZE_MAX) will never hit, although the call is to kzalloc() so
> >> large allocations are going to fail anyway.
> > 
> > sz is u64 and SIZE_MAX is ULONG_MAX so the (sz > SIZE_MAX) condition
> > does work to prevent an integer overflow on 32bit systems.  But it's not
> > beautiful.
> 
> sz is the result of struct_size() which returns a size_t, and SIZE_MAX
> in case of an overflow.

Correct.

> However the check is *greater than* SIZE_MAX
> which will never occur even on 32 bit systems.

You've missed a part.  We add ((u64)nr_cmds * sizeof(submit->cmd[0]))
to SIZE_MAX.  If nr_cmds is zero then, whatever, the kzmalloc() will
fail.  No big deal.  But if it's non-zero then "sz" is larger than
SIZE_MAX and we allocate a smaller buffer than expected leading to
memory corruption.

Btw, it turns out that I had a hand in writing that check so hooray for
me.  :)  #HoorayForMe

regards,
dan carpenter

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Dan Carpenter]

On Fri, Dec 17, 2021 at 09:16:19AM +0000, Steven Price wrote:
> On 17/12/2021 09:10, Dan Carpenter wrote:
> > On Fri, Dec 17, 2021 at 08:55:50AM +0000, Steven Price wrote:
> >> However this one is harder to fix without setting an arbitrary cap on
> >> the number of BOs during a sumbit. I'm not sure how other drivers handle
> >> this - the ones I've looked at so far all have the same issue. There's
> >> obviously the list that Dan already sent, but e.g. msm has the same bug
> >> in msm_gem_submit.c:submit_create() with an amusing bug where the check
> >> for (sz > SIZE_MAX) will never hit, although the call is to kzalloc() so
> >> large allocations are going to fail anyway.
> > 
> > sz is u64 and SIZE_MAX is ULONG_MAX so the (sz > SIZE_MAX) condition
> > does work to prevent an integer overflow on 32bit systems.  But it's not
> > beautiful.
> 
> sz is the result of struct_size() which returns a size_t, and SIZE_MAX
> in case of an overflow.

Correct.

> However the check is *greater than* SIZE_MAX
> which will never occur even on 32 bit systems.

You've missed a part.  We add ((u64)nr_cmds * sizeof(submit->cmd[0]))
to SIZE_MAX.  If nr_cmds is zero then, whatever, the kzmalloc() will
fail.  No big deal.  But if it's non-zero then "sz" is larger than
SIZE_MAX and we allocate a smaller buffer than expected leading to
memory corruption.

Btw, it turns out that I had a hand in writing that check so hooray for
me.  :)  #HoorayForMe

regards,
dan carpenter

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Steven Price]

On 17/12/2021 09:28, Dan Carpenter wrote:
> On Fri, Dec 17, 2021 at 09:16:19AM +0000, Steven Price wrote:
>> On 17/12/2021 09:10, Dan Carpenter wrote:
>>> On Fri, Dec 17, 2021 at 08:55:50AM +0000, Steven Price wrote:
>>>> However this one is harder to fix without setting an arbitrary cap on
>>>> the number of BOs during a sumbit. I'm not sure how other drivers handle
>>>> this - the ones I've looked at so far all have the same issue. There's
>>>> obviously the list that Dan already sent, but e.g. msm has the same bug
>>>> in msm_gem_submit.c:submit_create() with an amusing bug where the check
>>>> for (sz > SIZE_MAX) will never hit, although the call is to kzalloc() so
>>>> large allocations are going to fail anyway.
>>>
>>> sz is u64 and SIZE_MAX is ULONG_MAX so the (sz > SIZE_MAX) condition
>>> does work to prevent an integer overflow on 32bit systems.  But it's not
>>> beautiful.
>>
>> sz is the result of struct_size() which returns a size_t, and SIZE_MAX
>> in case of an overflow.
> 
> Correct.
> 
>> However the check is *greater than* SIZE_MAX
>> which will never occur even on 32 bit systems.
> 
> You've missed a part.  We add ((u64)nr_cmds * sizeof(submit->cmd[0]))
> to SIZE_MAX.  If nr_cmds is zero then, whatever, the kzmalloc() will
> fail.  No big deal.  But if it's non-zero then "sz" is larger than
> SIZE_MAX and we allocate a smaller buffer than expected leading to
> memory corruption.

Ah, my bracket matching is obviously off today - somehow I hadn't
noticed that the second line wasn't part of the call to struct_size().

> Btw, it turns out that I had a hand in writing that check so hooray for
> me.  :)  #HoorayForMe

Indeed hooray for you! ;) Although it still all seems like a very
round-a-bout way of enforcing an arbitrary maximum on the size!

Thanks,

Steve

Re: [PATCH] drm/panfrost: Avoid user size passed to kvmalloc()

[Steven Price]

On 17/12/2021 09:28, Dan Carpenter wrote:
> On Fri, Dec 17, 2021 at 09:16:19AM +0000, Steven Price wrote:
>> On 17/12/2021 09:10, Dan Carpenter wrote:
>>> On Fri, Dec 17, 2021 at 08:55:50AM +0000, Steven Price wrote:
>>>> However this one is harder to fix without setting an arbitrary cap on
>>>> the number of BOs during a sumbit. I'm not sure how other drivers handle
>>>> this - the ones I've looked at so far all have the same issue. There's
>>>> obviously the list that Dan already sent, but e.g. msm has the same bug
>>>> in msm_gem_submit.c:submit_create() with an amusing bug where the check
>>>> for (sz > SIZE_MAX) will never hit, although the call is to kzalloc() so
>>>> large allocations are going to fail anyway.
>>>
>>> sz is u64 and SIZE_MAX is ULONG_MAX so the (sz > SIZE_MAX) condition
>>> does work to prevent an integer overflow on 32bit systems.  But it's not
>>> beautiful.
>>
>> sz is the result of struct_size() which returns a size_t, and SIZE_MAX
>> in case of an overflow.
> 
> Correct.
> 
>> However the check is *greater than* SIZE_MAX
>> which will never occur even on 32 bit systems.
> 
> You've missed a part.  We add ((u64)nr_cmds * sizeof(submit->cmd[0]))
> to SIZE_MAX.  If nr_cmds is zero then, whatever, the kzmalloc() will
> fail.  No big deal.  But if it's non-zero then "sz" is larger than
> SIZE_MAX and we allocate a smaller buffer than expected leading to
> memory corruption.

Ah, my bracket matching is obviously off today - somehow I hadn't
noticed that the second line wasn't part of the call to struct_size().

> Btw, it turns out that I had a hand in writing that check so hooray for
> me.  :)  #HoorayForMe

Indeed hooray for you! ;) Although it still all seems like a very
round-a-bout way of enforcing an arbitrary maximum on the size!

Thanks,

Steve