[PATCH 0/5] rtc: Make sure clk_init_data is fully initialized

[PATCH 0/5] rtc: Make sure clk_init_data is fully initialized

[Geert Uytterhoeven]

	Hi all,

The clk_init_data structure contains several mutually-exclusive members
for different methods to specify the possible parents of a clock,
prompting drivers to initialize only the members they need.  However,
not initializing all members may cause subtle issues, which are only
exposed when CONFIG_INIT_STACK_ALL_PATTERN or CONFIG_INIT_STACK_NONE is
enabled.

Hence this series aims to make sure all members are fully initialized,
to avoid such bugs, and to prevent future breakage when converting
drivers to a different method for specifying the parents.

Part One[1] fixed all cases that I identified to be real bugs, in
response to a crash I saw on BeagleBone Black.

This series is the RTC subpart of Part Two, which fixes remaining cases
that are currently harmless.  These are still fragile, and may cause
future breakage when converting drivers to a different method for
specifying the parents.

Thanks for your comments!

[1] "[PATCH treewide 0/5] clk: Make sure clk_init_data is fully
     initialized (part 1)"
    https://lore.kernel.org/cover.1787165329.git.geert+renesas@glider.be

Geert Uytterhoeven (5):
  rtc: hym8563: Make sure clk_init_data is fully initialized
  rtc: m41t80: Make sure clk_init_data is fully initialized
  rtc: nct3018y: Make sure clk_init_data is fully initialized
  rtc: philips: Make sure clk_init_data is fully initialized
  rtc: microcrystal: Make sure clk_init_data is fully initialized

 drivers/rtc/rtc-hym8563.c  | 2 +-
 drivers/rtc/rtc-m41t80.c   | 2 +-
 drivers/rtc/rtc-nct3018y.c | 2 +-
 drivers/rtc/rtc-pcf85063.c | 2 +-
 drivers/rtc/rtc-pcf8563.c  | 2 +-
 drivers/rtc/rtc-rv3028.c   | 2 +-
 drivers/rtc/rtc-rv3032.c   | 2 +-
 7 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.43.0

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/5] rtc: hym8563: Make sure clk_init_data is fully initialized

[Geert Uytterhoeven]

The clk_init_data structure contains several mutually-exclusive members
for different methods to specify the possible parents of a clock,
prompting drivers to initialize only the members they need.  However,
not initializing all members may cause subtle issues, which are only
exposed when CONFIG_INIT_STACK_ALL_PATTERN or CONFIG_INIT_STACK_NONE is
enabled.

Make sure all members are fully initialized, to avoid such bugs, and to
prevent future breakage when converting drivers to a different method
for specifying the parents.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Compile-tested only.
---
 drivers/rtc/rtc-hym8563.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-hym8563.c b/drivers/rtc/rtc-hym8563.c
index 3156aa5f2d9ff264..1d52cc3def02ef22 100644
--- a/drivers/rtc/rtc-hym8563.c
+++ b/drivers/rtc/rtc-hym8563.c
@@ -376,8 +376,8 @@ static struct clk *hym8563_clkout_register_clk(struct hym8563 *hym8563)
 {
 	struct i2c_client *client = hym8563->client;
 	struct device_node *node = client->dev.of_node;
+	struct clk_init_data init = {};
 	struct clk *clk;
-	struct clk_init_data init;
 	int ret;
 
 	ret = i2c_smbus_write_byte_data(client, HYM8563_CLKOUT,
-- 
2.43.0

[PATCH 2/5] rtc: m41t80: Make sure clk_init_data is fully initialized

[Geert Uytterhoeven]

The clk_init_data structure contains several mutually-exclusive members
for different methods to specify the possible parents of a clock,
prompting drivers to initialize only the members they need.  However,
not initializing all members may cause subtle issues, which are only
exposed when CONFIG_INIT_STACK_ALL_PATTERN or CONFIG_INIT_STACK_NONE is
enabled.

Make sure all members are fully initialized, to avoid such bugs, and to
prevent future breakage when converting drivers to a different method
for specifying the parents.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Compile-tested only.
---
 drivers/rtc/rtc-m41t80.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
index 3c8c379392c14078..ec86ca3f2c4c85f8 100644
--- a/drivers/rtc/rtc-m41t80.c
+++ b/drivers/rtc/rtc-m41t80.c
@@ -574,8 +574,8 @@ static struct clk *m41t80_sqw_register_clk(struct m41t80_data *m41t80)
 	struct i2c_client *client = m41t80->client;
 	struct device_node *node = client->dev.of_node;
 	struct device_node *fixed_clock;
+	struct clk_init_data init = {};
 	struct clk *clk;
-	struct clk_init_data init;
 	int ret;
 
 	fixed_clock = of_get_child_by_name(node, "clock");
-- 
2.43.0

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

[Geert Uytterhoeven]

The clk_init_data structure contains several mutually-exclusive members
for different methods to specify the possible parents of a clock,
prompting drivers to initialize only the members they need.  However,
not initializing all members may cause subtle issues, which are only
exposed when CONFIG_INIT_STACK_ALL_PATTERN or CONFIG_INIT_STACK_NONE is
enabled.

Make sure all members are fully initialized, to avoid such bugs, and to
prevent future breakage when converting drivers to a different method
for specifying the parents.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Compile-tested only.
---
 drivers/rtc/rtc-nct3018y.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-nct3018y.c b/drivers/rtc/rtc-nct3018y.c
index 700a395fad3a63b6..2f7ad57057a4b8a1 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;
-- 
2.43.0

[PATCH 4/5] rtc: philips: Make sure clk_init_data is fully initialized

[Geert Uytterhoeven]

The clk_init_data structure contains several mutually-exclusive members
for different methods to specify the possible parents of a clock,
prompting drivers to initialize only the members they need.  However,
not initializing all members may cause subtle issues, which are only
exposed when CONFIG_INIT_STACK_ALL_PATTERN or CONFIG_INIT_STACK_NONE is
enabled.

Make sure all members are fully initialized, to avoid such bugs, and to
prevent future breakage when converting drivers to a different method
for specifying the parents.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Compile-tested only.
---
 drivers/rtc/rtc-pcf85063.c | 2 +-
 drivers/rtc/rtc-pcf8563.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
index 01e209d88f5f357f..8cb9ffc73f6d9001 100644
--- a/drivers/rtc/rtc-pcf85063.c
+++ b/drivers/rtc/rtc-pcf85063.c
@@ -498,7 +498,7 @@ static const struct clk_ops pcf85063_clkout_ops = {
 static struct clk *pcf85063_clkout_register_clk(struct pcf85063 *pcf85063)
 {
 	struct clk *clk;
-	struct clk_init_data init;
+	struct clk_init_data init = {};
 	struct device_node *node = pcf85063->rtc->dev.parent->of_node;
 	struct device_node *fixed_clock;
 
diff --git a/drivers/rtc/rtc-pcf8563.c b/drivers/rtc/rtc-pcf8563.c
index 81d13733b1e9372f..5503eb61df13f4a6 100644
--- a/drivers/rtc/rtc-pcf8563.c
+++ b/drivers/rtc/rtc-pcf8563.c
@@ -425,7 +425,7 @@ static const struct clk_ops pcf8563_clkout_ops = {
 static struct clk *pcf8563_clkout_register_clk(struct pcf8563 *pcf8563)
 {
 	struct device_node *node = pcf8563->rtc->dev.parent->of_node;
-	struct clk_init_data init;
+	struct clk_init_data init = {};
 	struct clk *clk;
 	int ret;
 
-- 
2.43.0

[PATCH 5/5] rtc: microcrystal: Make sure clk_init_data is fully initialized

[Geert Uytterhoeven]

The clk_init_data structure contains several mutually-exclusive members
for different methods to specify the possible parents of a clock,
prompting drivers to initialize only the members they need.  However,
not initializing all members may cause subtle issues, which are only
exposed when CONFIG_INIT_STACK_ALL_PATTERN or CONFIG_INIT_STACK_NONE is
enabled.

Make sure all members are fully initialized, to avoid such bugs, and to
prevent future breakage when converting drivers to a different method
for specifying the parents.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Compile-tested only.
---
 drivers/rtc/rtc-rv3028.c | 2 +-
 drivers/rtc/rtc-rv3032.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-rv3028.c b/drivers/rtc/rtc-rv3028.c
index d96f6bb68850dadb..d25e33d0835eea04 100644
--- a/drivers/rtc/rtc-rv3028.c
+++ b/drivers/rtc/rtc-rv3028.c
@@ -816,7 +816,7 @@ static int rv3028_clkout_register_clk(struct rv3028_data *rv3028,
 {
 	int ret;
 	struct clk *clk;
-	struct clk_init_data init;
+	struct clk_init_data init = {};
 	struct device_node *node = client->dev.of_node;
 
 	ret = regmap_update_bits(rv3028->regmap, RV3028_STATUS,
diff --git a/drivers/rtc/rtc-rv3032.c b/drivers/rtc/rtc-rv3032.c
index 6bafdec637aeadd0..687faaef42b257e5 100644
--- a/drivers/rtc/rtc-rv3032.c
+++ b/drivers/rtc/rtc-rv3032.c
@@ -752,7 +752,7 @@ static int rv3032_clkout_register_clk(struct rv3032_data *rv3032,
 {
 	int ret;
 	struct clk *clk;
-	struct clk_init_data init;
+	struct clk_init_data init = {};
 	struct device_node *node = client->dev.of_node;
 
 	ret = regmap_update_bits(rv3032->regmap, RV3032_TLSB, RV3032_TLSB_CLKF, 0);
-- 
2.43.0

Re: [PATCH 1/5] rtc: hym8563: Make sure clk_init_data is fully initialized

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-after-free and resource leak on driver unbind or probe failure.
- [High] RTC alarm interrupts are acknowledged but not reported to the RTC subsystem, breaking alarm functionality.
--

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

rtc: hym8563: Make sure clk_init_data is fully initialized

The clk_init_data structure contains several mutually-exclusive members
for different methods to specify the possible parents of a clock. This
commit ensures all members are fully initialized to avoid subtle issues
with uninitialized stack memory.

> diff --git a/drivers/rtc/rtc-hym8563.c b/drivers/rtc/rtc-hym8563.c
> index 3156aa5f2d9ff..1d52cc3def02e 100644
> --- a/drivers/rtc/rtc-hym8563.c
> +++ b/drivers/rtc/rtc-hym8563.c
> @@ -376,8 +376,8 @@ static struct clk *hym8563_clkout_register_clk(struct hym8563 *hym8563)
>  {
>  	struct i2c_client *client = hym8563->client;
>  	struct device_node *node = client->dev.of_node;
> +	struct clk_init_data init = {};
>  	struct clk *clk;
> -	struct clk_init_data init;
>  	int ret;
>  
>  	ret = i2c_smbus_write_byte_data(client, HYM8563_CLKOUT,

[Severity: High]
This isn't a bug introduced by this patch, but could there be a
use-after-free if devm_rtc_register_device() fails or if the driver is
unbound?

Looking at hym8563_clkout_register_clk(), it registers an unmanaged clock
and an OF clock provider using clk_register() and of_clk_add_provider().
However, the hym8563 structure containing the clkout_hw is allocated via
devm_kzalloc().

If devm_rtc_register_device() fails in hym8563_probe(), or if the driver
is later unbound (there is no .remove callback in hym8563_driver), the
devres framework will free the hym8563 structure. Because the clock is
never unregistered, the clock framework will retain a pointer to the
freed clkout_hw, leading to a crash if the clock is accessed later.

Should this use devm_clk_register() and devm_of_clk_add_hw_provider(), or
provide a .remove callback to clean up the clock and OF provider?

[Severity: High]
This is a pre-existing issue, but does the interrupt handler fail to
notify the RTC core of alarm events?

In hym8563_irq(), the handler reads and clears the hardware alarm flag
(HYM8563_CTL2_AF) to acknowledge the interrupt at the hardware level, but
it then exits like this:

out:
        rtc_unlock(hym8563->rtc);
        return IRQ_HANDLED;
}

It appears to never call rtc_update_irq(). Doesn't this mean rtc->irq_queue
is never woken up, causing any userspace process waiting for the alarm via
select(), poll(), or timerfd to stall indefinitely?

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

Re: [PATCH 2/5] rtc: m41t80: Make sure clk_init_data is fully initialized

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing cleanup of clock registration and OF provider on probe failure and device unbind leads to memory leaks and Use-After-Free.
- [High] Missing cleanup of watchdog device and reboot notifier on probe failure causes resource leaks and Use-After-Free.
- [High] The seconds register is corrupted with the flags register value during time setting.
- [High] Clock operations race with RTC operations causing I2C register corruption.
--

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

rtc: m41t80: Make sure clk_init_data is fully initialized

The clk_init_data structure contains several mutually-exclusive members for
different methods to specify the possible parents of a clock, prompting
drivers to initialize only the members they need. Make sure all members are
fully initialized, to avoid bugs and prevent future breakage.

> diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
> index 3c8c379392c14..ec86ca3f2c4c8 100644
> --- a/drivers/rtc/rtc-m41t80.c
> +++ b/drivers/rtc/rtc-m41t80.c
> @@ -574,8 +574,8 @@ static struct clk *m41t80_sqw_register_clk(struct m41t80_data *m41t80)
>  	struct i2c_client *client = m41t80->client;
>  	struct device_node *node = client->dev.of_node;
>  	struct device_node *fixed_clock;
> +	struct clk_init_data init = {};
>  	struct clk *clk;
> -	struct clk_init_data init;
>  	int ret;

[Severity: High]
This is a pre-existing issue, but does m41t80_sqw_register_clk() leak the
registered clock and OF provider?

If devm_rtc_register_device() fails in m41t80_probe(), or when the driver
unbinds and calls m41t80_remove(), the m41t80_data struct is freed. However,
the clock and its OF provider remain registered:

m41t80_sqw_register_clk() {
    ...
    clk = clk_register(&client->dev, &m41t80->sqw);
    if (!IS_ERR(clk))
        of_clk_add_provider(node, of_clk_src_simple_get, clk);
    ...
}

Will subsequent clock operations dereference the freed m41t80->sqw field?

[Severity: High]
This is a pre-existing issue, but is there a missing cleanup of the watchdog
device and reboot notifier on probe failure?

In m41t80_probe(), the watchdog device and reboot notifier are registered:

m41t80_probe() {
    ...
    rc = misc_register(&wdt_dev);
    if (rc)
        return rc;
    rc = register_reboot_notifier(&wdt_notifier);
    ...
}

If devm_rtc_register_device() subsequently fails, the driver returns the
error directly without deregistering the watchdog device or the reboot
notifier:

m41t80_probe() {
    ...
    rc = devm_rtc_register_device(m41t80_data->rtc);
    if (rc)
        return rc;
    ...
}

Could this lead to a use-after-free if userspace then accesses /dev/watchdog,
causing wdt_ping() to dereference the freed clientdata memory?

[Severity: High]
This is a pre-existing issue, but does m41t80_rtc_set_time() inadvertently
corrupt the seconds register?

During time setting, when attempting to clear the STOP bit in the seconds
register, it writes the flags variable instead:

m41t80_rtc_set_time() {
    ...
    err = i2c_smbus_write_byte_data(client, M41T80_REG_SEC,
                                    flags & ~M41T80_SEC_ST);
    ...
}

Since flags is populated earlier by reading the FLAGS register
(M41T80_REG_FLAGS), would this write the raw value of the FLAGS register into
the seconds register, overwriting the previously set time?

[Severity: High]
This is a pre-existing issue, but can clock operations race with RTC
operations and corrupt I2C registers?

The CCF clock callbacks perform read-modify-write sequences on the
M41T80_REG_ALARM_MON and M41T80_REG_WDAY registers without holding a lock
that protects against concurrent time-setting operations:

m41t80_sqw_control() {
    ...
    int ret = i2c_smbus_read_byte_data(client, M41T80_REG_ALARM_MON);
    ...
    ret = i2c_smbus_write_byte_data(client, M41T80_REG_ALARM_MON, ret);
    ...
}

m41t80_sqw_set_rate() {
    ...
    reg = i2c_smbus_read_byte_data(client, reg_sqw);
    ...
    ret = i2c_smbus_write_byte_data(client, reg_sqw, reg);
    ...
}

Meanwhile, the RTC core callbacks modify these same registers under the
protection of the RTC subsystem's rtc_lock.

Since the CCF operations execute asynchronously under clock framework locks
and do not acquire the rtc_lock, could concurrent modifications by both
subsystems clobber the shared I2C registers?

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

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

[sashiko-bot]

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

Re: [PATCH 4/5] rtc: philips: Make sure clk_init_data is fully initialized

[sashiko-bot]

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

Pre-existing issues:
- [High] Unmanaged `of_clk_add_provider()` causes memory leak and Use-After-Free (UAF) on probe failure or driver unbind.
--

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

rtc: philips: Make sure clk_init_data is fully initialized

This commit zero-initializes clk_init_data upon declaration to prevent subtle
bugs from uninitialized mutually-exclusive members. This helps avoid issues
exposed when certain stack initialization configurations are enabled.

> diff --git a/drivers/rtc/rtc-pcf85063.c b/drivers/rtc/rtc-pcf85063.c
> index 01e209d88f5f3..8cb9ffc73f6d9 100644
> --- a/drivers/rtc/rtc-pcf85063.c
> +++ b/drivers/rtc/rtc-pcf85063.c
> @@ -498,7 +498,7 @@ static const struct clk_ops pcf85063_clkout_ops = {
>  static struct clk *pcf85063_clkout_register_clk(struct pcf85063 *pcf85063)
>  {
>  	struct clk *clk;
> -	struct clk_init_data init;
> +	struct clk_init_data init = {};

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free and
resource leak regarding the clock provider registered later in this function.

In pcf85063_clkout_register_clk(), the clock provider is added using the
unmanaged API without any cleanup mechanism:

	clk = devm_clk_register(&pcf85063->rtc->dev, &pcf85063->clkout_hw);
	if (!IS_ERR(clk))
		of_clk_add_provider(node, of_clk_src_simple_get, clk);

If the probe later fails when calling devm_rtc_register_device() in
pcf85063_probe():

	return devm_rtc_register_device(pcf85063->rtc);

Or if the driver is unbound via sysfs, devres will automatically free the
clock structure and driver data.

Since of_clk_add_provider() is unmanaged, does this leave the clock provider
in the global of_clk_providers list pointing to a freed clock?

It seems a subsequent device tree lookup could invoke of_clk_src_simple_get()
and cause a use-after-free by returning the dangling pointer.

Would it be better to use a managed API or devm_add_action_or_reset()
to ensure the provider is safely removed on teardown?

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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Unmanaged `of_clk_add_provider` combined with managed `devm_clk_register` creates a Use-After-Free and memory leak upon driver unbind.
- [High] Missing timeout in `rv3032_hwmon_read_temp` hardware polling loop can cause an infinite loop.
--

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

rtc: microcrystal: Make sure clk_init_data is fully initialized

This commit fully initializes the clk_init_data structure to avoid subtle
issues when CONFIG_INIT_STACK_ALL_PATTERN or CONFIG_INIT_STACK_NONE is
enabled. This prevents breakage when drivers are converted to different
methods for specifying parents.

> diff --git a/drivers/rtc/rtc-rv3028.c b/drivers/rtc/rtc-rv3028.c
> index d96f6bb68850d..d25e33d0835ee 100644
> --- a/drivers/rtc/rtc-rv3028.c
> +++ b/drivers/rtc/rtc-rv3028.c
> @@ -816,7 +816,7 @@ static int rv3028_clkout_register_clk(struct rv3028_data *rv3028,
>  {
>  	int ret;
>  	struct clk *clk;
> -	struct clk_init_data init;
> +	struct clk_init_data init = {};
>  	struct device_node *node = client->dev.of_node;
>  
>  	ret = regmap_update_bits(rv3028->regmap, RV3028_STATUS,

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this function,
I noticed a potential issue with how the clock provider is registered later in
rv3028_clkout_register_clk():

	clk = devm_clk_register(&client->dev, &rv3028->clkout_hw);
	if (!IS_ERR(clk))
		of_clk_add_provider(node, of_clk_src_simple_get, clk);

Because of_clk_add_provider() is unmanaged, does this cause a use-after-free
and memory leak when the driver is unbound?

The driver registers a clock provider using the unmanaged
of_clk_add_provider() but uses devm_clk_register() for the clock itself.

When the driver is unbound, devres automatically unwinds devm_clk_register()
and frees the clock structure.

Since there is no remove callback or devres action to call
of_clk_del_provider(), it seems the provider remains in the global
of_clk_providers list with a dangling pointer to the freed clock.

Since driver unbind can be triggered by a privileged user via sysfs or by
hardware hot-unplug, can this lead to a kernel panic if another driver looks
up the clock after the device is unbound?

> diff --git a/drivers/rtc/rtc-rv3032.c b/drivers/rtc/rtc-rv3032.c
> index 6bafdec637aea..687faaef42b25 100644
> --- a/drivers/rtc/rtc-rv3032.c
> +++ b/drivers/rtc/rtc-rv3032.c
> @@ -752,7 +752,7 @@ static int rv3032_clkout_register_clk(struct rv3032_data *rv3032,
>  {
>  	int ret;
>  	struct clk *clk;
> -	struct clk_init_data init;
> +	struct clk_init_data init = {};
>  	struct device_node *node = client->dev.of_node;
>  
>  	ret = regmap_update_bits(rv3032->regmap, RV3032_TLSB, RV3032_TLSB_CLKF, 0);

[Severity: High]
This is also a pre-existing issue, but rv3032_clkout_register_clk() appears
to have the same unmanaged provider registration:

	clk = devm_clk_register(&client->dev, &rv3032->clkout_hw);
	if (!IS_ERR(clk))
		of_clk_add_provider(node, of_clk_src_simple_get, clk);

Does this also leave a dangling pointer in the global of_clk_providers list
when devres frees the clock upon unbind?

While looking at drivers/rtc/rtc-rv3032.c, I also noticed another
pre-existing issue in the hwmon code:

drivers/rtc/rtc-rv3032.c:rv3032_hwmon_read_temp() {
    ...
	do {
		prev = temp;

		ret = regmap_bulk_read(rv3032->regmap, RV3032_TLSB, buf, sizeof(buf));
		if (ret)
			return ret;

		temp = sign_extend32(buf[1], 7) << 4;
		temp |= FIELD_GET(RV3032_TLSB_TEMP, buf[0]);
	} while (temp != prev);
    ...
}

[Severity: High]
This is a pre-existing issue, but does this loop lack a timeout or maximum
retry count?

If the I2C hardware enters an unstable state and is faulty or noisy, it could
consistently return varying values, preventing the loop from terminating.

Since this code path is exposed to unprivileged userspace via the hwmon sysfs
attributes, can this be used by unprivileged users to trigger an indefinite
hang on a task?

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