Re: [PATCH v7 3/3] clk: cs2600: Add KUnit test for CS2600 driver

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing length validation for read messages in the dummy I2C transfer function allows a heap buffer overflow.
- [Medium] Integer overflow in delta frequency calculation causes improperly strict test failure bounds.
- [Medium] Device node reference leak on KUnit assertion failure.
--

--- Patch [3]: [PATCH v7 3/3] clk: cs2600: Add KUnit test for CS2600 driver ---
commit 2362317ae5c3adcc9e6eb5a18429400bcf90b30f
Author: Richard Fitzgerald <[email protected]>

    clk: cs2600: Add KUnit test for CS2600 driver

This commit adds a KUnit test for the CS2600 clock driver using a mock I2C
host driver and regmap. It validates the frequency ratio and PLL
configuration register fields against hardcoded tables of expected settings.

> diff --git a/drivers/clk/cirrus/clk-cs2600-kunit.c b/drivers/clk/cirrus/clk-cs2600-kunit.c
> new file mode 100644
> index 0000000000000..2fd3f3ff4ef3f
> --- /dev/null
> +++ b/drivers/clk/cirrus/clk-cs2600-kunit.c
[ ... ]
> +	for (i = 0; cs2600_kunit_bclk_divs[i].pll_out != 0; i++) {
[ ... ]
> +		/*
> +		 * Actual reported frequency can be slightly out because of
> +		 * PLL resolution, but should be within worst-case PPM.
> +		 */
> +		fdelta = (cs2600_kunit_bclk_divs[i].bf_out * CS2600_20_12_PPM) >> 20;

[Severity: Medium]
Could this 32-bit multiplication overflow for large values of bf_out before
the result is stored in the 64-bit fdelta?

[ ... ]
> +	for (i = 0; cs2600_kunit_fsync_divs[i].pll_out != 0; i++) {
[ ... ]
> +		/*
> +		 * Actual reported frequency can be slightly out because of
> +		 * PLL resolution, but should be within worst-case PPM.
> +		 */
> +		fdelta = (cs2600_kunit_fsync_divs[i].bf_out * CS2600_20_12_PPM) >> 20;

[Severity: Medium]
Does this have the same 32-bit overflow vulnerability as the BCLK divider
calculation above?

[ ... ]
> +	/* If there is a second packet it is a read */
> +	if (num == 2) {
> +		ret = cs2600_kunit_i2c_read(priv, addr, &val);
> +		if (ret < 0)
> +			return ret;
> +
> +		bebuf = (__force __be16 *)msgs[1].buf;
> +		bebuf[0] = cpu_to_be16(val);

[Severity: High]
Could this write out of bounds if the read message length is less than 2
bytes, since msgs[1].len is not validated before the write?

> +
> +		return 2;
> +	}
[ ... ]
> +static int cs2600_kunit_case_common_init(struct kunit *test)
> +{
[ ... ]
> +	/* Wait for probes if the test case init already loaded the overlay */
> +	np = of_find_node_by_name(NULL, "cs2600-test-consumer");
> +	if (!IS_ERR_OR_NULL(np))
> +		cs2600_kunit_wait_for_probes(test);
> +	of_node_put(np);

[Severity: Medium]
If an assertion inside cs2600_kunit_wait_for_probes() fails, the KUnit
framework aborts the test thread. Does this result in a leaked device node
reference since the of_node_put() call would be bypassed?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3