Re: [PATCH 3/5] rtc: nct3018y: Make sure clk_init_data is fully initialized

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Unmanaged OF clock provider registration causes a resource leak and potential use-after-free on probe failure or driver unbind.
- [High] Missing restore of NCT3018Y_REG_CTRL state on error paths in nct3018y_rtc_set_time.
- [High] Data race due to missing lock for read-modify-write sequences on NCT3018Y_REG_CTRL.
- [High] Unhandled I2C short block reads lead to uninitialized kernel stack memory being used and potentially leaked to userspace.
--

--- Patch [3]: [PATCH 3/5] rtc: nct3018y: Make sure clk_init_data is fully initialized ---
commit 3afc7c4d645666922e04824eeba8803bb2dbf8c0
Author: Geert Uytterhoeven <geert+renesas@glider.be>

rtc: nct3018y: Make sure clk_init_data is fully initialized

This commit fixes a potential issue where uninitialized members in the
clk_init_data structure could cause subtle bugs depending on stack
initialization configs. It fully initializes the structure to avoid
future breakage.

> diff --git a/drivers/rtc/rtc-nct3018y.c b/drivers/rtc/rtc-nct3018y.c
> index 700a395fad3a6..2f7ad57057a4b 100644
> --- a/drivers/rtc/rtc-nct3018y.c
> +++ b/drivers/rtc/rtc-nct3018y.c
> @@ -459,8 +459,8 @@ static struct clk *nct3018y_clkout_register_clk(struct nct3018y *nct3018y)
>  {
>  	struct i2c_client *client = nct3018y->client;
>  	struct device_node *node = client->dev.of_node;
> +	struct clk_init_data init = {};
>  	struct clk *clk;
> -	struct clk_init_data init;
>  
>  	init.name = "nct3018y-clkout";
>  	init.ops = &nct3018y_clkout_ops;

[Severity: High]
This isn't a bug introduced by this patch, but does this function leak the
clock provider on probe failure or driver unbind?

Further down in nct3018y_clkout_register_clk():

    of_clk_add_provider(node, of_clk_src_simple_get, clk);
    
When of_clk_add_provider() is called without a corresponding remove path or
devres wrapper like devm_of_clk_add_hw_provider, it looks like a dangling
pointer could be left in the global of_clk_providers list. If the memory
backing the clock is freed via devres and another device queries the clock,
can this lead to a use-after-free?


[Severity: High]
This is a pre-existing issue, but is there a missing restore of the
register state on error paths in nct3018y_rtc_set_time?

drivers/rtc/rtc-nct3018y.c:nct3018y_rtc_set_time() {
    ...
    err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_SC, buf[0]);
    if (err < 0) {
        dev_dbg(&client->dev, "Unable to write NCT3018Y_REG_SC\n");
        return err;
    }
    ...
}

If an I2C write error occurs during time setting, the driver returns early
without restoring NCT3018Y_REG_CTRL. Does this leave the hardware in an
inconsistent state with the NCT3018Y_BIT_TWO flag set incorrectly?


[Severity: High]
This isn't a bug introduced by this patch, but is there a data race on
NCT3018Y_REG_CTRL?

nct3018y_rtc_set_time() executes in process context and performs a
read-modify-write:

drivers/rtc/rtc-nct3018y.c:nct3018y_rtc_set_time() {
    ...
    flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
    ... 
    flags |= NCT3018Y_BIT_TWO;
    err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_CTRL, flags);
    ...
}
    
Simultaneously, the threaded IRQ handler can call nct3018y_set_alarm_mode()
which does the same:

drivers/rtc/rtc-nct3018y.c:nct3018y_set_alarm_mode() {
    ...
    flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
    ...
    flags |= NCT3018Y_BIT_CIE;
    err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_CTRL, flags);
    ...
}
    
Since the RTC core's ops_lock doesn't serialize against the threaded IRQ
handler, can concurrent operations overwrite each other's register state
and lead to missed alarms?


[Severity: High]
This is a pre-existing issue, but could unhandled short reads leak
uninitialized kernel stack memory to userspace?

drivers/rtc/rtc-nct3018y.c:nct3018y_rtc_read_time() {
    ...
    err = i2c_smbus_read_i2c_block_data(client, NCT3018Y_REG_SC,
                                        sizeof(buf), buf);
    if (err < 0)
        return err;
        
    tm->tm_sec = bcd2bin(buf[0] & 0x7F);
    ...
}
    
i2c_smbus_read_i2c_block_data() returns the number of bytes read on success.
If it returns fewer bytes than requested, the remainder of the stack buffer
remains uninitialized but is still parsed into the rtc_time struct.

Can this uninitialized memory then be read by userspace via the RTC_RD_TIME
ioctl?

The same pattern appears to exist in nct3018y_rtc_read_alarm():

drivers/rtc/rtc-nct3018y.c:nct3018y_rtc_read_alarm() {
    ...
    err = i2c_smbus_read_i2c_block_data(client, NCT3018Y_REG_SCA,
                                        sizeof(buf), buf);
    if (err < 0) {
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787241693.git.geert+renesas@glider.be?part=3