[PATCH 0/3] Fix a couble of bugs in drm_kunit_helpers.c

[PATCH 0/3] Fix a couble of bugs in drm_kunit_helpers.c

[Arthur Grillo]

This patchset started when I found a use-after-free error reported by
KASAN while running some tests that did some mocking. When trying to fix
the initial problem, I found another noon-related one.

The second bug is just a wrong argument passed to a kunit_release_action
call. Patch #1 solves that.

Patches #2 and #3 solve the use-after-free bug. This error was a bit
trickier to find. Basically, the usual order in which the kunit_actions
run is the culprit, so #2 creates a helper function to reorder actions,
and #3 uses that helper.

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
---
Arthur Grillo (3):
      drm/tests: Fix kunit_release_action ctx argument
      kunit: Add kunit_move_action_to_top_or_reset() to reorder actions
      drm/tests: Fix a use-after-free bug in __drm_kunit_helper_alloc_drm_device()

 drivers/gpu/drm/tests/drm_kunit_helpers.c | 18 +++++++++++++++++-
 include/kunit/resource.h                  | 17 +++++++++++++++++
 lib/kunit/resource.c                      | 19 +++++++++++++++++++
 3 files changed, 53 insertions(+), 1 deletion(-)
---
base-commit: 37454bcbb68601c326b58ac45f508067047d791f
change-id: 20230918-kunit-kasan-fixes-88ee78002078

Best regards,
-- 
Arthur Grillo <arthurgrillo@riseup.net>

[PATCH 1/3] drm/tests: Fix kunit_release_action ctx argument

[Arthur Grillo]

The kunit_action_platform_driver_unregister is added with
&fake_platform_driver as ctx, but the kunit_release_action is called
pdev as ctx. Fix that by replacing it with &fake_platform_driver.

Fixes: 4f2b0b583baa ("drm/tests: helpers: Switch to kunit actions")
Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
---
 drivers/gpu/drm/tests/drm_kunit_helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
index 3d624ff2f651..3150dbc647ee 100644
--- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
+++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
@@ -118,7 +118,7 @@ void drm_kunit_helper_free_device(struct kunit *test, struct device *dev)
 
 	kunit_release_action(test,
 			     kunit_action_platform_driver_unregister,
-			     pdev);
+			     &fake_platform_driver);
 }
 EXPORT_SYMBOL_GPL(drm_kunit_helper_free_device);
 

-- 
2.41.0

Re: [PATCH 1/3] drm/tests: Fix kunit_release_action ctx argument

[Maxime Ripard]

On Wed, 20 Sep 2023 03:11:36 -0300, Arthur Grillo wrote:
> The kunit_action_platform_driver_unregister is added with
> &fake_platform_driver as ctx, but the kunit_release_action is called
> pdev as ctx. Fix that by replacing it with &fake_platform_driver.
> 
> Fixes: 4f2b0b583baa ("drm/tests: helpers: Switch to kunit actions")
> 
> [ ... ]

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

Thanks!
Maxime

Re: [PATCH 1/3] drm/tests: Fix kunit_release_action ctx argument

[Maira Canal]

Hi Arthur,

On 9/20/23 03:11, Arthur Grillo wrote:
> The kunit_action_platform_driver_unregister is added with
> &fake_platform_driver as ctx, but the kunit_release_action is called
> pdev as ctx. Fix that by replacing it with &fake_platform_driver.
> 
> Fixes: 4f2b0b583baa ("drm/tests: helpers: Switch to kunit actions")
> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>

Reviewed-by: Maíra Canal <mairacanal@riseup.net>

Do you need me to apply this patch to drm-misc-fixes?

Best Regards,
- Maíra

> ---
>   drivers/gpu/drm/tests/drm_kunit_helpers.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> index 3d624ff2f651..3150dbc647ee 100644
> --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> @@ -118,7 +118,7 @@ void drm_kunit_helper_free_device(struct kunit *test, struct device *dev)
>   
>   	kunit_release_action(test,
>   			     kunit_action_platform_driver_unregister,
> -			     pdev);
> +			     &fake_platform_driver);
>   }
>   EXPORT_SYMBOL_GPL(drm_kunit_helper_free_device);
>   
> 

Re: [PATCH 1/3] drm/tests: Fix kunit_release_action ctx argument

[Arthur Grillo]

On 27/09/23 19:47, Maira Canal wrote:
> Hi Arthur,
> 
> On 9/20/23 03:11, Arthur Grillo wrote:
>> The kunit_action_platform_driver_unregister is added with
>> &fake_platform_driver as ctx, but the kunit_release_action is called
>> pdev as ctx. Fix that by replacing it with &fake_platform_driver.
>>
>> Fixes: 4f2b0b583baa ("drm/tests: helpers: Switch to kunit actions")
>> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> 
> Reviewed-by: Maíra Canal <mairacanal@riseup.net>
> 
> Do you need me to apply this patch to drm-misc-fixes?

Yes, please do, if possible.

Thanks,
~Arthur Grillo

> 
> Best Regards,
> - Maíra
> 
>> ---
>>   drivers/gpu/drm/tests/drm_kunit_helpers.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
>> index 3d624ff2f651..3150dbc647ee 100644
>> --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
>> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
>> @@ -118,7 +118,7 @@ void drm_kunit_helper_free_device(struct kunit *test, struct device *dev)
>>         kunit_release_action(test,
>>                    kunit_action_platform_driver_unregister,
>> -                 pdev);
>> +                 &fake_platform_driver);
>>   }
>>   EXPORT_SYMBOL_GPL(drm_kunit_helper_free_device);
>>  

Re: [PATCH 1/3] drm/tests: Fix kunit_release_action ctx argument

[Maira Canal]

Hi Arthur,

On 9/27/23 19:52, Arthur Grillo wrote:
> 
> 
> On 27/09/23 19:47, Maira Canal wrote:
>> Hi Arthur,
>>
>> On 9/20/23 03:11, Arthur Grillo wrote:
>>> The kunit_action_platform_driver_unregister is added with
>>> &fake_platform_driver as ctx, but the kunit_release_action is called
>>> pdev as ctx. Fix that by replacing it with &fake_platform_driver.
>>>
>>> Fixes: 4f2b0b583baa ("drm/tests: helpers: Switch to kunit actions")
>>> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
>>
>> Reviewed-by: Maíra Canal <mairacanal@riseup.net>
>>
>> Do you need me to apply this patch to drm-misc-fixes?
> 
> Yes, please do, if possible.

Applied to drm-misc/drm-misc-fixes!

Thanks,
- Maíra

> 
> Thanks,
> ~Arthur Grillo
> 
>>
>> Best Regards,
>> - Maíra
>>
>>> ---
>>>    drivers/gpu/drm/tests/drm_kunit_helpers.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
>>> index 3d624ff2f651..3150dbc647ee 100644
>>> --- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
>>> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
>>> @@ -118,7 +118,7 @@ void drm_kunit_helper_free_device(struct kunit *test, struct device *dev)
>>>          kunit_release_action(test,
>>>                     kunit_action_platform_driver_unregister,
>>> -                 pdev);
>>> +                 &fake_platform_driver);
>>>    }
>>>    EXPORT_SYMBOL_GPL(drm_kunit_helper_free_device);
>>>   

[PATCH 2/3] kunit: Add kunit_move_action_to_top_or_reset() to reorder actions

[Arthur Grillo]

On Kunit, if we allocate a resource A and B on this order, with its
deferred actions to free them. The resource stack would be something
like this:

	 +---------+
	 | free(B) |
	 +---------+
	 |   ...   |
	 +---------+
	 | free(A) |
	 +---------+

If the deferred action of A accesses B, this would cause a
use-after-free bug. To solve that, we need a way to change the order
of actions.

Create a function to move an action to the top of the resource stack,
as shown in the diagram below.

	 +---------+    +---------+
	 | free(B) |	| free(A) |
	 +---------+    +---------+
	 |   ...   | -> | free(B) |
	 +---------+ 	+---------+
	 | free(A) |	|   ...   |
	 +---------+	+---------+

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
---
 include/kunit/resource.h | 17 +++++++++++++++++
 lib/kunit/resource.c     | 19 +++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/include/kunit/resource.h b/include/kunit/resource.h
index c7383e90f5c9..c598b23680e3 100644
--- a/include/kunit/resource.h
+++ b/include/kunit/resource.h
@@ -479,4 +479,21 @@ void kunit_remove_action(struct kunit *test,
 void kunit_release_action(struct kunit *test,
 			  kunit_action_t *action,
 			  void *ctx);
+
+/**
+ * kunit_move_action_to_top_or_reset - Move a previously added action to the top
+ *				       of the order of actions calls.
+ * @test: Test case to associate the action with.
+ * @action: The function to run on test exit
+ * @ctx: Data passed into @func
+ *
+ * Reorder the action stack, by moving the desired action to the top.
+ *
+ * Returns:
+ *   0 on success, an error if the action could not be inserted on the top.
+ */
+int kunit_move_action_to_top_or_reset(struct kunit *test,
+				      kunit_action_t *action,
+				      void *ctx);
+
 #endif /* _KUNIT_RESOURCE_H */
diff --git a/lib/kunit/resource.c b/lib/kunit/resource.c
index f0209252b179..fe40a34b62a6 100644
--- a/lib/kunit/resource.c
+++ b/lib/kunit/resource.c
@@ -176,3 +176,22 @@ void kunit_release_action(struct kunit *test,
 	}
 }
 EXPORT_SYMBOL_GPL(kunit_release_action);
+
+int kunit_move_action_to_top_or_reset(struct kunit *test,
+				      kunit_action_t *action,
+				      void *ctx)
+{
+	struct kunit_action_ctx match_ctx;
+	struct kunit_resource *res;
+
+	match_ctx.func = action;
+	match_ctx.ctx = ctx;
+	res = kunit_find_resource(test, __kunit_action_match, &match_ctx);
+	if (res) {
+		kunit_remove_action(test, action, ctx);
+		return kunit_add_action_or_reset(test, action, ctx);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(kunit_move_action_to_top_or_reset);

-- 
2.41.0

Re: [PATCH 2/3] kunit: Add kunit_move_action_to_top_or_reset() to reorder actions

[David Gow]

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

On Wed, 20 Sept 2023 at 14:12, Arthur Grillo <arthurgrillo@riseup.net> wrote:
>
> On Kunit, if we allocate a resource A and B on this order, with its
> deferred actions to free them. The resource stack would be something
> like this:
>
>          +---------+
>          | free(B) |
>          +---------+
>          |   ...   |
>          +---------+
>          | free(A) |
>          +---------+
>
> If the deferred action of A accesses B, this would cause a
> use-after-free bug. To solve that, we need a way to change the order
> of actions.
>
> Create a function to move an action to the top of the resource stack,
> as shown in the diagram below.
>
>          +---------+    +---------+
>          | free(B) |    | free(A) |
>          +---------+    +---------+
>          |   ...   | -> | free(B) |
>          +---------+    +---------+
>          | free(A) |    |   ...   |
>          +---------+    +---------+
>
> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> ---

Thanks. This is a really interesting patch: my hope was that something
like this wouldn't be necessary, as in most cases freeing things in
the reverse order to which they were created is the right thing to do.

It looks like, from the comments on patch 3, this may no longer be
necessary? Is that so?

Otherwise, if you have a real use case for it, I've no objection to
KUnit adding this as a feature (though I'd probably add some
documentation suggesting that it's best avoided if you can order your
allocations / calls better).

Cheers,
-- David

>  include/kunit/resource.h | 17 +++++++++++++++++
>  lib/kunit/resource.c     | 19 +++++++++++++++++++
>  2 files changed, 36 insertions(+)
>
> diff --git a/include/kunit/resource.h b/include/kunit/resource.h
> index c7383e90f5c9..c598b23680e3 100644
> --- a/include/kunit/resource.h
> +++ b/include/kunit/resource.h
> @@ -479,4 +479,21 @@ void kunit_remove_action(struct kunit *test,
>  void kunit_release_action(struct kunit *test,
>                           kunit_action_t *action,
>                           void *ctx);
> +
> +/**
> + * kunit_move_action_to_top_or_reset - Move a previously added action to the top
> + *                                    of the order of actions calls.
> + * @test: Test case to associate the action with.
> + * @action: The function to run on test exit
> + * @ctx: Data passed into @func
> + *
> + * Reorder the action stack, by moving the desired action to the top.
> + *
> + * Returns:
> + *   0 on success, an error if the action could not be inserted on the top.
> + */
> +int kunit_move_action_to_top_or_reset(struct kunit *test,
> +                                     kunit_action_t *action,
> +                                     void *ctx);
> +
>  #endif /* _KUNIT_RESOURCE_H */
> diff --git a/lib/kunit/resource.c b/lib/kunit/resource.c
> index f0209252b179..fe40a34b62a6 100644
> --- a/lib/kunit/resource.c
> +++ b/lib/kunit/resource.c
> @@ -176,3 +176,22 @@ void kunit_release_action(struct kunit *test,
>         }
>  }
>  EXPORT_SYMBOL_GPL(kunit_release_action);
> +
> +int kunit_move_action_to_top_or_reset(struct kunit *test,
> +                                     kunit_action_t *action,
> +                                     void *ctx)
> +{
> +       struct kunit_action_ctx match_ctx;
> +       struct kunit_resource *res;
> +
> +       match_ctx.func = action;
> +       match_ctx.ctx = ctx;
> +       res = kunit_find_resource(test, __kunit_action_match, &match_ctx);
> +       if (res) {
> +               kunit_remove_action(test, action, ctx);
> +               return kunit_add_action_or_reset(test, action, ctx);
> +       }
> +

if (!res), this doesn't call the action, so the _or_reset() part of
this doesn't quite make sense.

As I understand it, there are three cases handled here:
1. The action already existed, and we were able to recreate it at the top.
2. The action already existed, but we were unable to recreate it.
3. The action did not previously exist.

In this case, for (1), the action is successfully moved to the top.
This is the "good case".
For (2), we run the action immediately (the idea being that it's
better to not leak memory).
For (3), we do nothing, the action is never run.

My guess, from the name ending in _or_reset, (3) should:
- Try to defer the action. If deferring it fails, run the action immediately.

Or possibly, always run the action immediately in case (3).

Whatever we want, we need to decide on what happens here and document them.

And of course, we can get some of those behaviours without needing to
call kunit_find_resource() at all, just by calling
kunit_remove_action(...)
kunit_add_action_or_reset()
unconditionally.

> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(kunit_move_action_to_top_or_reset);
>
> --
> 2.41.0
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH 2/3] kunit: Add kunit_move_action_to_top_or_reset() to reorder actions

[Maxime Ripard]

Hi David,

On Fri, Sep 22, 2023 at 04:00:21PM +0800, David Gow wrote:
> On Wed, 20 Sept 2023 at 14:12, Arthur Grillo <arthurgrillo@riseup.net> wrote:
> >
> > On Kunit, if we allocate a resource A and B on this order, with its
> > deferred actions to free them. The resource stack would be something
> > like this:
> >
> >          +---------+
> >          | free(B) |
> >          +---------+
> >          |   ...   |
> >          +---------+
> >          | free(A) |
> >          +---------+
> >
> > If the deferred action of A accesses B, this would cause a
> > use-after-free bug. To solve that, we need a way to change the order
> > of actions.
> >
> > Create a function to move an action to the top of the resource stack,
> > as shown in the diagram below.
> >
> >          +---------+    +---------+
> >          | free(B) |    | free(A) |
> >          +---------+    +---------+
> >          |   ...   | -> | free(B) |
> >          +---------+    +---------+
> >          | free(A) |    |   ...   |
> >          +---------+    +---------+
> >
> > Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> > ---
> 
> Thanks. This is a really interesting patch: my hope was that something
> like this wouldn't be necessary, as in most cases freeing things in
> the reverse order to which they were created is the right thing to do.
> 
> It looks like, from the comments on patch 3, this may no longer be
> necessary? Is that so?

Yeah, it's no longer necessary

Maxime

[PATCH 3/3] drm/tests: Fix a use-after-free bug in __drm_kunit_helper_alloc_drm_device()

[Arthur Grillo]

In __drm_kunit_helper_alloc_drm_device_with_driver(), a drm_driver is
allocated with kunit_kzalloc. If the dev argument was allocated by
drm_kunit_helper_alloc_device, its deferred actions would access the
already deallocated drm_driver.

To fix that, move the deferred to the top of the resource stack.

==================================================================
BUG: KASAN: slab-use-after-free in devm_drm_dev_init_release+0x54/0xb0
Read of size 8 at addr 0000000063194e28 by task kunit_try_catch/127

CPU: 0 PID: 127 Comm: kunit_try_catch Tainted: G        W        N 6.5.0-rc2-00620-g4f77e58c6017 #35
Stack:
 606c9a22 606c9a22 00000000 606c9a22
 6056b8fe 63033b10 00000000 60040850
 63033a80 60574ca4 60574c4a 63033b10
Call Trace:
 [<600295a2>] show_stack+0x202/0x220
 [<6056b8fe>] ? _printk+0x0/0x78
 [<60040850>] ? um_set_signals+0x0/0x40
 [<60574ca4>] dump_stack_lvl+0x5a/0x6b
 [<60574c4a>] ? dump_stack_lvl+0x0/0x6b
 [<6019d40d>] print_report+0x1bd/0x670
 [<6056b96b>] ? _printk+0x6d/0x78
 [<6056b8fe>] ? _printk+0x0/0x78
 [<6019f0e6>] ? kasan_complete_mode_report_info+0x116/0x180
 [<603d16d4>] ? devm_drm_dev_init_release+0x54/0xb0
 [<6019db14>] kasan_report+0x184/0x1b0
 [<603d16d4>] ? devm_drm_dev_init_release+0x54/0xb0
 [<6019e770>] ? __asan_load8+0x0/0x80
 [<6019e770>] ? __asan_load8+0x0/0x80
 [<6019e7ec>] __asan_load8+0x7c/0x80
 [<603d16d4>] devm_drm_dev_init_release+0x54/0xb0
 [<6019e770>] ? __asan_load8+0x0/0x80
 [<603d1680>] ? devm_drm_dev_init_release+0x0/0xb0
 [<604b0bde>] devm_action_release+0x2e/0x40
 [<604afd60>] devres_release_all+0x100/0x170
 [<6019e770>] ? __asan_load8+0x0/0x80
 [<6019e770>] ? __asan_load8+0x0/0x80
 [<604a7f7d>] device_release_driver_internal+0x39d/0x510
 [<6027d7f0>] ? sysfs_remove_link+0x0/0x50
 [<6019e770>] ? __asan_load8+0x0/0x80
 [<604a8104>] device_release_driver+0x14/0x20
 [<604a3592>] bus_remove_device+0x1e2/0x200
 [<6019e770>] ? __asan_load8+0x0/0x80
 [<6049de5a>] device_del+0x3ba/0x850
 [<6019f790>] ? kasan_quarantine_put+0x0/0x170
 [<60137bad>] ? kfree+0x5d/0x80
 [<6019e7f0>] ? __asan_store8+0x0/0x90
 [<6019e770>] ? __asan_load8+0x0/0x80
 [<604475a0>] ? kunit_action_platform_device_del+0x0/0x20
 [<604ad4b0>] platform_device_del+0x40/0x140
 [<601957f3>] ? __kmem_cache_free+0xc3/0x230
 [<6019e7f0>] ? __asan_store8+0x0/0x90
 [<6019e770>] ? __asan_load8+0x0/0x80
 [<604475a0>] ? kunit_action_platform_device_del+0x0/0x20
 [<604475b0>] kunit_action_platform_device_del+0x10/0x20
 [<6031bf80>] __kunit_action_free+0x30/0x40
 [<6031bf50>] ? __kunit_action_free+0x0/0x40
 [<6031bae4>] kunit_remove_resource+0xf4/0x150
 [<6019e770>] ? __asan_load8+0x0/0x80
 [<6019e770>] ? __asan_load8+0x0/0x80
 [<60040850>] ? um_set_signals+0x0/0x40
 [<6019e770>] ? __asan_load8+0x0/0x80
 [<6031b396>] kunit_cleanup+0xb6/0x140
 [<605848cd>] ? __schedule+0x6bd/0x7a0
 [<6019e7f0>] ? __asan_store8+0x0/0x90
 [<6019e770>] ? __asan_load8+0x0/0x80
 [<6031b715>] kunit_try_run_case_cleanup+0x95/0xb0
 [<6019e770>] ? __asan_load8+0x0/0x80
 [<6019e770>] ? __asan_load8+0x0/0x80
 [<6031b680>] ? kunit_try_run_case_cleanup+0x0/0xb0
 [<6031e240>] kunit_generic_run_threadfn_adapter+0x30/0x60
 [<6008d15e>] kthread+0x28e/0x2e0
 [<6031e210>] ? kunit_generic_run_threadfn_adapter+0x0/0x60
 [<6019e7f0>] ? __asan_store8+0x0/0x90
 [<6008ced0>] ? kthread+0x0/0x2e0
 [<6019e770>] ? __asan_load8+0x0/0x80
 [<600270e6>] new_thread_handler+0x136/0x1a0

Allocated by task 126:
 save_stack_trace+0x5b/0x70
 stack_trace_save+0x7a/0xa0
 kasan_set_track+0x6c/0xa0
 kasan_save_alloc_info+0x26/0x30
 __kasan_kmalloc+0x91/0xa0
 __kmalloc+0xb9/0x110
 kunit_kmalloc_array+0x23/0x60
 drm_test_fb_build_fourcc_list+0x8b/0x390
 kunit_try_run_case+0xab/0x140
 kunit_generic_run_threadfn_adapter+0x30/0x60
 kthread+0x28e/0x2e0
 new_thread_handler+0x136/0x1a0

Freed by task 127:
 save_stack_trace+0x5b/0x70
 stack_trace_save+0x7a/0xa0
 kasan_set_track+0x6c/0xa0
 kasan_save_free_info+0x30/0x50
 ____kasan_slab_free+0x12c/0x190
 __kasan_slab_free+0x18/0x20
 __kmem_cache_free+0xc3/0x230
 kfree+0x5d/0x80
 __kunit_action_free+0x30/0x40
 kunit_remove_resource+0xf4/0x150
 kunit_cleanup+0xb6/0x140
 kunit_try_run_case_cleanup+0x95/0xb0
 kunit_generic_run_threadfn_adapter+0x30/0x60
 kthread+0x28e/0x2e0
 new_thread_handler+0x136/0x1a0

The buggy address belongs to the object at 0000000063194e00
 which belongs to the cache kmalloc-256 of size 256
The buggy address is located 40 bytes inside of
 freed 256-byte region [0000000063194e00, 0000000063194f00)

The buggy address belongs to the physical page:
page:00000000d99927cc refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x3194
flags: 0x200(slab|zone=0)
page_type: 0xffffffff()
raw: 0000000000000200 0000000062401900 0000000000000122 0000000000000000
raw: 0000000000000000 0000000080080008 00000001ffffffff
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 0000000063194d00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 0000000063194d80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>0000000063194e00: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                                  ^
 0000000063194e80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 0000000063194f00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
---
 drivers/gpu/drm/tests/drm_kunit_helpers.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c b/drivers/gpu/drm/tests/drm_kunit_helpers.c
index 3150dbc647ee..655cedf7ab13 100644
--- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
+++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
@@ -129,6 +129,7 @@ __drm_kunit_helper_alloc_drm_device_with_driver(struct kunit *test,
 						const struct drm_driver *driver)
 {
 	struct drm_device *drm;
+	struct platform_device *pdev = to_platform_device(dev);
 	void *container;
 	int ret;
 
@@ -143,6 +144,21 @@ __drm_kunit_helper_alloc_drm_device_with_driver(struct kunit *test,
 	if (ret)
 		return ERR_PTR(ret);
 
+	ret = kunit_move_action_to_top_or_reset(test,
+						kunit_action_platform_driver_unregister,
+						&fake_platform_driver);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = kunit_move_action_to_top_or_reset(test,
+						kunit_action_platform_device_put,
+						pdev);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	ret = kunit_move_action_to_top_or_reset(test,
+						kunit_action_platform_device_del,
+						pdev);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
 	return drm;
 }
 EXPORT_SYMBOL_GPL(__drm_kunit_helper_alloc_drm_device_with_driver);

-- 
2.41.0

Re: [PATCH 3/3] drm/tests: Fix a use-after-free bug in __drm_kunit_helper_alloc_drm_device()

[Maxime Ripard]

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

Hi,

On Wed, Sep 20, 2023 at 03:11:38AM -0300, Arthur Grillo wrote:
> In __drm_kunit_helper_alloc_drm_device_with_driver(), a drm_driver is
> allocated with kunit_kzalloc. If the dev argument was allocated by
> drm_kunit_helper_alloc_device, its deferred actions would access the
> already deallocated drm_driver.

We already have a fix for that in drm-misc-fixes, could you give it a try?

Thanks!
Maxime

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

Re: [PATCH 3/3] drm/tests: Fix a use-after-free bug in __drm_kunit_helper_alloc_drm_device()

[Arthur Grillo]

On 20/09/23 03:40, Maxime Ripard wrote:
> Hi,
> 
> On Wed, Sep 20, 2023 at 03:11:38AM -0300, Arthur Grillo wrote:
>> In __drm_kunit_helper_alloc_drm_device_with_driver(), a drm_driver is
>> allocated with kunit_kzalloc. If the dev argument was allocated by
>> drm_kunit_helper_alloc_device, its deferred actions would access the
>> already deallocated drm_driver.
> 
> We already have a fix for that in drm-misc-fixes, could you give it a try?

Oh! I didn't see that. I just ran it, it worked! Great fix :)

Best Regards,
~Arthur Grillo

> 
> Thanks!
> Maxime