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 <[email protected]>
---
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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Wed, 20 Sept 2023 at 14:12, Arthur Grillo <[email protected]> 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 <[email protected]>
> ---
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
>
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 <[email protected]> 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 <[email protected]>
> > ---
>
> 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