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;
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);@@ -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);
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
dri-devel@lists.freedesktop.org
-
Dan Carpenter -
Felix Kuehling -
Oded Gabbay