Re: [PATCH] drm/amdkfd: Integer overflows in ioctl
On Tue, Apr 24, 2018 at 9:58 PM, Felix Kuehling wrote:
> Reviewed-by: Felix Kuehling
>
> 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.carpen...@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 mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amdkfd: Integer overflows in ioctl
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 >> >> 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 > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amdkfd: Integer overflows in ioctl
On Tue, Apr 24, 2018 at 02:58:10PM -0400, Felix Kuehling wrote: > Reviewed-by: Felix Kuehling > > 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amdkfd: Integer overflows in ioctl
Reviewed-by: Felix Kuehling
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.carpen...@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;
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/amdkfd: Integer overflows in ioctl
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.carpen...@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;
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
