Re: [RFC PATCH 2/2] drivers/watchdog: Add RISC-V SBI watchdog driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] watchdog: riscv_sbi_wdt: Probe returns success on MSI virtual IRQ failure
- [High] watchdog: riscv_sbi_wdt: Silent u32 overflow in timeout conversion allows premature resets
- [High] watchdog: riscv_sbi_wdt: MSI interrupts leak and Use-After-Free on unbind
- [Medium] watchdog: riscv_sbi_wdt: Missing wdt->dev and parent initializations
- [Medium] watchdog: riscv_sbi_wdt: WDIOF_PRETIMEOUT exposed without hardware support
- [Medium] watchdog: riscv_sbi_wdt: set_timeout fails to adjust pretimeout
- [Low] watchdog: riscv_sbi_wdt: Build failure due to non-existent fwnode member
- [Low] watchdog: riscv_sbi_wdt: Missing module_exit() prevents unloading
- [Low] watchdog: riscv_sbi_wdt: Build failure on configs missing GENERIC_MSI_IRQ
--

commit 5a455815a87582fc7be391ea2d8512e1bd8d0b90
Author: Prajwal N <prajwal.n@oss.qualcomm.com>

drivers/watchdog: Add RISC-V SBI watchdog driver

This commit adds support for the RISC-V SBI watchdog extension, integrating
with the Linux watchdog framework. It configures and controls the watchdog
via standard SBI calls and optionally supports pretimeout MSIs.

> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
[ ... ]
> +config RISCV_SBI_WATCHDOG
> +	tristate "RISC-V SBI based Watchdog support"
> +	depends on RISCV_SBI

[Severity: Low]
Does this configuration need a dependency on CONFIG_GENERIC_MSI_IRQ?

Without it, the unconditional call to
platform_device_msi_init_and_alloc_irqs() in probe will cause a build
failure when that config is disabled.

> diff --git a/drivers/watchdog/riscv_sbi_wdt.c b/drivers/watchdog/riscv_sbi_wdt.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/watchdog/riscv_sbi_wdt.c
[ ... ]
> +/* Convert second to microsecond */
> +static inline u32 sec_to_usec(unsigned int sec)
> +{
> +	return (u32)(sec * 1000000ULL);
> +}

[Severity: High]
Can this silently overflow if a user with privileges configures a large
timeout?

For example, setting a timeout greater than 4294 seconds overflows the u32
calculation, causing the hardware to receive an extremely short timeout
instead, potentially triggering an unexpected system reset.

[ ... ]
> +/* Set the watchdog timeout value */
> +static int sbiwdt_set_timeout(struct watchdog_device *wdd, unsigned int timeout)
> +{
> +	struct sbiwdt *wdt = watchdog_get_drvdata(wdd);
> +	u32 period_usec;
> +	int ret;
> +
> +	period_usec = sec_to_usec(timeout);
> +
> +	ret = sbiwdt_write_attribute(SBI_WDT_PERIOD, period_usec);
> +	if (ret)
> +		return ret;
> +
> +	wdt->attrs.wdt_period = period_usec;
> +	wdt->wdd.timeout = timeout;
> +
> +	return 0;
> +}

[Severity: Medium]
Does this function need to adjust or validate against wdd->pretimeout?

If a user sets a new timeout that is smaller than the current pretimeout,
the hardware could be left in an invalid state where the notification time
is greater than the period time.

[ ... ]
> +/* Get the watchdog timeleft before expiry */
> +static unsigned int sbiwdt_get_timeleft(struct watchdog_device *wdd)
> +{
> +	struct sbiwdt *wdt = watchdog_get_drvdata(wdd);
> +	unsigned int time_left_usec;
> +	int ret;
> +
> +	ret = sbiwdt_read_attribute(SBI_WDT_TIME_LEFT, &time_left_usec);
> +	if (ret) {
> +		dev_err(wdt->dev, "Failed to read watchdog time left: %d\n", ret);

[Severity: Medium]
Will this dereference an uninitialized wdt->dev pointer?

It appears wdt->dev is never assigned during probe, which would lead to
obscured error logs displaying "(NULL device *)".

> +		return 0;
> +	}
> +
> +	return usec_to_sec(time_left_usec);
> +}
> +
> +static const struct watchdog_info sbiwdt_info = {
> +	.options	=	WDIOF_SETTIMEOUT	|
> +				WDIOF_KEEPALIVEPING	|
> +				WDIOF_PRETIMEOUT	|
> +				WDIOF_MAGICCLOSE,

[Severity: Medium]
Should WDIOF_PRETIMEOUT only be advertised if the firmware actually
supports MSI capabilities?

Statically advertising it means userspace could configure a pretimeout that
will silently never trigger on platforms without MSI support.

[ ... ]
> +/* Initialize the watchdog attributes */
> +static int sbiwdt_init_attributes(struct sbiwdt *wdt)
> +{
[ ... ]
> +	/* Initialize the watchdog timeout to default value */
> +	timeout_usec = sec_to_usec(SBI_WDT_DEFAULT_TIMEOUT);
> +	ret = sbiwdt_write_attribute(SBI_WDT_PERIOD, timeout_usec);
> +	if (ret)
> +		return ret;
> +
> +	wdt->attrs.wdt_period = timeout_usec;
> +	wdt->wdd.timeout = SBI_WDT_DEFAULT_TIMEOUT;

[Severity: High]
Should wdt->wdd.max_timeout be explicitly initialized here?

Without an upper bound set by the driver, the watchdog core defaults to no
hardware limit, allowing large timeouts to be passed to sec_to_usec()
which then truncates them via integer overflow.

[ ... ]
> +static int sbiwdt_probe(struct platform_device *pdev)
> +{
> +	struct sbiwdt *wdt;
> +	struct imsic_global_config *imsic_global;
> +	struct irq_domain *msi_domain;
> +	struct device *dev = &pdev->dev;
> +	int rc;
> +
> +	/* Allocate memory for the global watchdog structure */
> +	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, wdt);
> +	watchdog_set_drvdata(&wdt->wdd, wdt);

[Severity: Medium]
Are we missing assignments for wdt->dev and wdt->wdd.parent here?

Failing to set wdt->wdd.parent = dev breaks the sysfs hierarchy for the
watchdog device, and missing wdt->dev = dev leads to the uninitialized
pointer use observed in sbiwdt_get_timeleft().

[ ... ]
> +	/* Setup MSI if the capability is available */
> +	if (wdt->attrs.wdt_capability & SBI_WDT_CAP_MSI_SUPPORT_MASK) {
> +		imsic_global = imsic_get_global_config();
> +		if (!imsic_global) {
> +			dev_err(dev, "IMSIC firmware node is unavailable\n");
> +			return -ENODEV;
> +		}
> +
> +		msi_domain = irq_find_matching_fwnode(imsic_global->fwnode,
> +							DOMAIN_BUS_PLATFORM_MSI);

[Severity: Low]
Does struct imsic_global_config actually have an fwnode member?

It appears this member does not exist in the upstream struct definition,
which will cause an immediate build failure.

> +		if (!msi_domain) {
> +			dev_err(dev, "IMSIC platform MSI domain is unavailable\n");
> +			return -ENODEV;
> +		}
> +
> +		dev_set_msi_domain(&pdev->dev, msi_domain);
> +
> +		/* MSI setup*/
> +		rc = platform_device_msi_init_and_alloc_irqs(dev, 1, sbiwdt_msi_write);

[Severity: High]
Are these MSI allocations ever cleaned up if subsequent probe steps fail?

Since this is not a device-managed function, error paths that return early
will leak these interrupts.

> +		if (rc) {
> +			dev_err(dev, "Failed to allocate MSIs\n");
> +			return rc;
> +		}
> +
> +		wdt->virq = msi_get_virq(dev, 0);
> +		if (!wdt->virq) {
> +			dev_err(dev, "Failed to get MSI virtual IRQ: %d\n", wdt->virq);
> +			return wdt->virq;
> +		}

[Severity: High]
Will this return 0 (success) when it intends to return a failure?

msi_get_virq() returns 0 on failure. Returning wdt->virq here falsely
signals a successful probe to the driver core, leaving the system silently
unprotected since the watchdog never fully registers.

[ ... ]
> +static struct platform_driver sbiwdt_driver = {
> +	.probe  = sbiwdt_probe,
> +	.driver = {
> +			.name = DRV_NAME,
> +	},
> +};

[Severity: High]
Does this driver need a .remove callback to properly free the MSIs?

Without one, unbinding the driver permanently leaks the interrupts. If a
leaked MSI triggers later, it could execute sbiwdt_msi_write() causing a
use-after-free on the released driver data.

> +
> +static int __init sbiwdt_init(void)
> +{
> +	int ret;
> +	struct platform_device *pdev;
> +
> +	/* Probe for SBI WDT extension */
> +	if (sbi_spec_version < sbi_mk_version(3, 0) ||
> +		sbi_probe_extension(SBI_EXT_WDT) <= 0) {
> +		return -ENODEV;
> +	}
> +
> +	ret = platform_driver_register(&sbiwdt_driver);
> +	if (ret)
> +		return ret;
> +
> +	pdev = platform_device_register_simple(DRV_NAME, -1, NULL, 0);
> +	if (IS_ERR(pdev)) {
> +		platform_driver_unregister(&sbiwdt_driver);
> +		return PTR_ERR(pdev);
> +	}
> +
> +	return 0;
> +}
> +late_initcall(sbiwdt_init);

[Severity: Low]
Should a symmetric module_exit() routine be provided here?

Since the driver is configured as tristate, omitting an exit function
prevents it from ever being unloaded via rmmod.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002163758.2919372-1-prajwal.n@oss.qualcomm.com?part=2