drm/syncobj: Limit the number of handles and use kvmalloc for user-sized arrays

From: Fei Yu <[email protected]>

The syncobj ioctls allocate several temporary arrays whose size is taken directly from the u32 count_handles provided by userspace:

- handles and syncobjs in drm_syncobj_array_find()
- points and entries in drm_syncobj_array_wait_timeout()
- points and chains in drm_syncobj_timeline_signal_ioctl()

These are kmalloc() allocations, so a count above KMALLOC_MAX_SIZE / element size trips the WARN_ON_ONCE_GFP() in ___kmalloc_large_node():

  WARNING: mm/slub.c:5459 at ___kmalloc_large_node+0x10c/0x170 mm/slub.c:5459
  Call Trace:
   __kmalloc_large_node_noprof+0x18/0x90 mm/slub.c:5502
   __do_kmalloc_node mm/slub.c:5519 [inline]
   __kmalloc_noprof+0x3f6/0x710 mm/slub.c:5556
   _kmalloc_noprof include/linux/slab.h:995 [inline]
   _kmalloc_array_noprof include/linux/slab.h:1211 [inline]
   drm_syncobj_array_find+0x3a/0x440 drivers/gpu/drm/drm_syncobj.c:1241
   drm_syncobj_timeline_wait_ioctl+0x18a/0x640 drivers/gpu/drm/drm_syncobj.c:1356
   drm_ioctl_kernel+0x2df/0x3b0 drivers/gpu/drm/drm_ioctl.c:817
   drm_ioctl+0x70e/0xba0 drivers/gpu/drm/drm_ioctl.c:914

The wait entries are 64 bytes each on 64-bit, so with a 4 MiB KMALLOC_MAX_SIZE a wait on more than about 64K handles already fails today, with a warning.

Fix this in two parts:

- Reject more than DRM_SYNCOBJ_MAX_HANDLES (1M) handles with -EINVAL in drm_syncobj_array_find(), which every ioctl taking a handle array goes through. The limit is a fixed number, so it is the same on all architectures. It is above what these ioctls can handle today: with a 4 MiB KMALLOC_MAX_SIZE the 8-byte arrays already fail above 512K handles on 64-bit, so no request that succeeds now is rejected. A static_assert() makes sure that the largest per-handle allocation, the wait entries, stays below the INT_MAX size limit of kvmalloc().

- Switch the six allocations to kvmalloc(), so that handle counts up to the limit (64 MiB of wait entries for 1M handles) no longer depend on physically contiguous memory.

Using kvmalloc() alone is not enough: the reproducer asks for more than INT_MAX bytes, which hits the WARN_ON_ONCE() in __kvmalloc_node_noprof() instead.

Fixes 3e6fb72d6cef ("drm/syncobj: Add a syncobj_array_find helper")

Reported-by [email protected]

Closes https://syzkaller.appspot.com/bug?extid=95416f957d84e858b377

Suggested-by Christian König <[email protected]>

Cc [email protected]

Signed-off-by Fei Yu <[email protected]>

---
v3:
- Use a fixed DRM_SYNCOBJ_MAX_HANDLES of SZ_1M instead of deriving it from sizeof(struct syncobj_wait_entry), so the limit is the same on 32-bit and 64-bit, and add a static_assert() against the kvmalloc() INT_MAX limit (Christian).
- 1M rather than 64K: with a 4 MiB KMALLOC_MAX_SIZE, reset, signal, query and timeline_signal currently work up to 512K handles on 64-bit, so a 64K limit would turn requests that work today into -EINVAL.
- Keep the define kernel internal for -fixes; documenting the limit in the uAPI struct comments will be a separate patch for -next.
- Add Cc: stable.
- v2: https://lore.kernel.org/all/[email protected]/

v2:
- Drop __GFP_NOWARN (Christian).
- Add DRM_SYNCOBJ_MAX_HANDLES, return -EINVAL above it, and switch all six arrays to kvmalloc() (Christian).
- v1: https://lore.kernel.org/all/[email protected]/

The vmemdup_array_user() conversion and the points array cleanup Christian pointed out are left to Tvrtko's series, which he will rebase on top of this fix for -next.

Testing on v7.4-rc1 (x86_64, KASAN, virtio-gpu in QEMU):
- syzbot C reproducer: base warns in ___kmalloc_large_node(), patched is clean, the ioctl returns -EINVAL.
- 1048576 signaled syncobjs; wait, timeline_wait, timeline_signal, query, reset and signal with count 200000, 1M and 1M + 1:
base: 200000: wait and timeline_wait fail with -ENOMEM and the
warning, the others succeed. 1M: all six fail with -ENOMEM.
patched: 200000 and 1M: all succeed. 1M + 1: all return -EINVAL.
- drm_syncobj.o builds for i386 with W=1 (static_assert holds).
- checkpatch --strict clean, W=1 clean, kmemleak clean,
DRM KUnit 667/667 passed.
- IGT syncobj_basic, syncobj_wait, syncobj_timeline on virtio-gpu:
202/202 subtests pass, same as base.

 drivers/gpu/drm/drm_syncobj.c | 44 +++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 15 deletions(-)

drivers/gpu/drm/drm_syncobj.c +29 -15

index f1d4da168545..c8a8c49d12fb 100644
@@ -199,6 +199,7 @@
 #include <linux/file.h>
 #include <linux/fs.h>
 #include <linux/sched/signal.h>
+#include <linux/sizes.h>
 #include <linux/sync_file.h>
 #include <linux/uaccess.h>
 

@@ -220,6 +221,16 @@ struct syncobj_wait_entry {
 	u64    point;
 };
 
+/*
+ * Upper limit for the number of handles passed to a single ioctl. This is a
+ * fixed number so that the limit is the same on all architectures. The
+ * largest per-handle allocation is the wait entry array, which must stay
+ * below the INT_MAX size limit of kvmalloc().
+ */
+#define DRM_SYNCOBJ_MAX_HANDLES	SZ_1M
+static_assert(DRM_SYNCOBJ_MAX_HANDLES <=
+	      INT_MAX / sizeof(struct syncobj_wait_entry));
+
 static void syncobj_wait_syncobj_func(struct drm_syncobj *syncobj,
 				      struct syncobj_wait_entry *wait);
 

@@ -1051,7 +1062,7 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
 		lockdep_assert_none_held_once();
 	}
 
-	points = kmalloc_array(count, sizeof(*points), GFP_KERNEL);
+	points = kvmalloc_array(count, sizeof(*points), GFP_KERNEL);
 	if (points == NULL)
 		return -ENOMEM;
 

@@ -1064,7 +1075,7 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
 		goto err_free_points;
 	}
 
-	entries = kzalloc_objs(*entries, count);
+	entries = kvzalloc_objs(*entries, count);
 	if (!entries) {
 		timeout = -ENOMEM;
 		goto err_free_points;

@@ -1185,10 +1196,10 @@ static signed long drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
 						  &entries[i].fence_cb);
 		dma_fence_put(entries[i].fence);
 	}
-	kfree(entries);
+	kvfree(entries);
 
 err_free_points:
-	kfree(points);
+	kvfree(points);
 
 	return timeout;
 }

@@ -1238,7 +1249,10 @@ static int drm_syncobj_array_find(struct drm_file *file_private,
 	struct drm_syncobj **syncobjs;
 	int ret;
 
-	handles = kmalloc_array(count_handles, sizeof(*handles), GFP_KERNEL);
+	if (count_handles > DRM_SYNCOBJ_MAX_HANDLES)
+		return -EINVAL;
+
+	handles = kvmalloc_array(count_handles, sizeof(*handles), GFP_KERNEL);
 	if (handles == NULL)
 		return -ENOMEM;
 

@@ -1248,7 +1262,7 @@ static int drm_syncobj_array_find(struct drm_file *file_private,
 		goto err_free_handles;
 	}
 
-	syncobjs = kmalloc_objs(*syncobjs, count_handles);
+	syncobjs = kvmalloc_objs(*syncobjs, count_handles);
 	if (syncobjs == NULL) {
 		ret = -ENOMEM;
 		goto err_free_handles;

@@ -1262,16 +1276,16 @@ static int drm_syncobj_array_find(struct drm_file *file_private,
 		}
 	}
 
-	kfree(handles);
+	kvfree(handles);
 	*syncobjs_out = syncobjs;
 	return 0;
 
 err_put_syncobjs:
 	while (i-- > 0)
 		drm_syncobj_put(syncobjs[i]);
-	kfree(syncobjs);
+	kvfree(syncobjs);
 err_free_handles:
-	kfree(handles);
+	kvfree(handles);
 
 	return ret;
 }

@@ -1283,7 +1297,7 @@ static void drm_syncobj_array_free(struct drm_syncobj **syncobjs,
 
 	for (i = 0; i < count; i++)
 		drm_syncobj_put(syncobjs[i]);
-	kfree(syncobjs);
+	kvfree(syncobjs);
 }
 
 int

@@ -1572,8 +1586,8 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data,
 	if (ret < 0)
 		return ret;
 
-	points = kmalloc_array(args->count_handles, sizeof(*points),
-			       GFP_KERNEL);
+	points = kvmalloc_array(args->count_handles, sizeof(*points),
+				GFP_KERNEL);
 	if (!points) {
 		ret = -ENOMEM;
 		goto out;

@@ -1586,7 +1600,7 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data,
 		goto err_points;
 	}
 
-	chains = kmalloc_objs(*chains, args->count_handles);
+	chains = kvmalloc_objs(*chains, args->count_handles);
 	if (!chains) {
 		ret = -ENOMEM;
 		goto err_points;

@@ -1609,9 +1623,9 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data,
 		dma_fence_put(fence);
 	}
 err_chains:
-	kfree(chains);
+	kvfree(chains);
 err_points:
-	kfree(points);
+	kvfree(points);
 out:
 	drm_syncobj_array_free(syncobjs, args->count_handles);
 

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4 --
2.53.0

Quoted 1 line · Fei

Signed-off-by Fei Yu <[email protected]>

Reviewed-by Christian König <[email protected]>

Let me know if you need somebody to push this to drm-misc-next.