Re: [PATCH v3 3/3] serial: max310x: honour rs485 properties from per-channel DT subnode

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Temporary modification of registered device's `fwnode` creates a data race with sysfs/bus iterators.

Pre-existing issues:
- [Critical] Uninitialized port line values in `out_uart` cleanup path cause deregistration of other devices' serial ports.
- [High] Unprotected global bitmap `max310x_lines` allows concurrent probes to allocate the same UART line, leading to state corruption.
- [High] Missing registration ordering: UART port is exposed to userspace before its interrupt handler is registered.
--

commit 0bd4955b2ec9441be6e7a42bfe7ca700f31859d2
Author: Tapio Reijonen <tapio.reijonen@vaisala.com>

serial: max310x: honour rs485 properties from per-channel DT subnode

This commit updates the MAX310x driver to parse rs485 properties from
per-channel device tree subnodes by temporarily retargeting the parent
device's fwnode. It falls back to the top-level node for single-channel
variants.

> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
[ ... ]
> @@ -1426,6 +1426,9 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
>  #endif
>  
>  	for (i = 0; i < devtype->nr; i++) {
> +		struct fwnode_handle *saved_fwnode = dev_fwnode(dev);
> +		struct device_node *port_np = NULL;
> +		struct device_node *child;
>  		unsigned int line;
>  
>  		line = find_first_zero_bit(max310x_lines, MAX310X_UART_NRMAX);

[Severity: High]
This is a pre-existing issue, but does this lockless read of the global
bitmap allow concurrent probes to allocate the same UART line?

Since find_first_zero_bit() is called here, but set_bit() is only called
much later after port registration and DT parsing, multiple asynchronous
probes could observe the same zero bit. 

Could the second thread fail in uart_add_one_port() due to a collision, jump
to the cleanup path, and erroneously clear the bit that the first thread
successfully registered?

>  		if (line == MAX310X_UART_NRMAX) {
>  			ret = -ERANGE;
>  			goto out_uart;
>  		}
>  		s->p[i].port.line = line;
>  
> +		/* Locate the matching "serial@i" DT subnode, if any. */
> +		for_each_available_child_of_node(dev->of_node, child) {
[ ... ]
> +		/*
> +		 * Temporarily retarget dev's fwnode to the per-port subnode
> +		 * so uart_get_rs485_mode() picks up the per-port properties.
> +		 * For single-port variants, fall back to the chip's own
> +		 * fwnode so legacy DTs that declare rs485 properties at the
> +		 * top level keep working.
> +		 */
> +		if (port_np) {
> +			device_set_node(dev, of_fwnode_handle(port_np));

[Severity: Medium]
Does temporarily swapping the fwnode on a live device create a data race
with sysfs or bus iterators?

Since the parent SPI/I2C device is already registered with the driver core
and visible in sysfs at this stage, locklessly modifying dev->fwnode and
dev->of_node could expose a torn pointer state to concurrent udev events or
subsystem iterators.

> +			ret = uart_get_rs485_mode(&s->p[i].port);
> +			device_set_node(dev, saved_fwnode);
> +			of_node_put(port_np);
> +			if (ret)
> +				goto out_uart;
> +		} else if (devtype->nr == 1) {
> +			ret = uart_get_rs485_mode(&s->p[i].port);
> +			if (ret)
> +				goto out_uart;
> +		}
>  
>  		/* Register port */
>  		ret = uart_add_one_port(&max310x_uart, &s->p[i].port);

[Severity: High]
This is a pre-existing issue, but could this ordering lead to unhandled
interrupts if the port is opened immediately?

Because uart_add_one_port() makes the port available to userspace, udev or a
getty could open it before the probe loop finishes. If the port is opened
and powers on, the hardware might assert the IRQ line before
devm_request_threaded_irq() is called later in max310x_probe(), potentially
causing the IRQ core to permanently disable the shared interrupt line.

>  		if (ret)
>  			goto out_uart;

[ ... ]

>  out_uart:
>  	for (i = 0; i < devtype->nr; i++) {
>  		if (test_and_clear_bit(s->p[i].port.line, max310x_lines))
>  			uart_remove_one_port(&max310x_uart, &s->p[i].port);
>  	}

[Severity: Critical]
This is a pre-existing issue, but can this cleanup path unregister active
ports belonging to completely different devices?

During early probe failure, the allocated port structures are zeroed out,
meaning uninitialized elements have their port.line set to 0. Since this
loop unconditionally iterates over all devtype->nr elements, it will call
test_and_clear_bit() on line 0.

If another max310x instance has already probed and owns line 0, couldn't
this accidentally clear the other device's bit and forcefully unregister its
active port via uart_remove_one_port(), leading to a kernel panic or
use-after-free?

>  out_clk:
>  	clk_disable_unprepare(s->clk);
>  
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-b4-max310x-rs485-dt-v3-0-7e79f064bdd7@vaisala.com?part=3