[PATCH v3 0/2] drm/tests: Fix for UAF and a test for drm_exec lock alloc tracking warning

[PATCH v3 0/2] drm/tests: Fix for UAF and a test for drm_exec lock alloc tracking warning

[Thomas Hellström]

While trying to replicate a weird drm_exec lock alloc tracking warning
using the drm_exec kunit test, the warning was shadowed by a UAF warning
from KASAN due to a bug in the drm kunit helpers.

Patch 1 fixes that drm kunit UAF.
Patch 2 introduces a drm_exec kunit subtest that fails if the conditions
      for the weird warning are met.

The series previously also had a patch with a drm_exec workaround for the
warning but that patch has already been commited to drm_misc_next_fixes.

v2:
- Rewording of commit messages
- Add some commit message tags
v3:
- Remove an already committed patch
- Rework the test to not require dmesg inspection (Maxime Ripard)
- Condition the test on CONFIG_LOCK_ALLOC
- Update code comments and commit messages (Maxime Ripard)

Cc: Maxime Ripard <mripard@kernel.org>
Cc: Christian König <christian.koenig@amd.com>

Thomas Hellström (2):
  drm/tests: helpers: Avoid a driver uaf
  drm/tests/drm_exec: Add a test for object freeing within
    drm_exec_fini()

 drivers/gpu/drm/tests/drm_exec_test.c | 82 +++++++++++++++++++++++++++
 include/drm/drm_kunit_helpers.h       |  4 +-
 2 files changed, 85 insertions(+), 1 deletion(-)

-- 
2.41.0

[PATCH v3 1/2] drm/tests: helpers: Avoid a driver uaf

[Thomas Hellström]

when using __drm_kunit_helper_alloc_drm_device() the driver may be
dereferenced by device-managed resources up until the device is
freed, which is typically later than the kunit-managed resource code
frees it. Fix this by simply make the driver device-managed as well.

In short, the sequence leading to the UAF is as follows:

INIT:
Code allocates a struct device as a kunit-managed resource.
Code allocates a drm driver as a kunit-managed resource.
Code allocates a drm device as a device-managed resource.

EXIT:
Kunit resource cleanup frees the drm driver
Kunit resource cleanup puts the struct device, which starts a
      device-managed resource cleanup
device-managed cleanup calls drm_dev_put()
drm_dev_put() dereferences the (now freed) drm driver -> Boom.

Related KASAN message:
[55272.551542] ==================================================================
[55272.551551] BUG: KASAN: slab-use-after-free in drm_dev_put.part.0+0xd4/0xe0 [drm]
[55272.551603] Read of size 8 at addr ffff888127502828 by task kunit_try_catch/10353

[55272.551612] CPU: 4 PID: 10353 Comm: kunit_try_catch Tainted: G     U           N 6.5.0-rc7+ #155
[55272.551620] Hardware name: ASUS System Product Name/PRIME B560M-A AC, BIOS 0403 01/26/2021
[55272.551626] Call Trace:
[55272.551629]  <TASK>
[55272.551633]  dump_stack_lvl+0x57/0x90
[55272.551639]  print_report+0xcf/0x630
[55272.551645]  ? _raw_spin_lock_irqsave+0x5f/0x70
[55272.551652]  ? drm_dev_put.part.0+0xd4/0xe0 [drm]
[55272.551694]  kasan_report+0xd7/0x110
[55272.551699]  ? drm_dev_put.part.0+0xd4/0xe0 [drm]
[55272.551742]  drm_dev_put.part.0+0xd4/0xe0 [drm]
[55272.551783]  devres_release_all+0x15d/0x1f0
[55272.551790]  ? __pfx_devres_release_all+0x10/0x10
[55272.551797]  device_unbind_cleanup+0x16/0x1a0
[55272.551802]  device_release_driver_internal+0x3e5/0x540
[55272.551808]  ? kobject_put+0x5d/0x4b0
[55272.551814]  bus_remove_device+0x1f1/0x3f0
[55272.551819]  device_del+0x342/0x910
[55272.551826]  ? __pfx_device_del+0x10/0x10
[55272.551830]  ? lock_release+0x339/0x5e0
[55272.551836]  ? kunit_remove_resource+0x128/0x290 [kunit]
[55272.551845]  ? __pfx_lock_release+0x10/0x10
[55272.551851]  platform_device_del.part.0+0x1f/0x1e0
[55272.551856]  ? _raw_spin_unlock_irqrestore+0x30/0x60
[55272.551863]  kunit_remove_resource+0x195/0x290 [kunit]
[55272.551871]  ? _raw_spin_unlock_irqrestore+0x30/0x60
[55272.551877]  kunit_cleanup+0x78/0x120 [kunit]
[55272.551885]  ? __kthread_parkme+0xc1/0x1f0
[55272.551891]  ? __pfx_kunit_try_run_case_cleanup+0x10/0x10 [kunit]
[55272.551900]  ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10 [kunit]
[55272.551909]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
[55272.551919]  kthread+0x2e7/0x3c0
[55272.551924]  ? __pfx_kthread+0x10/0x10
[55272.551929]  ret_from_fork+0x2d/0x70
[55272.551935]  ? __pfx_kthread+0x10/0x10
[55272.551940]  ret_from_fork_asm+0x1b/0x30
[55272.551948]  </TASK>

[55272.551953] Allocated by task 10351:
[55272.551956]  kasan_save_stack+0x1c/0x40
[55272.551962]  kasan_set_track+0x21/0x30
[55272.551966]  __kasan_kmalloc+0x8b/0x90
[55272.551970]  __kmalloc+0x5e/0x160
[55272.551976]  kunit_kmalloc_array+0x1c/0x50 [kunit]
[55272.551984]  drm_exec_test_init+0xfa/0x2c0 [drm_exec_test]
[55272.551991]  kunit_try_run_case+0xdd/0x250 [kunit]
[55272.551999]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
[55272.552008]  kthread+0x2e7/0x3c0
[55272.552012]  ret_from_fork+0x2d/0x70
[55272.552017]  ret_from_fork_asm+0x1b/0x30

[55272.552024] Freed by task 10353:
[55272.552027]  kasan_save_stack+0x1c/0x40
[55272.552032]  kasan_set_track+0x21/0x30
[55272.552036]  kasan_save_free_info+0x27/0x40
[55272.552041]  __kasan_slab_free+0x106/0x180
[55272.552046]  slab_free_freelist_hook+0xb3/0x160
[55272.552051]  __kmem_cache_free+0xb2/0x290
[55272.552056]  kunit_remove_resource+0x195/0x290 [kunit]
[55272.552064]  kunit_cleanup+0x78/0x120 [kunit]
[55272.552072]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
[55272.552080]  kthread+0x2e7/0x3c0
[55272.552085]  ret_from_fork+0x2d/0x70
[55272.552089]  ret_from_fork_asm+0x1b/0x30

[55272.552096] The buggy address belongs to the object at ffff888127502800
                which belongs to the cache kmalloc-512 of size 512
[55272.552105] The buggy address is located 40 bytes inside of
                freed 512-byte region [ffff888127502800, ffff888127502a00)

[55272.552115] The buggy address belongs to the physical page:
[55272.552119] page:00000000af6c70ff refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x127500
[55272.552127] head:00000000af6c70ff order:3 entire_mapcount:0 nr_pages_mapped:0 pincount:0
[55272.552133] anon flags: 0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
[55272.552141] page_type: 0xffffffff()
[55272.552145] raw: 0017ffffc0010200 ffff888100042c80 0000000000000000 dead000000000001
[55272.552152] raw: 0000000000000000 0000000080200020 00000001ffffffff 0000000000000000
[55272.552157] page dumped because: kasan: bad access detected

[55272.552163] Memory state around the buggy address:
[55272.552167]  ffff888127502700: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[55272.552173]  ffff888127502780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[55272.552178] >ffff888127502800: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[55272.552184]                                   ^
[55272.552187]  ffff888127502880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[55272.552193]  ffff888127502900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[55272.552198] ==================================================================
[55272.552203] Disabling lock debugging due to kernel taint

v2:
- Update commit message, add Fixes: tag and Cc stable.
v3:
- Further commit message updates (Maxime Ripard).

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v6.3+
Fixes: d98780310719 ("drm/tests: helpers: Allow to pass a custom drm_driver")
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 include/drm/drm_kunit_helpers.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/drm/drm_kunit_helpers.h b/include/drm/drm_kunit_helpers.h
index 514c8a7a32f0..ba483c87f0e7 100644
--- a/include/drm/drm_kunit_helpers.h
+++ b/include/drm/drm_kunit_helpers.h
@@ -3,6 +3,8 @@
 #ifndef DRM_KUNIT_HELPERS_H_
 #define DRM_KUNIT_HELPERS_H_
 
+#include <linux/device.h>
+
 #include <kunit/test.h>
 
 struct drm_device;
@@ -51,7 +53,7 @@ __drm_kunit_helper_alloc_drm_device(struct kunit *test,
 {
 	struct drm_driver *driver;
 
-	driver = kunit_kzalloc(test, sizeof(*driver), GFP_KERNEL);
+	driver = devm_kzalloc(dev, sizeof(*driver), GFP_KERNEL);
 	KUNIT_ASSERT_NOT_NULL(test, driver);
 
 	driver->driver_features = features;
-- 
2.41.0

[PATCH v3 2/2] drm/tests/drm_exec: Add a test for object freeing within drm_exec_fini()

[Thomas Hellström]

Check that object freeing from within drm_exec_fini() works as expected
and is unlikely to generate any warnings.

v3:
- Condition the test on CONFIG_DEBUG_LOCK_ALLOC
- Make the test fail if the situation that generates the lockdep
  warning occurs. (Maxime Ripard)

Cc: Maxime Ripard <mripard@kernel.org>
Cc: Christian König <christian.koenig@amd.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/tests/drm_exec_test.c | 82 +++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c
index 563949d777dd..83fddc6fe1ae 100644
--- a/drivers/gpu/drm/tests/drm_exec_test.c
+++ b/drivers/gpu/drm/tests/drm_exec_test.c
@@ -21,6 +21,9 @@
 struct drm_exec_priv {
 	struct device *dev;
 	struct drm_device *drm;
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+	struct drm_exec *exec;
+#endif
 };
 
 static int drm_exec_test_init(struct kunit *test)
@@ -170,6 +173,82 @@ static void test_prepare_array(struct kunit *test)
 	drm_gem_private_object_fini(&gobj2);
 }
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+static void drm_exec_test_obj_free(struct drm_gem_object *gem)
+{
+	struct kunit *test = current->kunit_test;
+	struct drm_exec_priv *priv = test->priv;
+	bool resv_class_held;
+	bool first_object_locked;
+
+	/*
+	 * The lock alloc tracking code may warn if the dma_resv lock
+	 * class is still held, and we're freeing the first object we
+	 * locked.
+	 */
+	resv_class_held = (lockdep_is_held(&gem->resv->lock.base) ==
+			   LOCK_STATE_HELD);
+	first_object_locked = (gem == priv->exec->objects[0]);
+	KUNIT_EXPECT_FALSE(current->kunit_test,
+			   resv_class_held && first_object_locked);
+
+	dma_resv_fini(gem->resv);
+	kfree(gem);
+}
+
+static const struct drm_gem_object_funcs put_funcs = {
+	.free = drm_exec_test_obj_free,
+};
+
+/*
+ * Check that freeing objects from within drm_exec_fini()
+ * doesn't trigger a false lock alloc warning due to
+ * the dma_resv lock *class* still being held and we're
+ * freeing the first object locked, which *might* be
+ * registered as the address of the held lock of that
+ * lock class.
+ */
+static void test_early_put(struct kunit *test)
+{
+	struct drm_exec_priv *priv = test->priv;
+	struct drm_gem_object *gobj1;
+	struct drm_gem_object *gobj2;
+	struct drm_gem_object *array[2];
+	struct drm_exec exec;
+	int ret;
+
+	priv->exec = &exec;
+
+	gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL);
+	KUNIT_EXPECT_NOT_NULL(test, gobj1);
+	if (!gobj1)
+		return;
+
+	gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL);
+	KUNIT_EXPECT_NOT_NULL(test, gobj2);
+	if (!gobj2) {
+		kfree(gobj1);
+		return;
+	}
+
+	gobj1->funcs = &put_funcs;
+	gobj2->funcs = &put_funcs;
+	drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE);
+	drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE);
+	array[0] = gobj1;
+	array[1] = gobj2;
+
+	drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT);
+	drm_exec_until_all_locked(&exec)
+		ret = drm_exec_prepare_array(&exec, array, ARRAY_SIZE(array),
+					     1);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+	drm_gem_object_put(gobj1);
+	drm_gem_object_put(gobj2);
+	drm_exec_fini(&exec);
+}
+#endif
+
 static void test_multiple_loops(struct kunit *test)
 {
 	struct drm_exec exec;
@@ -198,6 +277,9 @@ static struct kunit_case drm_exec_tests[] = {
 	KUNIT_CASE(test_prepare),
 	KUNIT_CASE(test_prepare_array),
 	KUNIT_CASE(test_multiple_loops),
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+	KUNIT_CASE(test_early_put),
+#endif
 	{}
 };
 
-- 
2.41.0

Re: [PATCH v3 0/2] drm/tests: Fix for UAF and a test for drm_exec lock alloc tracking warning

[Christian König]

Am 07.09.23 um 15:53 schrieb Thomas Hellström:
> While trying to replicate a weird drm_exec lock alloc tracking warning
> using the drm_exec kunit test, the warning was shadowed by a UAF warning
> from KASAN due to a bug in the drm kunit helpers.
>
> Patch 1 fixes that drm kunit UAF.
> Patch 2 introduces a drm_exec kunit subtest that fails if the conditions
>        for the weird warning are met.
>
> The series previously also had a patch with a drm_exec workaround for the
> warning but that patch has already been commited to drm_misc_next_fixes.

Thinking more about this what happens when somebody calls 
drm_exec_unlock_obj() on the first locked object?

Christian.

>
> v2:
> - Rewording of commit messages
> - Add some commit message tags
> v3:
> - Remove an already committed patch
> - Rework the test to not require dmesg inspection (Maxime Ripard)
> - Condition the test on CONFIG_LOCK_ALLOC
> - Update code comments and commit messages (Maxime Ripard)
>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Christian König <christian.koenig@amd.com>
>
> Thomas Hellström (2):
>    drm/tests: helpers: Avoid a driver uaf
>    drm/tests/drm_exec: Add a test for object freeing within
>      drm_exec_fini()
>
>   drivers/gpu/drm/tests/drm_exec_test.c | 82 +++++++++++++++++++++++++++
>   include/drm/drm_kunit_helpers.h       |  4 +-
>   2 files changed, 85 insertions(+), 1 deletion(-)
>

Re: [PATCH v3 0/2] drm/tests: Fix for UAF and a test for drm_exec lock alloc tracking warning

[Thomas Hellström]

Hi,

On 9/7/23 16:37, Christian König wrote:
> Am 07.09.23 um 15:53 schrieb Thomas Hellström:
>> While trying to replicate a weird drm_exec lock alloc tracking warning
>> using the drm_exec kunit test, the warning was shadowed by a UAF warning
>> from KASAN due to a bug in the drm kunit helpers.
>>
>> Patch 1 fixes that drm kunit UAF.
>> Patch 2 introduces a drm_exec kunit subtest that fails if the conditions
>>        for the weird warning are met.
>>
>> The series previously also had a patch with a drm_exec workaround for 
>> the
>> warning but that patch has already been commited to drm_misc_next_fixes.
>
> Thinking more about this what happens when somebody calls 
> drm_exec_unlock_obj() on the first locked object?
>
Essentially the same thing. I've been thinking of the best way to handle 
that, but not sure what's the best one.

/Thomas


> Christian.
>
>>
>> v2:
>> - Rewording of commit messages
>> - Add some commit message tags
>> v3:
>> - Remove an already committed patch
>> - Rework the test to not require dmesg inspection (Maxime Ripard)
>> - Condition the test on CONFIG_LOCK_ALLOC
>> - Update code comments and commit messages (Maxime Ripard)
>>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Christian König <christian.koenig@amd.com>
>>
>> Thomas Hellström (2):
>>    drm/tests: helpers: Avoid a driver uaf
>>    drm/tests/drm_exec: Add a test for object freeing within
>>      drm_exec_fini()
>>
>>   drivers/gpu/drm/tests/drm_exec_test.c | 82 +++++++++++++++++++++++++++
>>   include/drm/drm_kunit_helpers.h       |  4 +-
>>   2 files changed, 85 insertions(+), 1 deletion(-)
>>
>

Re: [PATCH v3 0/2] drm/tests: Fix for UAF and a test for drm_exec lock alloc tracking warning

[Christian König]

Am 07.09.23 um 16:47 schrieb Thomas Hellström:
> Hi,
>
> On 9/7/23 16:37, Christian König wrote:
>> Am 07.09.23 um 15:53 schrieb Thomas Hellström:
>>> While trying to replicate a weird drm_exec lock alloc tracking warning
>>> using the drm_exec kunit test, the warning was shadowed by a UAF 
>>> warning
>>> from KASAN due to a bug in the drm kunit helpers.
>>>
>>> Patch 1 fixes that drm kunit UAF.
>>> Patch 2 introduces a drm_exec kunit subtest that fails if the 
>>> conditions
>>>        for the weird warning are met.
>>>
>>> The series previously also had a patch with a drm_exec workaround 
>>> for the
>>> warning but that patch has already been commited to 
>>> drm_misc_next_fixes.
>>
>> Thinking more about this what happens when somebody calls 
>> drm_exec_unlock_obj() on the first locked object?
>>
> Essentially the same thing. I've been thinking of the best way to 
> handle that, but not sure what's the best one.

Well what does lockdep store in that object in the first place? Could we 
fix that somehow?

Christian.

>
> /Thomas
>
>
>> Christian.
>>
>>>
>>> v2:
>>> - Rewording of commit messages
>>> - Add some commit message tags
>>> v3:
>>> - Remove an already committed patch
>>> - Rework the test to not require dmesg inspection (Maxime Ripard)
>>> - Condition the test on CONFIG_LOCK_ALLOC
>>> - Update code comments and commit messages (Maxime Ripard)
>>>
>>> Cc: Maxime Ripard <mripard@kernel.org>
>>> Cc: Christian König <christian.koenig@amd.com>
>>>
>>> Thomas Hellström (2):
>>>    drm/tests: helpers: Avoid a driver uaf
>>>    drm/tests/drm_exec: Add a test for object freeing within
>>>      drm_exec_fini()
>>>
>>>   drivers/gpu/drm/tests/drm_exec_test.c | 82 
>>> +++++++++++++++++++++++++++
>>>   include/drm/drm_kunit_helpers.h       |  4 +-
>>>   2 files changed, 85 insertions(+), 1 deletion(-)
>>>
>>

Re: [PATCH v3 1/2] drm/tests: helpers: Avoid a driver uaf

[Maxime Ripard]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 428 bytes --]

On Thu, 7 Sep 2023 15:53:38 +0200, Thomas Hellström wrote:
> when using __drm_kunit_helper_alloc_drm_device() the driver may be
> dereferenced by device-managed resources up until the device is
> freed, which is typically later than the kunit-managed resource code
> frees it. Fix this by simply make the driver device-managed as well.
> 
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [PATCH v3 2/2] drm/tests/drm_exec: Add a test for object freeing within drm_exec_fini()

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3784 bytes --]

Hi Thomas,

On Thu, Sep 07, 2023 at 03:53:39PM +0200, Thomas Hellström wrote:
> Check that object freeing from within drm_exec_fini() works as expected
> and is unlikely to generate any warnings.
> 
> v3:
> - Condition the test on CONFIG_DEBUG_LOCK_ALLOC
> - Make the test fail if the situation that generates the lockdep
>   warning occurs. (Maxime Ripard)
> 
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>  drivers/gpu/drm/tests/drm_exec_test.c | 82 +++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c
> index 563949d777dd..83fddc6fe1ae 100644
> --- a/drivers/gpu/drm/tests/drm_exec_test.c
> +++ b/drivers/gpu/drm/tests/drm_exec_test.c
> @@ -21,6 +21,9 @@
>  struct drm_exec_priv {
>  	struct device *dev;
>  	struct drm_device *drm;
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +	struct drm_exec *exec;
> +#endif
>  };
>  
>  static int drm_exec_test_init(struct kunit *test)
> @@ -170,6 +173,82 @@ static void test_prepare_array(struct kunit *test)
>  	drm_gem_private_object_fini(&gobj2);
>  }
>  
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +static void drm_exec_test_obj_free(struct drm_gem_object *gem)
> +{
> +	struct kunit *test = current->kunit_test;
> +	struct drm_exec_priv *priv = test->priv;
> +	bool resv_class_held;
> +	bool first_object_locked;
> +
> +	/*
> +	 * The lock alloc tracking code may warn if the dma_resv lock
> +	 * class is still held, and we're freeing the first object we
> +	 * locked.
> +	 */
> +	resv_class_held = (lockdep_is_held(&gem->resv->lock.base) ==
> +			   LOCK_STATE_HELD);
> +	first_object_locked = (gem == priv->exec->objects[0]);
> +	KUNIT_EXPECT_FALSE(current->kunit_test,
> +			   resv_class_held && first_object_locked);
> +
> +	dma_resv_fini(gem->resv);
> +	kfree(gem);
> +}
> +
> +static const struct drm_gem_object_funcs put_funcs = {
> +	.free = drm_exec_test_obj_free,
> +};
> +
> +/*
> + * Check that freeing objects from within drm_exec_fini()
> + * doesn't trigger a false lock alloc warning due to
> + * the dma_resv lock *class* still being held and we're
> + * freeing the first object locked, which *might* be
> + * registered as the address of the held lock of that
> + * lock class.
> + */
> +static void test_early_put(struct kunit *test)
> +{
> +	struct drm_exec_priv *priv = test->priv;
> +	struct drm_gem_object *gobj1;
> +	struct drm_gem_object *gobj2;
> +	struct drm_gem_object *array[2];
> +	struct drm_exec exec;
> +	int ret;
> +
> +	priv->exec = &exec;
> +
> +	gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL);
> +	KUNIT_EXPECT_NOT_NULL(test, gobj1);
> +	if (!gobj1)
> +		return;
> +
> +	gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL);
> +	KUNIT_EXPECT_NOT_NULL(test, gobj2);
> +	if (!gobj2) {
> +		kfree(gobj1);
> +		return;
> +	}
> +
> +	gobj1->funcs = &put_funcs;
> +	gobj2->funcs = &put_funcs;
> +	drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE);
> +	drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE);
> +	array[0] = gobj1;
> +	array[1] = gobj2;
> +
> +	drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT);
> +	drm_exec_until_all_locked(&exec)
> +		ret = drm_exec_prepare_array(&exec, array, ARRAY_SIZE(array),
> +					     1);
> +	KUNIT_EXPECT_EQ(test, ret, 0);
> +	drm_gem_object_put(gobj1);
> +	drm_gem_object_put(gobj2);
> +	drm_exec_fini(&exec);
> +}
> +#endif

We might want to revisit this later depending on the answer from the
kunit maintainers, but for now

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 0/2] drm/tests: Fix for UAF and a test for drm_exec lock alloc tracking warning

[Thomas Hellström]

Hi,

On 9/7/23 16:49, Christian König wrote:
> Am 07.09.23 um 16:47 schrieb Thomas Hellström:
>> Hi,
>>
>> On 9/7/23 16:37, Christian König wrote:
>>> Am 07.09.23 um 15:53 schrieb Thomas Hellström:
>>>> While trying to replicate a weird drm_exec lock alloc tracking warning
>>>> using the drm_exec kunit test, the warning was shadowed by a UAF 
>>>> warning
>>>> from KASAN due to a bug in the drm kunit helpers.
>>>>
>>>> Patch 1 fixes that drm kunit UAF.
>>>> Patch 2 introduces a drm_exec kunit subtest that fails if the 
>>>> conditions
>>>>        for the weird warning are met.
>>>>
>>>> The series previously also had a patch with a drm_exec workaround 
>>>> for the
>>>> warning but that patch has already been commited to 
>>>> drm_misc_next_fixes.
>>>
>>> Thinking more about this what happens when somebody calls 
>>> drm_exec_unlock_obj() on the first locked object?
>>>
>> Essentially the same thing. I've been thinking of the best way to 
>> handle that, but not sure what's the best one.
>
> Well what does lockdep store in that object in the first place? Could 
> we fix that somehow?

Lockdep maintains an array of held locks (lock classes) for each task. 
Upon freeing, that list is traversed to see if the address matches the 
stored memory address. This also has the interesting side effect that 
IICR dma_resv_assert_held() checks if *any* dma_resv is held....

Ideally each object would have its own class instance, but I think some 
applications would then exhaust the array size.

I'll dig a bit deeper into this.


Meanwhile for the unlock problem, looking at how the unlocks are used in 
i915 it's typically locks that are grabbed during eviction and released 
again once validation of a single object succeeded. The risk of them 
ending up at the first lock is small, unless they are prelocked as the 
contended lock. But for these "temporary" objects, the prelocked lock is 
immediately dropped after locking and are only used to find something 
suitable to wait for to relax the ww transaction.

If we were to implement something similar in drm_exec, we'd need an 
interface to mark an object as "temporary" when locking, and make sure 
we drop those objects if they end up as "prelocked". Personally I think 
this solution works well and would be my preferred choice.

Yet another alternative would be to keep a reference even of the 
unlocked objects...

But these workarounds ofc only push the problem out of drm_exec. Users 
of raw dma-resv or ww mutexes would still wonder what's going on.

/Thomas



>
> Christian.
>
>>
>> /Thomas
>>
>>
>>> Christian.
>>>
>>>>
>>>> v2:
>>>> - Rewording of commit messages
>>>> - Add some commit message tags
>>>> v3:
>>>> - Remove an already committed patch
>>>> - Rework the test to not require dmesg inspection (Maxime Ripard)
>>>> - Condition the test on CONFIG_LOCK_ALLOC
>>>> - Update code comments and commit messages (Maxime Ripard)
>>>>
>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>> Cc: Christian König <christian.koenig@amd.com>
>>>>
>>>> Thomas Hellström (2):
>>>>    drm/tests: helpers: Avoid a driver uaf
>>>>    drm/tests/drm_exec: Add a test for object freeing within
>>>>      drm_exec_fini()
>>>>
>>>>   drivers/gpu/drm/tests/drm_exec_test.c | 82 
>>>> +++++++++++++++++++++++++++
>>>>   include/drm/drm_kunit_helpers.h       |  4 +-
>>>>   2 files changed, 85 insertions(+), 1 deletion(-)
>>>>
>>>
>

Re: [PATCH v3 0/2] drm/tests: Fix for UAF and a test for drm_exec lock alloc tracking warning

[Christian König]

Am 08.09.23 um 09:37 schrieb Thomas Hellström:
> Hi,
>
> On 9/7/23 16:49, Christian König wrote:
>> Am 07.09.23 um 16:47 schrieb Thomas Hellström:
>>> Hi,
>>>
>>> On 9/7/23 16:37, Christian König wrote:
>>>> Am 07.09.23 um 15:53 schrieb Thomas Hellström:
>>>>> While trying to replicate a weird drm_exec lock alloc tracking 
>>>>> warning
>>>>> using the drm_exec kunit test, the warning was shadowed by a UAF 
>>>>> warning
>>>>> from KASAN due to a bug in the drm kunit helpers.
>>>>>
>>>>> Patch 1 fixes that drm kunit UAF.
>>>>> Patch 2 introduces a drm_exec kunit subtest that fails if the 
>>>>> conditions
>>>>>        for the weird warning are met.
>>>>>
>>>>> The series previously also had a patch with a drm_exec workaround 
>>>>> for the
>>>>> warning but that patch has already been commited to 
>>>>> drm_misc_next_fixes.
>>>>
>>>> Thinking more about this what happens when somebody calls 
>>>> drm_exec_unlock_obj() on the first locked object?
>>>>
>>> Essentially the same thing. I've been thinking of the best way to 
>>> handle that, but not sure what's the best one.
>>
>> Well what does lockdep store in that object in the first place? Could 
>> we fix that somehow?
>
> Lockdep maintains an array of held locks (lock classes) for each task. 
> Upon freeing, that list is traversed to see if the address matches the 
> stored memory address. This also has the interesting side effect that 
> IICR dma_resv_assert_held() checks if *any* dma_resv is held....
>
> Ideally each object would have its own class instance, but I think 
> some applications would then exhaust the array size.

IIRC Daniel once explained to me that he designed lockdep for ww_mutexes 
like this for some reason, but I don't remember the details any more.

Maybe lockdep wouldn't otherwise be able to deal with the fact that you 
could lock them in any order or something like that.

>
>
> I'll dig a bit deeper into this.
>
>
> Meanwhile for the unlock problem, looking at how the unlocks are used 
> in i915 it's typically locks that are grabbed during eviction and 
> released again once validation of a single object succeeded. The risk 
> of them ending up at the first lock is small, unless they are 
> prelocked as the contended lock. But for these "temporary" objects, 
> the prelocked lock is immediately dropped after locking and are only 
> used to find something suitable to wait for to relax the ww transaction.

Yeah, I don't see this as an use case in reality. It's more of a "what 
if?" thing.

>
> If we were to implement something similar in drm_exec, we'd need an 
> interface to mark an object as "temporary" when locking, and make sure 
> we drop those objects if they end up as "prelocked". Personally I 
> think this solution works well and would be my preferred choice.
>
> Yet another alternative would be to keep a reference even of the 
> unlocked objects...
>
> But these workarounds ofc only push the problem out of drm_exec. Users 
> of raw dma-resv or ww mutexes would still wonder what's going on.

Agree, completely. This is really a bug in lockdep or rather how we 
designed to implement ww_mutexes in lockdep and should therefore be 
fixed there I think.

Christian.

>
> /Thomas
>
>
>
>>
>> Christian.
>>
>>>
>>> /Thomas
>>>
>>>
>>>> Christian.
>>>>
>>>>>
>>>>> v2:
>>>>> - Rewording of commit messages
>>>>> - Add some commit message tags
>>>>> v3:
>>>>> - Remove an already committed patch
>>>>> - Rework the test to not require dmesg inspection (Maxime Ripard)
>>>>> - Condition the test on CONFIG_LOCK_ALLOC
>>>>> - Update code comments and commit messages (Maxime Ripard)
>>>>>
>>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>>> Cc: Christian König <christian.koenig@amd.com>
>>>>>
>>>>> Thomas Hellström (2):
>>>>>    drm/tests: helpers: Avoid a driver uaf
>>>>>    drm/tests/drm_exec: Add a test for object freeing within
>>>>>      drm_exec_fini()
>>>>>
>>>>>   drivers/gpu/drm/tests/drm_exec_test.c | 82 
>>>>> +++++++++++++++++++++++++++
>>>>>   include/drm/drm_kunit_helpers.h       |  4 +-
>>>>>   2 files changed, 85 insertions(+), 1 deletion(-)
>>>>>
>>>>
>>

Re: [PATCH v3 0/2] drm/tests: Fix for UAF and a test for drm_exec lock alloc tracking warning

[Thomas Hellström]

On 9/8/23 10:52, Christian König wrote:
> Am 08.09.23 um 09:37 schrieb Thomas Hellström:
>> Hi,
>>
>> On 9/7/23 16:49, Christian König wrote:
>>> Am 07.09.23 um 16:47 schrieb Thomas Hellström:
>>>> Hi,
>>>>
>>>> On 9/7/23 16:37, Christian König wrote:
>>>>> Am 07.09.23 um 15:53 schrieb Thomas Hellström:
>>>>>> While trying to replicate a weird drm_exec lock alloc tracking 
>>>>>> warning
>>>>>> using the drm_exec kunit test, the warning was shadowed by a UAF 
>>>>>> warning
>>>>>> from KASAN due to a bug in the drm kunit helpers.
>>>>>>
>>>>>> Patch 1 fixes that drm kunit UAF.
>>>>>> Patch 2 introduces a drm_exec kunit subtest that fails if the 
>>>>>> conditions
>>>>>>        for the weird warning are met.
>>>>>>
>>>>>> The series previously also had a patch with a drm_exec workaround 
>>>>>> for the
>>>>>> warning but that patch has already been commited to 
>>>>>> drm_misc_next_fixes.
>>>>>
>>>>> Thinking more about this what happens when somebody calls 
>>>>> drm_exec_unlock_obj() on the first locked object?
>>>>>
>>>> Essentially the same thing. I've been thinking of the best way to 
>>>> handle that, but not sure what's the best one.
>>>
>>> Well what does lockdep store in that object in the first place? 
>>> Could we fix that somehow?
>>
>> Lockdep maintains an array of held locks (lock classes) for each 
>> task. Upon freeing, that list is traversed to see if the address 
>> matches the stored memory address. This also has the interesting side 
>> effect that IICR dma_resv_assert_held() checks if *any* dma_resv is 
>> held....
>>
>> Ideally each object would have its own class instance, but I think 
>> some applications would then exhaust the array size.
>
> IIRC Daniel once explained to me that he designed lockdep for 
> ww_mutexes like this for some reason, but I don't remember the details 
> any more.
>
> Maybe lockdep wouldn't otherwise be able to deal with the fact that 
> you could lock them in any order or something like that.

Oh, that's well handled with the mutex_lock_nest_lock()  type of 
annotation that's used for WW mutexes. IIRC the problem is that lockdep 
can't really deal with either that vast number of locks overall or the 
vast number of held locks per process.


>
>
>>
>>
>> I'll dig a bit deeper into this.
>>
>>
>> Meanwhile for the unlock problem, looking at how the unlocks are used 
>> in i915 it's typically locks that are grabbed during eviction and 
>> released again once validation of a single object succeeded. The risk 
>> of them ending up at the first lock is small, unless they are 
>> prelocked as the contended lock. But for these "temporary" objects, 
>> the prelocked lock is immediately dropped after locking and are only 
>> used to find something suitable to wait for to relax the ww transaction.
>
> Yeah, I don't see this as an use case in reality. It's more of a "what 
> if?" thing.

Oh, it's a real use-case. As soon as you start having sleeping locks for 
eviction you hit it, in particular with WW mutex slowpath debugging. And 
we will need to work on improving TTM support for that for xe.

>
>>
>> If we were to implement something similar in drm_exec, we'd need an 
>> interface to mark an object as "temporary" when locking, and make 
>> sure we drop those objects if they end up as "prelocked". Personally 
>> I think this solution works well and would be my preferred choice.
>>
>> Yet another alternative would be to keep a reference even of the 
>> unlocked objects...
>>
>> But these workarounds ofc only push the problem out of drm_exec. 
>> Users of raw dma-resv or ww mutexes would still wonder what's going on.
>
> Agree, completely. This is really a bug in lockdep or rather how we 
> designed to implement ww_mutexes in lockdep and should therefore be 
> fixed there I think.


>
> Christian.
>
>>
>> /Thomas
>>
>>
>>
>>>
>>> Christian.
>>>
>>>>
>>>> /Thomas
>>>>
>>>>
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> v2:
>>>>>> - Rewording of commit messages
>>>>>> - Add some commit message tags
>>>>>> v3:
>>>>>> - Remove an already committed patch
>>>>>> - Rework the test to not require dmesg inspection (Maxime Ripard)
>>>>>> - Condition the test on CONFIG_LOCK_ALLOC
>>>>>> - Update code comments and commit messages (Maxime Ripard)
>>>>>>
>>>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>>>> Cc: Christian König <christian.koenig@amd.com>
>>>>>>
>>>>>> Thomas Hellström (2):
>>>>>>    drm/tests: helpers: Avoid a driver uaf
>>>>>>    drm/tests/drm_exec: Add a test for object freeing within
>>>>>>      drm_exec_fini()
>>>>>>
>>>>>>   drivers/gpu/drm/tests/drm_exec_test.c | 82 
>>>>>> +++++++++++++++++++++++++++
>>>>>>   include/drm/drm_kunit_helpers.h       |  4 +-
>>>>>>   2 files changed, 85 insertions(+), 1 deletion(-)
>>>>>>
>>>>>
>>>
>

Re: [PATCH v3 0/2] drm/tests: Fix for UAF and a test for drm_exec lock alloc tracking warning

[Christian König]

Am 08.09.23 um 11:04 schrieb Thomas Hellström:
>
> On 9/8/23 10:52, Christian König wrote:
>> Am 08.09.23 um 09:37 schrieb Thomas Hellström:
>>> Hi,
>>>
>>> On 9/7/23 16:49, Christian König wrote:
>>>> Am 07.09.23 um 16:47 schrieb Thomas Hellström:
>>>>> Hi,
>>>>>
>>>>> On 9/7/23 16:37, Christian König wrote:
>>>>>> Am 07.09.23 um 15:53 schrieb Thomas Hellström:
>>>>>>> While trying to replicate a weird drm_exec lock alloc tracking 
>>>>>>> warning
>>>>>>> using the drm_exec kunit test, the warning was shadowed by a UAF 
>>>>>>> warning
>>>>>>> from KASAN due to a bug in the drm kunit helpers.
>>>>>>>
>>>>>>> Patch 1 fixes that drm kunit UAF.
>>>>>>> Patch 2 introduces a drm_exec kunit subtest that fails if the 
>>>>>>> conditions
>>>>>>>        for the weird warning are met.
>>>>>>>
>>>>>>> The series previously also had a patch with a drm_exec 
>>>>>>> workaround for the
>>>>>>> warning but that patch has already been commited to 
>>>>>>> drm_misc_next_fixes.
>>>>>>
>>>>>> Thinking more about this what happens when somebody calls 
>>>>>> drm_exec_unlock_obj() on the first locked object?
>>>>>>
>>>>> Essentially the same thing. I've been thinking of the best way to 
>>>>> handle that, but not sure what's the best one.
>>>>
>>>> Well what does lockdep store in that object in the first place? 
>>>> Could we fix that somehow?
>>>
>>> Lockdep maintains an array of held locks (lock classes) for each 
>>> task. Upon freeing, that list is traversed to see if the address 
>>> matches the stored memory address. This also has the interesting 
>>> side effect that IICR dma_resv_assert_held() checks if *any* 
>>> dma_resv is held....
>>>
>>> Ideally each object would have its own class instance, but I think 
>>> some applications would then exhaust the array size.
>>
>> IIRC Daniel once explained to me that he designed lockdep for 
>> ww_mutexes like this for some reason, but I don't remember the 
>> details any more.
>>
>> Maybe lockdep wouldn't otherwise be able to deal with the fact that 
>> you could lock them in any order or something like that.
>
> Oh, that's well handled with the mutex_lock_nest_lock()  type of 
> annotation that's used for WW mutexes. IIRC the problem is that 
> lockdep can't really deal with either that vast number of locks 
> overall or the vast number of held locks per process.

Could we somehow teach lockdep that multiple locks of a lock class can 
be held at the same time? E.g. like a reference count in the lockclass 
or something like that?

>
>>
>>>
>>> I'll dig a bit deeper into this.
>>>
>>>
>>> Meanwhile for the unlock problem, looking at how the unlocks are 
>>> used in i915 it's typically locks that are grabbed during eviction 
>>> and released again once validation of a single object succeeded. The 
>>> risk of them ending up at the first lock is small, unless they are 
>>> prelocked as the contended lock. But for these "temporary" objects, 
>>> the prelocked lock is immediately dropped after locking and are only 
>>> used to find something suitable to wait for to relax the ww 
>>> transaction.
>>
>> Yeah, I don't see this as an use case in reality. It's more of a 
>> "what if?" thing.
>
> Oh, it's a real use-case. As soon as you start having sleeping locks 
> for eviction you hit it, in particular with WW mutex slowpath 
> debugging. And we will need to work on improving TTM support for that 
> for xe.

Oh, good point! When we have contention on a lock, rollback and take 
that lock then first it can be that this lock then needs to be unlocked 
again. Unlikely, but certainly possible.

Sounds like we really need to fix this in lockdep then.

Christian.

>
>>
>>>
>>> If we were to implement something similar in drm_exec, we'd need an 
>>> interface to mark an object as "temporary" when locking, and make 
>>> sure we drop those objects if they end up as "prelocked". Personally 
>>> I think this solution works well and would be my preferred choice.
>>>
>>> Yet another alternative would be to keep a reference even of the 
>>> unlocked objects...
>>>
>>> But these workarounds ofc only push the problem out of drm_exec. 
>>> Users of raw dma-resv or ww mutexes would still wonder what's going on.
>>
>> Agree, completely. This is really a bug in lockdep or rather how we 
>> designed to implement ww_mutexes in lockdep and should therefore be 
>> fixed there I think.
>
>
>>
>> Christian.
>>
>>>
>>> /Thomas
>>>
>>>
>>>
>>>>
>>>> Christian.
>>>>
>>>>>
>>>>> /Thomas
>>>>>
>>>>>
>>>>>> Christian.
>>>>>>
>>>>>>>
>>>>>>> v2:
>>>>>>> - Rewording of commit messages
>>>>>>> - Add some commit message tags
>>>>>>> v3:
>>>>>>> - Remove an already committed patch
>>>>>>> - Rework the test to not require dmesg inspection (Maxime Ripard)
>>>>>>> - Condition the test on CONFIG_LOCK_ALLOC
>>>>>>> - Update code comments and commit messages (Maxime Ripard)
>>>>>>>
>>>>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>>>>> Cc: Christian König <christian.koenig@amd.com>
>>>>>>>
>>>>>>> Thomas Hellström (2):
>>>>>>>    drm/tests: helpers: Avoid a driver uaf
>>>>>>>    drm/tests/drm_exec: Add a test for object freeing within
>>>>>>>      drm_exec_fini()
>>>>>>>
>>>>>>>   drivers/gpu/drm/tests/drm_exec_test.c | 82 
>>>>>>> +++++++++++++++++++++++++++
>>>>>>>   include/drm/drm_kunit_helpers.h       |  4 +-
>>>>>>>   2 files changed, 85 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>
>>>>
>>

Re: [PATCH v3 0/2] drm/tests: Fix for UAF and a test for drm_exec lock alloc tracking warning

[Thomas Hellström]

On 9/8/23 11:14, Christian König wrote:
> Am 08.09.23 um 11:04 schrieb Thomas Hellström:
>>
>> On 9/8/23 10:52, Christian König wrote:
>>> Am 08.09.23 um 09:37 schrieb Thomas Hellström:
>>>> Hi,
>>>>
>>>> On 9/7/23 16:49, Christian König wrote:
>>>>> Am 07.09.23 um 16:47 schrieb Thomas Hellström:
>>>>>> Hi,
>>>>>>
>>>>>> On 9/7/23 16:37, Christian König wrote:
>>>>>>> Am 07.09.23 um 15:53 schrieb Thomas Hellström:
>>>>>>>> While trying to replicate a weird drm_exec lock alloc tracking 
>>>>>>>> warning
>>>>>>>> using the drm_exec kunit test, the warning was shadowed by a 
>>>>>>>> UAF warning
>>>>>>>> from KASAN due to a bug in the drm kunit helpers.
>>>>>>>>
>>>>>>>> Patch 1 fixes that drm kunit UAF.
>>>>>>>> Patch 2 introduces a drm_exec kunit subtest that fails if the 
>>>>>>>> conditions
>>>>>>>>        for the weird warning are met.
>>>>>>>>
>>>>>>>> The series previously also had a patch with a drm_exec 
>>>>>>>> workaround for the
>>>>>>>> warning but that patch has already been commited to 
>>>>>>>> drm_misc_next_fixes.
>>>>>>>
>>>>>>> Thinking more about this what happens when somebody calls 
>>>>>>> drm_exec_unlock_obj() on the first locked object?
>>>>>>>
>>>>>> Essentially the same thing. I've been thinking of the best way to 
>>>>>> handle that, but not sure what's the best one.
>>>>>
>>>>> Well what does lockdep store in that object in the first place? 
>>>>> Could we fix that somehow?
>>>>
>>>> Lockdep maintains an array of held locks (lock classes) for each 
>>>> task. Upon freeing, that list is traversed to see if the address 
>>>> matches the stored memory address. This also has the interesting 
>>>> side effect that IICR dma_resv_assert_held() checks if *any* 
>>>> dma_resv is held....
>>>>
>>>> Ideally each object would have its own class instance, but I think 
>>>> some applications would then exhaust the array size.
>>>
>>> IIRC Daniel once explained to me that he designed lockdep for 
>>> ww_mutexes like this for some reason, but I don't remember the 
>>> details any more.
>>>
>>> Maybe lockdep wouldn't otherwise be able to deal with the fact that 
>>> you could lock them in any order or something like that.
>>
>> Oh, that's well handled with the mutex_lock_nest_lock()  type of 
>> annotation that's used for WW mutexes. IIRC the problem is that 
>> lockdep can't really deal with either that vast number of locks 
>> overall or the vast number of held locks per process.
>
> Could we somehow teach lockdep that multiple locks of a lock class can 
> be held at the same time? E.g. like a reference count in the lockclass 
> or something like that?
>
>>
>>>
>>>>
>>>> I'll dig a bit deeper into this.
>>>>
>>>>
>>>> Meanwhile for the unlock problem, looking at how the unlocks are 
>>>> used in i915 it's typically locks that are grabbed during eviction 
>>>> and released again once validation of a single object succeeded. 
>>>> The risk of them ending up at the first lock is small, unless they 
>>>> are prelocked as the contended lock. But for these "temporary" 
>>>> objects, the prelocked lock is immediately dropped after locking 
>>>> and are only used to find something suitable to wait for to relax 
>>>> the ww transaction.
>>>
>>> Yeah, I don't see this as an use case in reality. It's more of a 
>>> "what if?" thing.
>>
>> Oh, it's a real use-case. As soon as you start having sleeping locks 
>> for eviction you hit it, in particular with WW mutex slowpath 
>> debugging. And we will need to work on improving TTM support for that 
>> for xe.
>
> Oh, good point! When we have contention on a lock, rollback and take 
> that lock then first it can be that this lock then needs to be 
> unlocked again. Unlikely, but certainly possible.
>
> Sounds like we really need to fix this in lockdep then.

So it seems lockdep *does* reference counting in this case, but stores 
the address of the first locked lockdep map, and then subsequently uses 
it for various things. In short freeing the first lock isn't something 
lockdep thinks you should do. Ever.

The good thing about this is that this refcounting appears only done on 
nest locks, that is, when we have a ww context AFAICT. That means we can 
probably store a fake ww_mutex lockdep map with the ww acquire context 
and lock it when we initialize the context and unlock it on 
ww_acquire_fini().

Should take care of the problem I think, although the problem of 
lockdep_assert() and lock freeing granularity will remain. It looks like 
there is a comparison function one can optionally set to make different 
objects look separate to lockdep. Probably something to think of for 
enhanced debugging with a limited set of locked objects.

Need to also check what happens if we do a sequence of successful trylocks.

/Thomas

>
> Christian.
>
>>
>>>
>>>>
>>>> If we were to implement something similar in drm_exec, we'd need an 
>>>> interface to mark an object as "temporary" when locking, and make 
>>>> sure we drop those objects if they end up as "prelocked". 
>>>> Personally I think this solution works well and would be my 
>>>> preferred choice.
>>>>
>>>> Yet another alternative would be to keep a reference even of the 
>>>> unlocked objects...
>>>>
>>>> But these workarounds ofc only push the problem out of drm_exec. 
>>>> Users of raw dma-resv or ww mutexes would still wonder what's going 
>>>> on.
>>>
>>> Agree, completely. This is really a bug in lockdep or rather how we 
>>> designed to implement ww_mutexes in lockdep and should therefore be 
>>> fixed there I think.
>>
>>
>>>
>>> Christian.
>>>
>>>>
>>>> /Thomas
>>>>
>>>>
>>>>
>>>>>
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> /Thomas
>>>>>>
>>>>>>
>>>>>>> Christian.
>>>>>>>
>>>>>>>>
>>>>>>>> v2:
>>>>>>>> - Rewording of commit messages
>>>>>>>> - Add some commit message tags
>>>>>>>> v3:
>>>>>>>> - Remove an already committed patch
>>>>>>>> - Rework the test to not require dmesg inspection (Maxime Ripard)
>>>>>>>> - Condition the test on CONFIG_LOCK_ALLOC
>>>>>>>> - Update code comments and commit messages (Maxime Ripard)
>>>>>>>>
>>>>>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>>>>>> Cc: Christian König <christian.koenig@amd.com>
>>>>>>>>
>>>>>>>> Thomas Hellström (2):
>>>>>>>>    drm/tests: helpers: Avoid a driver uaf
>>>>>>>>    drm/tests/drm_exec: Add a test for object freeing within
>>>>>>>>      drm_exec_fini()
>>>>>>>>
>>>>>>>>   drivers/gpu/drm/tests/drm_exec_test.c | 82 
>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>   include/drm/drm_kunit_helpers.h       |  4 +-
>>>>>>>>   2 files changed, 85 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>
>>>>>
>>>
>

Re: [PATCH v3 0/2] drm/tests: Fix for UAF and a test for drm_exec lock alloc tracking warning

[Thomas Hellström]

On 9/8/23 13:13, Thomas Hellström wrote:
>
> On 9/8/23 11:14, Christian König wrote:
>> Am 08.09.23 um 11:04 schrieb Thomas Hellström:
>>>
>>> On 9/8/23 10:52, Christian König wrote:
>>>> Am 08.09.23 um 09:37 schrieb Thomas Hellström:
>>>>> Hi,
>>>>>
>>>>> On 9/7/23 16:49, Christian König wrote:
>>>>>> Am 07.09.23 um 16:47 schrieb Thomas Hellström:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 9/7/23 16:37, Christian König wrote:
>>>>>>>> Am 07.09.23 um 15:53 schrieb Thomas Hellström:
>>>>>>>>> While trying to replicate a weird drm_exec lock alloc tracking 
>>>>>>>>> warning
>>>>>>>>> using the drm_exec kunit test, the warning was shadowed by a 
>>>>>>>>> UAF warning
>>>>>>>>> from KASAN due to a bug in the drm kunit helpers.
>>>>>>>>>
>>>>>>>>> Patch 1 fixes that drm kunit UAF.
>>>>>>>>> Patch 2 introduces a drm_exec kunit subtest that fails if the 
>>>>>>>>> conditions
>>>>>>>>>        for the weird warning are met.
>>>>>>>>>
>>>>>>>>> The series previously also had a patch with a drm_exec 
>>>>>>>>> workaround for the
>>>>>>>>> warning but that patch has already been commited to 
>>>>>>>>> drm_misc_next_fixes.
>>>>>>>>
>>>>>>>> Thinking more about this what happens when somebody calls 
>>>>>>>> drm_exec_unlock_obj() on the first locked object?
>>>>>>>>
>>>>>>> Essentially the same thing. I've been thinking of the best way 
>>>>>>> to handle that, but not sure what's the best one.
>>>>>>
>>>>>> Well what does lockdep store in that object in the first place? 
>>>>>> Could we fix that somehow?
>>>>>
>>>>> Lockdep maintains an array of held locks (lock classes) for each 
>>>>> task. Upon freeing, that list is traversed to see if the address 
>>>>> matches the stored memory address. This also has the interesting 
>>>>> side effect that IICR dma_resv_assert_held() checks if *any* 
>>>>> dma_resv is held....
>>>>>
>>>>> Ideally each object would have its own class instance, but I think 
>>>>> some applications would then exhaust the array size.
>>>>
>>>> IIRC Daniel once explained to me that he designed lockdep for 
>>>> ww_mutexes like this for some reason, but I don't remember the 
>>>> details any more.
>>>>
>>>> Maybe lockdep wouldn't otherwise be able to deal with the fact that 
>>>> you could lock them in any order or something like that.
>>>
>>> Oh, that's well handled with the mutex_lock_nest_lock()  type of 
>>> annotation that's used for WW mutexes. IIRC the problem is that 
>>> lockdep can't really deal with either that vast number of locks 
>>> overall or the vast number of held locks per process.
>>
>> Could we somehow teach lockdep that multiple locks of a lock class 
>> can be held at the same time? E.g. like a reference count in the 
>> lockclass or something like that?
>>
>>>
>>>>
>>>>>
>>>>> I'll dig a bit deeper into this.
>>>>>
>>>>>
>>>>> Meanwhile for the unlock problem, looking at how the unlocks are 
>>>>> used in i915 it's typically locks that are grabbed during eviction 
>>>>> and released again once validation of a single object succeeded. 
>>>>> The risk of them ending up at the first lock is small, unless they 
>>>>> are prelocked as the contended lock. But for these "temporary" 
>>>>> objects, the prelocked lock is immediately dropped after locking 
>>>>> and are only used to find something suitable to wait for to relax 
>>>>> the ww transaction.
>>>>
>>>> Yeah, I don't see this as an use case in reality. It's more of a 
>>>> "what if?" thing.
>>>
>>> Oh, it's a real use-case. As soon as you start having sleeping locks 
>>> for eviction you hit it, in particular with WW mutex slowpath 
>>> debugging. And we will need to work on improving TTM support for 
>>> that for xe.
>>
>> Oh, good point! When we have contention on a lock, rollback and take 
>> that lock then first it can be that this lock then needs to be 
>> unlocked again. Unlikely, but certainly possible.
>>
>> Sounds like we really need to fix this in lockdep then.
>
> So it seems lockdep *does* reference counting in this case, but stores 
> the address of the first locked lockdep map, and then subsequently 
> uses it for various things. In short freeing the first lock isn't 
> something lockdep thinks you should do. Ever.
>
> The good thing about this is that this refcounting appears only done 
> on nest locks, that is, when we have a ww context AFAICT. That means 
> we can probably store a fake ww_mutex lockdep map with the ww acquire 
> context and lock it when we initialize the context and unlock it on 
> ww_acquire_fini().
>
> Should take care of the problem I think, although the problem of 
> lockdep_assert() and lock freeing granularity will remain. It looks 
> like there is a comparison function one can optionally set to make 
> different objects look separate to lockdep. Probably something to 
> think of for enhanced debugging with a limited set of locked objects.
>
> Need to also check what happens if we do a sequence of successful 
> trylocks.

OK, nested trylocks indeed seem to store one instance per lock, so not 
prone to the problem.

For locks under a ww_acquire_ctx, the solution outlined above appears to 
work, and it's restricted to lockdep code only.

/Thomas


>
> /Thomas
>
>>
>> Christian.
>>
>>>
>>>>
>>>>>
>>>>> If we were to implement something similar in drm_exec, we'd need 
>>>>> an interface to mark an object as "temporary" when locking, and 
>>>>> make sure we drop those objects if they end up as "prelocked". 
>>>>> Personally I think this solution works well and would be my 
>>>>> preferred choice.
>>>>>
>>>>> Yet another alternative would be to keep a reference even of the 
>>>>> unlocked objects...
>>>>>
>>>>> But these workarounds ofc only push the problem out of drm_exec. 
>>>>> Users of raw dma-resv or ww mutexes would still wonder what's 
>>>>> going on.
>>>>
>>>> Agree, completely. This is really a bug in lockdep or rather how we 
>>>> designed to implement ww_mutexes in lockdep and should therefore be 
>>>> fixed there I think.
>>>
>>>
>>>>
>>>> Christian.
>>>>
>>>>>
>>>>> /Thomas
>>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Christian.
>>>>>>
>>>>>>>
>>>>>>> /Thomas
>>>>>>>
>>>>>>>
>>>>>>>> Christian.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> v2:
>>>>>>>>> - Rewording of commit messages
>>>>>>>>> - Add some commit message tags
>>>>>>>>> v3:
>>>>>>>>> - Remove an already committed patch
>>>>>>>>> - Rework the test to not require dmesg inspection (Maxime Ripard)
>>>>>>>>> - Condition the test on CONFIG_LOCK_ALLOC
>>>>>>>>> - Update code comments and commit messages (Maxime Ripard)
>>>>>>>>>
>>>>>>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>>>>>>> Cc: Christian König <christian.koenig@amd.com>
>>>>>>>>>
>>>>>>>>> Thomas Hellström (2):
>>>>>>>>>    drm/tests: helpers: Avoid a driver uaf
>>>>>>>>>    drm/tests/drm_exec: Add a test for object freeing within
>>>>>>>>>      drm_exec_fini()
>>>>>>>>>
>>>>>>>>>   drivers/gpu/drm/tests/drm_exec_test.c | 82 
>>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>   include/drm/drm_kunit_helpers.h       |  4 +-
>>>>>>>>>   2 files changed, 85 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>

Re: [Intel-xe] [PATCH v3 1/2] drm/tests: helpers: Avoid a driver uaf

[Francois Dugast]

On Thu, Sep 07, 2023 at 03:53:38PM +0200, Thomas Hellström wrote:
> when using __drm_kunit_helper_alloc_drm_device() the driver may be
> dereferenced by device-managed resources up until the device is
> freed, which is typically later than the kunit-managed resource code
> frees it. Fix this by simply make the driver device-managed as well.
> 
> In short, the sequence leading to the UAF is as follows:
> 
> INIT:
> Code allocates a struct device as a kunit-managed resource.
> Code allocates a drm driver as a kunit-managed resource.
> Code allocates a drm device as a device-managed resource.
> 
> EXIT:
> Kunit resource cleanup frees the drm driver
> Kunit resource cleanup puts the struct device, which starts a
>       device-managed resource cleanup
> device-managed cleanup calls drm_dev_put()
> drm_dev_put() dereferences the (now freed) drm driver -> Boom.
> 
> Related KASAN message:
> [55272.551542] ==================================================================
> [55272.551551] BUG: KASAN: slab-use-after-free in drm_dev_put.part.0+0xd4/0xe0 [drm]
> [55272.551603] Read of size 8 at addr ffff888127502828 by task kunit_try_catch/10353
> 
> [55272.551612] CPU: 4 PID: 10353 Comm: kunit_try_catch Tainted: G     U           N 6.5.0-rc7+ #155
> [55272.551620] Hardware name: ASUS System Product Name/PRIME B560M-A AC, BIOS 0403 01/26/2021
> [55272.551626] Call Trace:
> [55272.551629]  <TASK>
> [55272.551633]  dump_stack_lvl+0x57/0x90
> [55272.551639]  print_report+0xcf/0x630
> [55272.551645]  ? _raw_spin_lock_irqsave+0x5f/0x70
> [55272.551652]  ? drm_dev_put.part.0+0xd4/0xe0 [drm]
> [55272.551694]  kasan_report+0xd7/0x110
> [55272.551699]  ? drm_dev_put.part.0+0xd4/0xe0 [drm]
> [55272.551742]  drm_dev_put.part.0+0xd4/0xe0 [drm]
> [55272.551783]  devres_release_all+0x15d/0x1f0
> [55272.551790]  ? __pfx_devres_release_all+0x10/0x10
> [55272.551797]  device_unbind_cleanup+0x16/0x1a0
> [55272.551802]  device_release_driver_internal+0x3e5/0x540
> [55272.551808]  ? kobject_put+0x5d/0x4b0
> [55272.551814]  bus_remove_device+0x1f1/0x3f0
> [55272.551819]  device_del+0x342/0x910
> [55272.551826]  ? __pfx_device_del+0x10/0x10
> [55272.551830]  ? lock_release+0x339/0x5e0
> [55272.551836]  ? kunit_remove_resource+0x128/0x290 [kunit]
> [55272.551845]  ? __pfx_lock_release+0x10/0x10
> [55272.551851]  platform_device_del.part.0+0x1f/0x1e0
> [55272.551856]  ? _raw_spin_unlock_irqrestore+0x30/0x60
> [55272.551863]  kunit_remove_resource+0x195/0x290 [kunit]
> [55272.551871]  ? _raw_spin_unlock_irqrestore+0x30/0x60
> [55272.551877]  kunit_cleanup+0x78/0x120 [kunit]
> [55272.551885]  ? __kthread_parkme+0xc1/0x1f0
> [55272.551891]  ? __pfx_kunit_try_run_case_cleanup+0x10/0x10 [kunit]
> [55272.551900]  ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10 [kunit]
> [55272.551909]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
> [55272.551919]  kthread+0x2e7/0x3c0
> [55272.551924]  ? __pfx_kthread+0x10/0x10
> [55272.551929]  ret_from_fork+0x2d/0x70
> [55272.551935]  ? __pfx_kthread+0x10/0x10
> [55272.551940]  ret_from_fork_asm+0x1b/0x30
> [55272.551948]  </TASK>
> 
> [55272.551953] Allocated by task 10351:
> [55272.551956]  kasan_save_stack+0x1c/0x40
> [55272.551962]  kasan_set_track+0x21/0x30
> [55272.551966]  __kasan_kmalloc+0x8b/0x90
> [55272.551970]  __kmalloc+0x5e/0x160
> [55272.551976]  kunit_kmalloc_array+0x1c/0x50 [kunit]
> [55272.551984]  drm_exec_test_init+0xfa/0x2c0 [drm_exec_test]
> [55272.551991]  kunit_try_run_case+0xdd/0x250 [kunit]
> [55272.551999]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
> [55272.552008]  kthread+0x2e7/0x3c0
> [55272.552012]  ret_from_fork+0x2d/0x70
> [55272.552017]  ret_from_fork_asm+0x1b/0x30
> 
> [55272.552024] Freed by task 10353:
> [55272.552027]  kasan_save_stack+0x1c/0x40
> [55272.552032]  kasan_set_track+0x21/0x30
> [55272.552036]  kasan_save_free_info+0x27/0x40
> [55272.552041]  __kasan_slab_free+0x106/0x180
> [55272.552046]  slab_free_freelist_hook+0xb3/0x160
> [55272.552051]  __kmem_cache_free+0xb2/0x290
> [55272.552056]  kunit_remove_resource+0x195/0x290 [kunit]
> [55272.552064]  kunit_cleanup+0x78/0x120 [kunit]
> [55272.552072]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
> [55272.552080]  kthread+0x2e7/0x3c0
> [55272.552085]  ret_from_fork+0x2d/0x70
> [55272.552089]  ret_from_fork_asm+0x1b/0x30
> 
> [55272.552096] The buggy address belongs to the object at ffff888127502800
>                 which belongs to the cache kmalloc-512 of size 512
> [55272.552105] The buggy address is located 40 bytes inside of
>                 freed 512-byte region [ffff888127502800, ffff888127502a00)
> 
> [55272.552115] The buggy address belongs to the physical page:
> [55272.552119] page:00000000af6c70ff refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x127500
> [55272.552127] head:00000000af6c70ff order:3 entire_mapcount:0 nr_pages_mapped:0 pincount:0
> [55272.552133] anon flags: 0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
> [55272.552141] page_type: 0xffffffff()
> [55272.552145] raw: 0017ffffc0010200 ffff888100042c80 0000000000000000 dead000000000001
> [55272.552152] raw: 0000000000000000 0000000080200020 00000001ffffffff 0000000000000000
> [55272.552157] page dumped because: kasan: bad access detected
> 
> [55272.552163] Memory state around the buggy address:
> [55272.552167]  ffff888127502700: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [55272.552173]  ffff888127502780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [55272.552178] >ffff888127502800: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [55272.552184]                                   ^
> [55272.552187]  ffff888127502880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [55272.552193]  ffff888127502900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> [55272.552198] ==================================================================
> [55272.552203] Disabling lock debugging due to kernel taint
> 
> v2:
> - Update commit message, add Fixes: tag and Cc stable.
> v3:
> - Further commit message updates (Maxime Ripard).
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v6.3+
> Fixes: d98780310719 ("drm/tests: helpers: Allow to pass a custom drm_driver")
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

Reviewed-by: Francois Dugast <francois.dugast@intel.com>

> ---
>  include/drm/drm_kunit_helpers.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_kunit_helpers.h b/include/drm/drm_kunit_helpers.h
> index 514c8a7a32f0..ba483c87f0e7 100644
> --- a/include/drm/drm_kunit_helpers.h
> +++ b/include/drm/drm_kunit_helpers.h
> @@ -3,6 +3,8 @@
>  #ifndef DRM_KUNIT_HELPERS_H_
>  #define DRM_KUNIT_HELPERS_H_
>  
> +#include <linux/device.h>
> +
>  #include <kunit/test.h>
>  
>  struct drm_device;
> @@ -51,7 +53,7 @@ __drm_kunit_helper_alloc_drm_device(struct kunit *test,
>  {
>  	struct drm_driver *driver;
>  
> -	driver = kunit_kzalloc(test, sizeof(*driver), GFP_KERNEL);
> +	driver = devm_kzalloc(dev, sizeof(*driver), GFP_KERNEL);
>  	KUNIT_ASSERT_NOT_NULL(test, driver);
>  
>  	driver->driver_features = features;
> -- 
> 2.41.0
> 

Re: [Intel-xe] [PATCH v3 1/2] drm/tests: helpers: Avoid a driver uaf

[Thomas Hellström]

On 9/11/23 14:40, Francois Dugast wrote:
> On Thu, Sep 07, 2023 at 03:53:38PM +0200, Thomas Hellström wrote:
>> when using __drm_kunit_helper_alloc_drm_device() the driver may be
>> dereferenced by device-managed resources up until the device is
>> freed, which is typically later than the kunit-managed resource code
>> frees it. Fix this by simply make the driver device-managed as well.
>>
>> In short, the sequence leading to the UAF is as follows:
>>
>> INIT:
>> Code allocates a struct device as a kunit-managed resource.
>> Code allocates a drm driver as a kunit-managed resource.
>> Code allocates a drm device as a device-managed resource.
>>
>> EXIT:
>> Kunit resource cleanup frees the drm driver
>> Kunit resource cleanup puts the struct device, which starts a
>>        device-managed resource cleanup
>> device-managed cleanup calls drm_dev_put()
>> drm_dev_put() dereferences the (now freed) drm driver -> Boom.
>>
>> Related KASAN message:
>> [55272.551542] ==================================================================
>> [55272.551551] BUG: KASAN: slab-use-after-free in drm_dev_put.part.0+0xd4/0xe0 [drm]
>> [55272.551603] Read of size 8 at addr ffff888127502828 by task kunit_try_catch/10353
>>
>> [55272.551612] CPU: 4 PID: 10353 Comm: kunit_try_catch Tainted: G     U           N 6.5.0-rc7+ #155
>> [55272.551620] Hardware name: ASUS System Product Name/PRIME B560M-A AC, BIOS 0403 01/26/2021
>> [55272.551626] Call Trace:
>> [55272.551629]  <TASK>
>> [55272.551633]  dump_stack_lvl+0x57/0x90
>> [55272.551639]  print_report+0xcf/0x630
>> [55272.551645]  ? _raw_spin_lock_irqsave+0x5f/0x70
>> [55272.551652]  ? drm_dev_put.part.0+0xd4/0xe0 [drm]
>> [55272.551694]  kasan_report+0xd7/0x110
>> [55272.551699]  ? drm_dev_put.part.0+0xd4/0xe0 [drm]
>> [55272.551742]  drm_dev_put.part.0+0xd4/0xe0 [drm]
>> [55272.551783]  devres_release_all+0x15d/0x1f0
>> [55272.551790]  ? __pfx_devres_release_all+0x10/0x10
>> [55272.551797]  device_unbind_cleanup+0x16/0x1a0
>> [55272.551802]  device_release_driver_internal+0x3e5/0x540
>> [55272.551808]  ? kobject_put+0x5d/0x4b0
>> [55272.551814]  bus_remove_device+0x1f1/0x3f0
>> [55272.551819]  device_del+0x342/0x910
>> [55272.551826]  ? __pfx_device_del+0x10/0x10
>> [55272.551830]  ? lock_release+0x339/0x5e0
>> [55272.551836]  ? kunit_remove_resource+0x128/0x290 [kunit]
>> [55272.551845]  ? __pfx_lock_release+0x10/0x10
>> [55272.551851]  platform_device_del.part.0+0x1f/0x1e0
>> [55272.551856]  ? _raw_spin_unlock_irqrestore+0x30/0x60
>> [55272.551863]  kunit_remove_resource+0x195/0x290 [kunit]
>> [55272.551871]  ? _raw_spin_unlock_irqrestore+0x30/0x60
>> [55272.551877]  kunit_cleanup+0x78/0x120 [kunit]
>> [55272.551885]  ? __kthread_parkme+0xc1/0x1f0
>> [55272.551891]  ? __pfx_kunit_try_run_case_cleanup+0x10/0x10 [kunit]
>> [55272.551900]  ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10 [kunit]
>> [55272.551909]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
>> [55272.551919]  kthread+0x2e7/0x3c0
>> [55272.551924]  ? __pfx_kthread+0x10/0x10
>> [55272.551929]  ret_from_fork+0x2d/0x70
>> [55272.551935]  ? __pfx_kthread+0x10/0x10
>> [55272.551940]  ret_from_fork_asm+0x1b/0x30
>> [55272.551948]  </TASK>
>>
>> [55272.551953] Allocated by task 10351:
>> [55272.551956]  kasan_save_stack+0x1c/0x40
>> [55272.551962]  kasan_set_track+0x21/0x30
>> [55272.551966]  __kasan_kmalloc+0x8b/0x90
>> [55272.551970]  __kmalloc+0x5e/0x160
>> [55272.551976]  kunit_kmalloc_array+0x1c/0x50 [kunit]
>> [55272.551984]  drm_exec_test_init+0xfa/0x2c0 [drm_exec_test]
>> [55272.551991]  kunit_try_run_case+0xdd/0x250 [kunit]
>> [55272.551999]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
>> [55272.552008]  kthread+0x2e7/0x3c0
>> [55272.552012]  ret_from_fork+0x2d/0x70
>> [55272.552017]  ret_from_fork_asm+0x1b/0x30
>>
>> [55272.552024] Freed by task 10353:
>> [55272.552027]  kasan_save_stack+0x1c/0x40
>> [55272.552032]  kasan_set_track+0x21/0x30
>> [55272.552036]  kasan_save_free_info+0x27/0x40
>> [55272.552041]  __kasan_slab_free+0x106/0x180
>> [55272.552046]  slab_free_freelist_hook+0xb3/0x160
>> [55272.552051]  __kmem_cache_free+0xb2/0x290
>> [55272.552056]  kunit_remove_resource+0x195/0x290 [kunit]
>> [55272.552064]  kunit_cleanup+0x78/0x120 [kunit]
>> [55272.552072]  kunit_generic_run_threadfn_adapter+0x4a/0x90 [kunit]
>> [55272.552080]  kthread+0x2e7/0x3c0
>> [55272.552085]  ret_from_fork+0x2d/0x70
>> [55272.552089]  ret_from_fork_asm+0x1b/0x30
>>
>> [55272.552096] The buggy address belongs to the object at ffff888127502800
>>                  which belongs to the cache kmalloc-512 of size 512
>> [55272.552105] The buggy address is located 40 bytes inside of
>>                  freed 512-byte region [ffff888127502800, ffff888127502a00)
>>
>> [55272.552115] The buggy address belongs to the physical page:
>> [55272.552119] page:00000000af6c70ff refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x127500
>> [55272.552127] head:00000000af6c70ff order:3 entire_mapcount:0 nr_pages_mapped:0 pincount:0
>> [55272.552133] anon flags: 0x17ffffc0010200(slab|head|node=0|zone=2|lastcpupid=0x1fffff)
>> [55272.552141] page_type: 0xffffffff()
>> [55272.552145] raw: 0017ffffc0010200 ffff888100042c80 0000000000000000 dead000000000001
>> [55272.552152] raw: 0000000000000000 0000000080200020 00000001ffffffff 0000000000000000
>> [55272.552157] page dumped because: kasan: bad access detected
>>
>> [55272.552163] Memory state around the buggy address:
>> [55272.552167]  ffff888127502700: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>> [55272.552173]  ffff888127502780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>> [55272.552178] >ffff888127502800: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>> [55272.552184]                                   ^
>> [55272.552187]  ffff888127502880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>> [55272.552193]  ffff888127502900: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>> [55272.552198] ==================================================================
>> [55272.552203] Disabling lock debugging due to kernel taint
>>
>> v2:
>> - Update commit message, add Fixes: tag and Cc stable.
>> v3:
>> - Further commit message updates (Maxime Ripard).
>>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: <stable@vger.kernel.org> # v6.3+
>> Fixes: d98780310719 ("drm/tests: helpers: Allow to pass a custom drm_driver")
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Reviewed-by: Francois Dugast <francois.dugast@intel.com>

Thanks for the R-B, Francois.

/Thomas

Re: (subset) [PATCH v3 1/2] drm/tests: helpers: Avoid a driver uaf

[Maxime Ripard]

On Thu, 07 Sep 2023 15:53:38 +0200, Thomas Hellström wrote:
> when using __drm_kunit_helper_alloc_drm_device() the driver may be
> dereferenced by device-managed resources up until the device is
> freed, which is typically later than the kunit-managed resource code
> frees it. Fix this by simply make the driver device-managed as well.
> 
> In short, the sequence leading to the UAF is as follows:
> 
> [...]

Applied to drm/drm-misc (drm-misc-fixes).

Thanks!
Maxime