| Message ID | 20211216161603.983711-1-steven.price@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panfrost: Avoid user size passed to kvmalloc() | expand |
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
> 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 :(
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
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
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
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
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
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
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
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
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,
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(-)