| Message ID | 20180424133549.GB10167@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com> We could probably add a sanity check for n_devices to avoid user mode causing excessive memory allocations in the kernel. There is no good reason for this to be bigger than the number of GPUs in the system. The maximum number of GPUs supported due to device minor limit in DRM is 128. Regards, Felix On 2018-04-24 09:35 AM, Dan Carpenter wrote: > args->n_devices is a u32 that comes from the user. The multiplication > could overflow on 32 bit systems possibly leading to privilege > escalation. > > Fixes: 5ec7e02854b3 ("drm/amdkfd: Add ioctls for GPUVM memory management") > Signed-off-by: Dan Carpenter dan.carpenter@oracle.com> > > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > index cd679cf1fd30..ce36e556da38 100644 > --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > @@ -1295,8 +1295,8 @@ static int kfd_ioctl_map_memory_to_gpu(struct file *filep, > return -EINVAL; > } > > - devices_arr = kmalloc(args->n_devices * sizeof(*devices_arr), > - GFP_KERNEL); > + devices_arr = kmalloc_array(args->n_devices, sizeof(*devices_arr), > + GFP_KERNEL); > if (!devices_arr) > return -ENOMEM; > > @@ -1404,8 +1404,8 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file *filep, > return -EINVAL; > } > > - devices_arr = kmalloc(args->n_devices * sizeof(*devices_arr), > - GFP_KERNEL); > + devices_arr = kmalloc_array(args->n_devices, sizeof(*devices_arr), > + GFP_KERNEL); > if (!devices_arr) > return -ENOMEM; >
On Tue, Apr 24, 2018 at 02:58:10PM -0400, Felix Kuehling wrote: > Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com> > > We could probably add a sanity check for n_devices to avoid user mode > causing excessive memory allocations in the kernel. There is no good > reason for this to be bigger than the number of GPUs in the system. The > maximum number of GPUs supported due to device minor limit in DRM is 128. > 128 is sort of a magic number. Is there a MAX_GPU define or something? regards, dan carpenter
On 2018-04-25 05:21 AM, Dan Carpenter wrote: > On Tue, Apr 24, 2018 at 02:58:10PM -0400, Felix Kuehling wrote: >> Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com> >> >> We could probably add a sanity check for n_devices to avoid user mode >> causing excessive memory allocations in the kernel. There is no good >> reason for this to be bigger than the number of GPUs in the system. The >> maximum number of GPUs supported due to device minor limit in DRM is 128. >> > 128 is sort of a magic number. Is there a MAX_GPU define or something? Actually, looking at drm_minor_alloc in drm_file.c, the maximum is only 64 GPUs. It's just a magic number in the code. There is no #define or enum for this. Regards, Felix > > regards, > dan carpenter >
On Tue, Apr 24, 2018 at 9:58 PM, Felix Kuehling <felix.kuehling@amd.com> wrote: > Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com> > > We could probably add a sanity check for n_devices to avoid user mode > causing excessive memory allocations in the kernel. There is no good > reason for this to be bigger than the number of GPUs in the system. The > maximum number of GPUs supported due to device minor limit in DRM is 128. > > Regards, > Felix > > > On 2018-04-24 09:35 AM, Dan Carpenter wrote: >> args->n_devices is a u32 that comes from the user. The multiplication >> could overflow on 32 bit systems possibly leading to privilege >> escalation. >> >> Fixes: 5ec7e02854b3 ("drm/amdkfd: Add ioctls for GPUVM memory management") >> Signed-off-by: Dan Carpenter dan.carpenter@oracle.com> >> >> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >> index cd679cf1fd30..ce36e556da38 100644 >> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c >> @@ -1295,8 +1295,8 @@ static int kfd_ioctl_map_memory_to_gpu(struct file *filep, >> return -EINVAL; >> } >> >> - devices_arr = kmalloc(args->n_devices * sizeof(*devices_arr), >> - GFP_KERNEL); >> + devices_arr = kmalloc_array(args->n_devices, sizeof(*devices_arr), >> + GFP_KERNEL); >> if (!devices_arr) >> return -ENOMEM; >> >> @@ -1404,8 +1404,8 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file *filep, >> return -EINVAL; >> } >> >> - devices_arr = kmalloc(args->n_devices * sizeof(*devices_arr), >> - GFP_KERNEL); >> + devices_arr = kmalloc_array(args->n_devices, sizeof(*devices_arr), >> + GFP_KERNEL); >> if (!devices_arr) >> return -ENOMEM; >> > Thanks! Patch applied to amdkfd-fixes Oded
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c index cd679cf1fd30..ce36e556da38 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c @@ -1295,8 +1295,8 @@ static int kfd_ioctl_map_memory_to_gpu(struct file *filep, return -EINVAL; } - devices_arr = kmalloc(args->n_devices * sizeof(*devices_arr), - GFP_KERNEL); + devices_arr = kmalloc_array(args->n_devices, sizeof(*devices_arr), + GFP_KERNEL); if (!devices_arr) return -ENOMEM; @@ -1404,8 +1404,8 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file *filep, return -EINVAL; } - devices_arr = kmalloc(args->n_devices * sizeof(*devices_arr), - GFP_KERNEL); + devices_arr = kmalloc_array(args->n_devices, sizeof(*devices_arr), + GFP_KERNEL); if (!devices_arr) return -ENOMEM;
args->n_devices is a u32 that comes from the user. The multiplication could overflow on 32 bit systems possibly leading to privilege escalation. Fixes: 5ec7e02854b3 ("drm/amdkfd: Add ioctls for GPUVM memory management") Signed-off-by: Dan Carpenter dan.carpenter@oracle.com>