Re: [PATCH 3/4] clocksource/drivers/timer-tegra186: Register all accessible watchdog timers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Data race on the shared TKEIE register during watchdog enable.
- [High] Hardware timer collision allows multiple watchdogs to clobber the same physical timer registers.
- [High] Missing bounds check on the timer source index allows out-of-bounds MMIO accesses.
--

commit 173c64a9e63a9e27e2f2b9544cfaa4b12f9d5f9c
Author: Kartik Rajput <kkartik@nvidia.com>

clocksource/drivers/timer-tegra186: Register all accessible watchdog timers

Tegra186+ SoCs expose multiple watchdog timers, but the driver only
registers WDT(0). Iterate over num_wdts and, for each WDT, check the SCR
(firewall) registers in the TKE block to determine whether Linux has read
and write access. Register the watchdogs that are accessible.

> diff --git a/drivers/clocksource/timer-tegra186.c b/drivers/clocksource/timer-tegra186.c
> index fd82a73ab2d27..dd1d1a0dd63eb 100644
> --- a/drivers/clocksource/timer-tegra186.c
> +++ b/drivers/clocksource/timer-tegra186.c

[ ... ]

> @@ -442,12 +467,20 @@ static int tegra186_timer_probe(struct platform_device *pdev)
>  	if (err < 0)
>  		return err;
>  
> -	/* create a watchdog using a preconfigured timer */
> -	tegra->wdt = tegra186_wdt_create(tegra, 0);
> -	if (IS_ERR(tegra->wdt)) {
> -		err = PTR_ERR(tegra->wdt);
> -		dev_err(dev, "failed to create WDT: %d\n", err);
> -		return err;
> +	tegra->wdts = devm_kcalloc(dev, tegra->soc->num_wdts, sizeof(*tegra->wdts), GFP_KERNEL);
> +	if (!tegra->wdts)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < tegra->soc->num_wdts; i++) {
> +		if (!tegra186_wdt_is_accessible(tegra, i)) {
> +			dev_warn(dev, "WDT%u is not accessible\n", i);
> +			continue;
> +		}
> +
> +		tegra->wdts[i] = tegra186_wdt_create(tegra, i);

If the bootloader hasn't configured the watchdogs, could multiple watchdogs
extract the exact same source index and clobber each other's timer registers?

Looking at tegra186_wdt_create():

    value = wdt_readl(wdt, WDTCR);
    ...
    source = value & WDTCR_TIMER_SOURCE_MASK;
    wdt->tmr = tegra186_tmr_create(tegra, source);

If WDTCR contains its default reset value, multiple watchdogs could be
assigned the identical physical timer MMIO region, allowing userspace to
concurrently overwrite the same TMRCR and TMRCSSR registers.

Is a bounds check needed to prevent out-of-bounds MMIO accesses?
The WDTCR_TIMER_SOURCE_MASK allows values up to 15, but
tegra->soc->num_timers is only 10 for Tegra186. If a value like 14 or 15
is provided, the computed MMIO offset in tegra186_tmr_create() could fall
entirely outside the valid mapped memory region.

Finally, can concurrent watchdog operations race on the shared TKEIE register?

Looking at tegra186_wdt_enable():

    value = readl(tegra->regs + TKEIE(wdt->tmr->hwirq));
    value |= TKEIE_WDT_MASK(wdt->index, 1);
    writel(value, tegra->regs + TKEIE(wdt->tmr->hwirq));

Since hwirq is 0, all watchdogs access the exact same physical register. The
watchdog core's wdd->lock mutex only serializes operations on a per-device
basis. Concurrent start or ping operations to different watchdog devices
could cause one CPU's read-modify-write sequence to overwrite another's,
losing interrupt unmasks.

> +		if (IS_ERR(tegra->wdts[i]))
> +			return dev_err_probe(dev, PTR_ERR(tegra->wdts[i]),
> +					     "failed to create WDT%u\n", i);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260507154557.2082697-1-kkartik@nvidia.com?part=3