Re: [PATCH] watchdog: sp5100_tco: add reversible legacy MMIO relocation

[sashiko-bot@kernel.org]

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

New issues:
- [High] Lockless traversal of the global `iomem_resource` tree can lead to a Time-of-Check to Time-of-Use (TOCTOU) Use-After-Free (UAF) and system crash.
- [High] Algorithmic Denial of Service (O(N) boot lockup) due to a step-by-8-bytes brute-force resource search loop.
- [High] The watchdog relocation logic can allocate memory inside leaf devices such as the IOAPIC, causing hardware conflicts and immediate system hangs.
- [High] Intermediate MMIO base addresses are left actively decoding during byte-by-byte register writes, risking hardware hangs.
- [High] The patch fails to restore the hardware PM registers on system shutdown/reboot, risking the same BIOS boot failure it aims to prevent.
- [Medium] The relocated watchdog MMIO resource is allocated without the `IORESOURCE_BUSY` flag.

Pre-existing issues:
- [High] Incorrect release of a muxed memory region leaves concurrent waiters sleeping forever.
- [High] The probe function leaks the muxed IO port region lock on an error path.
--

--- Patch [1]: [PATCH] watchdog: sp5100_tco: add reversible legacy MMIO relocation ---
commit acc706a989fc9058e6e8b5ba34a48d8247e973fd
Author: Christoph Berliner <caberliner@gmail.com>
Subject: watchdog: sp5100_tco: add reversible legacy MMIO relocation

This patch adds a reversible relocation path for legacy SP5100 watchdog
registers when they conflict with reserved resources. It attempts to locate an
unreserved child resource inside the conflicting parent, reprogram the
watchdog base address, and uses devres to restore the original state upon
failure or removal.

> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> index 2bd3dc25cb030..aafa73bd3be12 100644
> --- a/drivers/watchdog/sp5100_tco.c
> +++ b/drivers/watchdog/sp5100_tco.c
[ ... ]
> +/*
> + * Find the top-level firmware resource containing the conflicting
> + * watchdog address.  The watchdog can then be allocated as a sibling
> + * of resources such as the IOAPIC rather than as a child of them.
> + */
> +static struct resource *
> +sp5100_tco_find_parent_resource(resource_size_t addr)
> +{
> +	struct resource *res;
> +
> +	for (res = iomem_resource.child; res; res = res->sibling) {

[Severity: High]
Is it safe to traverse the iomem_resource.child tree without holding
read_lock(&resource_lock)?

Lockless traversal can race with concurrent resource modifications, potentially
returning an unpinned pointer that could lead to a use-after-free when passed
to request_resource() later.

[Severity: High]
Could this top-level traversal return a leaf device (such as the IOAPIC)
instead of a container?

If it returns a leaf device, request_resource() won't detect any child
conflicts and will succeed, allocating watchdog memory that perfectly overlaps
the IOAPIC. This might cause immediate hardware hangs due to interrupt
starvation.

> +		if (addr >= res->start && addr <= res->end)
> +			return res;
> +	}
> +
> +	return NULL;
> +}
[ ... ]
> +static int sp5100_tco_reprogram_base(struct sp5100_tco *tco,
> +				     u32 conflict_addr,
> +				     const char *dev_name)
> +{
[ ... ]
> +	reloc = devm_kzalloc(dev, sizeof(*reloc), GFP_KERNEL);
> +	if (!reloc)
> +		return -ENOMEM;
> +
> +	res = &reloc->res;
> +	res->name = dev_name;
> +	res->flags = IORESOURCE_MEM;

[Severity: Medium]
Does this resource initialization omit the IORESOURCE_BUSY flag?

Without it, the kernel treats this as a non-leaf container rather than an
exclusively owned region. If another driver calls request_mem_region() for an
overlapping address space, it might incorrectly succeed by carving out a child
resource.

> +	candidate = ALIGN(parent->start, SP5100_WDT_MEM_MAP_SIZE);
> +
> +	for (;;) {
> +		if (candidate > max_addr ||
> +		    max_addr - candidate + 1 < SP5100_WDT_MEM_MAP_SIZE) {
> +			dev_err(dev,
> +				"No free watchdog MMIO slot found inside %pR\n",
> +				parent);
> +			return -EBUSY;
> +		}
> +
> +		res->start = candidate;
> +		res->end = candidate + SP5100_WDT_MEM_MAP_SIZE - 1;
> +
> +		ret = request_resource(parent, res);
> +		if (!ret)
> +			break;
> +
> +		if (ret != -EBUSY)
> +			return ret;
> +
> +		candidate += SP5100_WDT_MEM_MAP_SIZE;
> +	}

[Severity: High]
Does this loop excessively delay boot on large, occupied parent resources?

Blindly incrementing the candidate address by 8 bytes
(SP5100_WDT_MEM_MAP_SIZE) on each request_resource() failure, instead of using
the conflicting resource's end address to skip occupied regions, could result
in millions of loop iterations and acquisitions of the global resource_lock.

> +	reloc->saved_control =
> +		sp5100_tco_read_pm_reg8(SP5100_PM_WATCHDOG_CONTROL);
> +
> +	for (i = 0; i < 4; i++)
> +		reloc->saved_base[i] =
> +			sp5100_tco_read_pm_reg8(SP5100_PM_WATCHDOG_BASE + i);
> +
> +	ret = devm_add_action_or_reset(dev,
> +				       sp5100_tco_release_relocation,
> +				       reloc);

[Severity: High]
Is devm_add_action_or_reset() sufficient for restoring the hardware PM
registers on system shutdown or reboot?

Devres callbacks fire on device unbind or probe failure, but not during
shutdown. Could this leave the MMIO base actively relocated on a warm reboot,
potentially causing the exact BIOS boot failures this patch intends to prevent?

> +	if (ret)
> +		return ret;
> +
> +	mmio_addr = (u32)res->start;
> +
> +	/*
> +	 * Disable the watchdog before changing its decode address.
> +	 */
> +	sp5100_tco_update_pm_reg8(SP5100_PM_WATCHDOG_CONTROL,
> +				  0xff,
> +				  SP5100_PM_WATCHDOG_DISABLE);
> +
> +	/*
> +	 * Preserve the reserved low three bits of BASE0 while
> +	 * reprogramming the aligned watchdog address.
> +	 */
> +	base0_reserved = reloc->saved_base[0] & 0x07;
> +
> +	for (i = 0; i < 4; i++) {
> +		u8 val = (mmio_addr >> (8 * i)) & 0xff;
> +
> +		if (!i)
> +			val |= base0_reserved;
> +
> +		sp5100_tco_write_pm_reg8(SP5100_PM_WATCHDOG_BASE + i, val);
> +	}

[Severity: High]
Are intermediate MMIO base addresses actively decoding during these
byte-by-byte register writes?

For the SP5100/SB7x0 layout, setting SP5100_PM_WATCHDOG_DISABLE disables the
timer, but decoding is controlled by SP5100_PCI_WATCHDOG_DECODE_EN which
appears to remain active. As the base address shifts during the loop, could
DMA or CPU accesses intercept these intermediate addresses and cause hardware
hangs?

[ ... ]
> @@ -404,7 +632,7 @@ static int sp5100_tco_setupdevice_mmio(struct device *dev,
>  		alt_mmio_addr = EFCH_PM_ACPI_MMIO_ADDR +
>  			EFCH_PM_ACPI_MMIO_WDT_OFFSET;
>  
> -	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name);
> +	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name, true);

[Severity: High]
This is a pre-existing issue, but further down in sp5100_tco_setupdevice_mmio()
on the teardown/error path, release_resource(res) is called instead of
release_mem_region(res).

Since the memory was requested with request_mem_region_muxed(), using
release_resource() skips the wake_up(&muxed_resource_wait) call. Could this
leave concurrent waiters sleeping forever?

[ ... ]
> @@ -490,7 +720,24 @@ static int sp5100_tco_setupdevice(struct device *dev,
>  		return -ENODEV;
>  	}

[Severity: High]
This is a pre-existing issue, but the above return -ENODEV occurs when the
switch statement hits the default branch for an unsupported tco_reg_layout.

This exits the probe function without releasing the muxed IO port region
acquired earlier via request_muxed_region(SP5100_IO_PM_INDEX_REG). Does this
lock leak indefinitely starve other drivers requiring the muxed region?

> -	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name);
> +	report_error = tco->tco_reg_layout != sp5100;
> +	ret = sp5100_tco_prepare_base(tco, mmio_addr, alt_mmio_addr, dev_name, report_error);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820222541.16494-1-caberliner@gmail.com?part=1