Skip to contentSkip to conversation

drm/amdkfd: Integer overflows in ioctl

PATCH In review Dan Carpenter 3 participants 5 messages Apr 24, 2018 – May 11, 2018 1 message not in the archive

Subsystems (4)
DRM drivers Kernel janitors Radeon and amdgpu DRM drivers AMD KFD
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 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 [email protected]>
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +4 -4
index cd679cf1fd30..ce36e556da38 100644
@@ -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;

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Reviewed-by on v1 1/1
#1 lore raw
Reviewed-by Felix Kuehling <[email protected]>

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

Quoted 23 lines · 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 [email protected]>
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +2 -2
index cd679cf1fd30..ce36e556da38 100644
@@ -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;

drivers/gpu/drm/amd/amdkfd/kfd_chardev.c static int kfd_ioctl_unmap_memory_from_gpu(struct file *filep, · line 1411

Quoted 10 lines
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +2 -2
@@ -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;

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

A message that is not in the archive (sent off-list or not yet received): e2b7b3c1-cadb-330f-53a8-57568d4bc877-5C7GfCeVMHo@public.gmane.org

author
#3 lore raw
Quoted 7 lines · Felix Kuehling
Reviewed-by Felix Kuehling <[email protected]>

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

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

#4 lore raw
Quoted 9 lines · Dan Carpenter
Quoted 7 lines · Felix Kuehling
Reviewed-by Felix Kuehling <[email protected]>

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

Quoted 4 lines

regards,
dan carpenter

_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

#5 lore raw
In reply to Felix Kuehling
Quoted 38 lines · Felix Kuehling
Reviewed-by Felix Kuehling <[email protected]>

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

Quoted 26 lines · 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 [email protected]>
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +2 -2
index cd679cf1fd30..ce36e556da38 100644
@@ -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;
      }
Quoted 8 lines

drivers/gpu/drm/amd/amdkfd/kfd_chardev.c static int kfd_ioctl_map_memory_to_gpu(struct file *filep, · line 1302

Quoted 7 lines
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c +2 -2
-     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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel