Re: [PATCH 2/2] bus: add Altera timeout bridge driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] bus: UAF of irq_node during teardown or early interrupt
- [High] bus: CFI violation in devres teardown due to mismatched function pointer cast
- [High] bus: driver incorrectly claims simple-bus devices, breaking power management
- [Medium] bus: bypassed hardware initialization when bound via driver_override
- [Medium] bus: leaked child devices due to missing depopulation on driver remove
- [Medium] bus: incorrectly adds physical base address to timeout address
- [Medium] bus: un-rate-limited error logging in ISR causes kernel log flooding
--

commit c74336cb2c00c16659c7f03c8c51687597e67ab5
Author: Vyacheslav Yurkov <V.Yurkov.EXT@bruker.com>
Subject: bus: add Altera timeout bridge driver

Add support for the Altera timeout bridge IP. The bridge connects an Avalon
bus to the HPS and monitors accesses for timeouts. When a timeout occurs
the interrupt communicates the event via sysfs.

> diff --git a/drivers/bus/altera-timeout-bridge.c b/drivers/bus/altera-timeout-bridge.c
> new file mode 100644
> index 0000000000000..bb44ff4a75910
> --- /dev/null
> +++ b/drivers/bus/altera-timeout-bridge.c
> @@ -0,0 +1,219 @@

[ ... ]

> +static irqreturn_t altera_timeout_bridge_isr(int irq, void *data)
> +{
> +	struct altera_timeout_bridge_data *driver_data = data;
> +	u32 timed_out_address;
> +	resource_size_t effective_base_address;
> +	resource_size_t effective_timed_out_address;
> +
> +	atomic_inc(&driver_data->irq_count);
> +
> +	// Read and print the timed-out address (encoded in the lower nibble
> +	// 0x8..0xF according to AXI Timeout Bridge IP (CSR Interrupt Status Information)
> +	timed_out_address = bridge_reg_read(
> +		driver_data,
> +		ALTERA_TIMEOUT_BRIDGE_REG_TIMED_OUT_ADDRESS);
> +	effective_base_address = driver_data->bridge_register_resource->start;
> +	effective_timed_out_address = effective_base_address + timed_out_address;

[Severity: Medium]
Does this calculation corrupt the actual timed-out AXI address by erroneously
adding the driver's own register base?

> +	pr_err("%s: timeout at address 0x%llx\n",
> +		__func__, (unsigned long long)effective_timed_out_address);

[Severity: Medium]
Will this synchronous pr_err() statement flood the kernel log if the hardware
triggers continuous faults?

> +
> +	// Reset IRQ status in the bridge
> +	bridge_reg_write(
> +		driver_data,
> +		ALTERA_TIMEOUT_BRIDGE_REG_IRQ_STATUS_RESET,
> +		0x1);
> +	// notify the user space
> +	sysfs_notify_dirent(driver_data->irq_node);

[Severity: High]
Can this sysfs_notify_dirent() dereference an uninitialized or freed
irq_node?

During probe, devm_request_irq() is called before driver_data->irq_node is
initialized, leaving a window where early interrupts see a NULL pointer.
Additionally, because the devres action to free irq_node is registered after
the interrupt request, devres executes in LIFO order and frees irq_node
before the interrupt is disabled during teardown.

> +
> +	return IRQ_HANDLED;
> +}
> +static int altera_timeout_bridge_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct of_device_id *match;
> +	const struct of_dev_auxdata *lookup = dev_get_platdata(dev);
> +	struct device_node *np = dev->of_node;
> +	struct altera_timeout_bridge_data *driver_data;
> +	int ret = 0;
> +	int irq = -1;
> +
> +	/*
> +	 * Allow user to use driver_override to bind this driver to a
> +	 * transparent bus device which has a different compatible string
> +	 * that's not listed in simple_pm_bus_of_match. We don't want to do any
> +	 * of the simple-pm-bus tasks for these devices, so return early.
> +	 */
> +	if (device_has_driver_override(&pdev->dev))
> +		return 0;

[Severity: Medium]
Does this check incorrectly bypass the entire hardware initialization logic
if a driver override is used?

If a user explicitly binds the driver to a device via the driver_override
sysfs attribute, the probe function immediately returns 0, skipping all
memory allocations, register mappings, and interrupt requests.

> +
> +	match = of_match_device(dev->driver->of_match_table, dev);
> +
> +	/* Use a fall back logic like in simple-pm-bus */
> +	if (match && match->data) {
> +		if (of_property_match_string(np, "compatible", match->compatible) == 0)
> +			return 0;

[Severity: High]
Does this cause the driver to incorrectly claim generic system buses and break
power management?

The driver's OF match table includes the generic "simple-bus" compatible
string. Returning 0 here prevents the correct simple-pm-bus driver from
binding to these buses, which is responsible for enabling their power domains
and clocks, breaking all child devices on the bus.

> +		else
> +			return -ENODEV;
> +	}

[ ... ]

> +	ret = devm_request_irq(dev, irq,
> +			       altera_timeout_bridge_isr,
> +			       0,
> +			       dev_name(dev),
> +			       driver_data);
> +	if (ret) {
> +		dev_err(dev,
> +			"%s: devm_request_irq failed for IRQ %d: %d\n",
> +			__func__, irq, ret);
> +		return ret;
> +	}
> +
> +	ret = devm_device_add_group(dev, &altera_timeout_bridge_sysfs_group);
> +	if (ret)
> +		return ret;
> +
> +	driver_data->irq_node = sysfs_get_dirent(dev->kobj.sd, "timeout_irq");
> +	if (!driver_data->irq_node)
> +		return -ENOENT;
> +
> +	ret = devm_add_action_or_reset(dev, (void (*)(void *))sysfs_put, driver_data->irq_node);

[Severity: High]
Does this mismatched function pointer cast cause a Control-Flow Integrity
(CFI) violation during devres teardown?

The driver registers sysfs_put() with devm_add_action_or_reset() by casting
it to (void (*)(void *)). The actual signature of sysfs_put() expects a
struct kernfs_node *, not void *. When devres executes this action on module
unload or probe failure, KCFI detects the signature mismatch and panics.

> +	if (ret)
> +		return ret;
> +
> +	if (np)
> +		of_platform_populate(np, NULL, lookup, &pdev->dev);
> +
> +	return 0;
> +}

[Severity: Medium]
Are these child devices leaked when the driver is unbound?

During probe, of_platform_populate() creates child platform devices. However,
the platform driver lacks a .remove callback and does not use
devm_of_platform_populate(). The child devices are never destroyed when the
driver is unbound.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261003-feature-timeout-ip-v1-0-8837c311d571@bruker.com?part=2