[PATCH] drm/amdkfd: Integer overflows in ioctl

[PATCH] drm/amdkfd: Integer overflows in ioctl

[Dan Carpenter]

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;
 

Re: [PATCH] drm/amdkfd: Integer overflows in ioctl

[Felix Kuehling]

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;
>  

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drm/amdkfd: Integer overflows in ioctl

[Dan Carpenter]

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

Re: [PATCH] drm/amdkfd: Integer overflows in ioctl

[Felix Kuehling]

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
>

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drm/amdkfd: Integer overflows in ioctl

[Oded Gabbay]

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