Re: [PATCH 3/3] hwrng: rng90: add KUnit tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hwrng: rng90: NULL dereference in rng90_test_exit() if rng90_test_init() fails
--

commit 738c911c8957ee3bcb09ed18f6daa72a41daed79
Author: Patrick Tudor <ptudor@ptudor.net>

hwrng: rng90: add KUnit tests

This commit adds built-in KUnit tests for the RNG90 hardware random number
generator driver using a scripted I2C adapter to exercise command framing,
CRC validation, polling, recovery, health failures, and power management.

> diff --git a/drivers/char/hw_random/rng90-kunit.c b/drivers/char/hw_random/rng90-kunit.c
> new file mode 100644
> index 0000000000000..f6daffb2e3d97
> --- /dev/null
> +++ b/drivers/char/hw_random/rng90-kunit.c
> @@ -0,0 +1,820 @@

[ ... ]

> +static int rng90_test_init(struct kunit *test)
> +{
> +	struct rng90_test_context *ctx;
> +	int ret;
> +
> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +	test->priv = ctx;
> +	ctx->test = test;
> +	ctx->adapter.owner = THIS_MODULE;
> +	ctx->adapter.algo = &rng90_test_algorithm;
> +	ctx->adapter.algo_data = ctx;
> +	strscpy(ctx->adapter.name, "rng90-kunit", sizeof(ctx->adapter.name));
> +	ret = i2c_add_adapter(&ctx->adapter);
> +	if (ret)
> +		return ret;
> +	ctx->client = i2c_new_dummy_device(&ctx->adapter, RNG90_ADDRESS);
> +	if (IS_ERR(ctx->client)) {
> +		ret = PTR_ERR(ctx->client);
> +		i2c_del_adapter(&ctx->adapter);
> +		return ret;
> +	}
> +	ctx->priv.client = ctx->client;
> +	mutex_init(&ctx->priv.lock);
> +	i2c_set_clientdata(ctx->client, &ctx->priv);
> +	/* Never register this predictable test source with the hwrng core. */
> +	return 0;
> +}
> +
> +/* Verify the script was consumed, allowing a final busy step at the deadline. */
> +static void rng90_test_settled(struct rng90_test_context *ctx)
> +{
> +	if (ctx->next < ctx->count &&
> +	    ctx->steps[ctx->next].repeat == RNG90_TEST_BUSY &&
> +	    ctx->steps[ctx->next].hits)
> +		ctx->next++;
> +	KUNIT_EXPECT_EQ(ctx->test, ctx->next, ctx->count);
> +}
> +
> +static void rng90_test_exit(struct kunit *test)
> +{
> +	struct rng90_test_context *ctx = test->priv;
> +
> +	rng90_test_settled(ctx);
> +	i2c_unregister_device(ctx->client);
> +	i2c_del_adapter(&ctx->adapter);
> +}

[Severity: Medium]
Can rng90_test_exit() dereference a null pointer or repeat adapter teardown
if rng90_test_init() fails?

In KUnit, kunit_run_case_catch_errors() invokes kunit_try_run_case_cleanup()
-> kunit_run_case_cleanup() -> suite->exit(test) even when suite->init(test)
returns a non-zero error code.

If kunit_kzalloc() fails in rng90_test_init(), test->priv remains NULL, and
rng90_test_exit() will dereference ctx in rng90_test_settled(ctx),
i2c_unregister_device(ctx->client), and i2c_del_adapter(&ctx->adapter).

Additionally, test->priv = ctx is assigned before i2c_add_adapter() and
i2c_new_dummy_device(). If i2c_new_dummy_device() fails, rng90_test_init()
calls i2c_del_adapter(&ctx->adapter) and leaves ctx->client as an error
pointer, after which rng90_test_exit() will run and call
i2c_del_adapter(&ctx->adapter) a second time.

Would it make sense to assign test->priv = ctx only after initialization
succeeds in rng90_test_init(), and check for !ctx in rng90_test_exit() before
cleaning up?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024-rng90-submit-v1-v1-0-e265cfabc9af@ptudor.net?part=3