Re: [PATCH v14 4/5] reset: Add KUnit test cases

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] reset: test: KUnit assertion failures cause UAF and unkillable thread leaks
--

commit 222c19f0c2d0f6b0dadf93fc51cab0de65a40955
Author: Tzung-Bi Shih <tzungbi@kernel.org>

reset: Add KUnit test cases

Since commit 9035073d0ef1 ("reset: convert reset core to using firmware
nodes"), reset suppliers and consumers can be wired up using software
nodes without requiring Device Tree or hardware devices.  Leverage this
to add KUnit test cases for the reset controller framework.

> diff --git a/drivers/reset/core-test.c b/drivers/reset/core-test.c
> new file mode 100644
> index 0000000000000..834556322b236
> --- /dev/null
> +++ b/drivers/reset/core-test.c

[ ... ]

> +static int reset_test_init(struct kunit *test)
> +{

[ ... ]

> +	ctx->rcdev.ops = &reset_test_ops;
> +	ctx->rcdev.nr_resets = 1;
> +	ctx->rcdev.fwnode = ctx->supp_fwnode;
> +
> +	ret = reset_controller_register(&ctx->rcdev);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	ctx->rstc = __fwnode_reset_control_get(ctx->cons_fwnode, NULL, 0,
> +					       RESET_CONTROL_EXCLUSIVE);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->rstc);

[Severity: High]
If __fwnode_reset_control_get() fails and triggers this assertion, the test
will abort immediately without unregistering ctx->rcdev. Since ctx was
allocated with kunit_kzalloc(), it will be automatically freed.

Could this leave a dangling pointer in the global reset_controller_list,
leading to memory corruption?

Would using a deferred cleanup action via kunit_add_action() be appropriate
immediately after registering the controller?

[ ... ]

> +static void reset_test_concurrent_unregister(struct kunit *test)
> +{
> +	struct reset_test_context *rtc = test->priv;
> +	struct test_concurrent_unregister_context *ctx;
> +	int i, ret, val;
> +
> +	rtc->rcdev.ops = &reset_test_concurrent_unregister_ops;
> +
> +	ctx = kunit_kmalloc_array(test, 3, sizeof(*ctx), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, ctx);
> +
> +	for (i = 0; i < 3; ++i) {
> +		ctx[i].test = test;
> +		init_completion(&ctx[i].started);
> +		init_completion(&ctx[i].enter);
> +
> +		if (i == 0) {
> +			ctx[i].thread = kthread_run(
> +				test_concurrent_unregister_supplier, ctx + i,
> +				"supplier%d", i);
> +		} else {
> +			init_completion(&ctx[i].entered);
> +			init_completion(&ctx[i].exit);
> +
> +			ctx[i].thread = kthread_run(
> +				test_concurrent_unregister_consumer, ctx + i,
> +				"consumer%d", i);
> +		}
> +		KUNIT_ASSERT_FALSE(test, IS_ERR(ctx[i].thread));

[Severity: High]
If thread 1 or 2 fails to spawn, this assertion will abort the test
immediately. However, previously spawned threads might still be waiting on
synchronization variables (like ctx[0].enter) inside the test-managed memory.

When KUnit frees ctx on abort, will those still-running threads access
freed memory, resulting in a use-after-free?

> +		get_task_struct(ctx[i].thread);
> +
> +		wait_for_completion(&ctx[i].started);
> +	}
> +
> +	rtc->priv = &ctx[1];
> +	ctx[1].expected_ret = 0x12345678;
> +	/* consumer1 enters reset(). */
> +	KUNIT_EXPECT_EQ(test, rtc->reset_count, 0);
> +	complete(&ctx[1].enter);
> +	wait_for_completion(&ctx[1].entered);
> +	KUNIT_EXPECT_EQ(test, rtc->reset_count, 1);
> +
> +	/* supplier0 unregisters. */
> +	complete(&ctx[0].enter);
> +	ret = read_poll_timeout(reset_control_status, val, val == -ENODEV,
> +				1000, 100000, false, rtc->rstc);
> +	KUNIT_ASSERT_EQ(test, ret, 0);

[Severity: High]
If the poll times out, this assertion aborts the test without stopping the
spawned concurrent kthreads.

Since kthread_stop_put() is skipped, does this result in thread leaks and a
use-after-free when KUnit cleans up the test memory while threads are still
active?

> +	/* supplier0 can't exit.  It's waiting for all consumers. */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011121246.162759-1-tzungbi@kernel.org?part=4