Re: [PATCH v2 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 5 potential issue(s) to consider:

New issues:
- [High] Temporarily retargeting `dev->of_node` on a registered device causes TOCTOU race

Pre-existing issues:
- [Critical] Error path blindly processes uninitialized ports, causing NULL pointer dereference
- [High] Unprotected concurrent access to global `max310x_lines` causes race condition during probe
- [High] Missing `cancel_work_sync()` in error path leads to Use-After-Free
- [High] Interrupt handler requested after port registration causes unhandled interrupt storm
--

commit 3cfa2d1749d3467560c6e80f4881b8af12a9e51e
Author: Tapio Reijonen <tapio.reijonen@vaisala.com>

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

The MAX310x driver is modified to read rs485 properties from per-channel DT
subnodes for multi-port variants. It walks the child nodes for a matching
"serial@N" node and temporarily retargets the parent device's fwnode so
uart_get_rs485_mode() picks up the subnode properties.

> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index 5cb7d01e40466..aee3b75fff000 100644
> --- 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
[ ... ]
>  	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 code leave max310x_lines open to
a race condition?

The line number is allocated here using find_first_zero_bit(), but set_bit()
is not called until after uart_add_one_port() completes successfully later in
the function.

If two max310x devices probe concurrently, could they observe the same zero
bit, assign the same line, and attempt to register it?

>  		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. */
[ ... ]
> +		/*
> +		 * 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: High]
The commit message states "Probe is serialised, so the swap is safe."
Is this swap safe against lockless readers in other subsystems?

Since the parent device is already registered and globally visible to the
device model, could a concurrent lockless lookup via of_find_device_by_node()
or bus_find_device_by_of_node() observe the temporarily swapped dev->of_node?

If so, this might cause a concurrent lookup for the parent device's phandle
to spuriously fail, or a lookup for the child node to erroneously return the
parent device.

Would it be better to read subnode properties by explicitly passing the target
fwnode rather than temporarily replacing the parent device's identity?

> +			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;

[Severity: Critical]
This isn't a bug introduced by this patch, but does the out_uart cleanup
path handle partially initialized devices correctly if we goto out_uart here?

The out_uart cleanup loop unconditionally iterates over all devtype->nr ports.
For ports that haven't been initialized yet, s->p[i].port.line evaluates to 0.
Could test_and_clear_bit(0, max310x_lines) return true if line 0 is currently
in use by another instance?

If it does, this would call uart_remove_one_port() on an unregistered port.
Could this lead to a NULL pointer dereference in serial_core_unregister_port()
since port->port_dev would be NULL?

[Severity: High]
This is also a pre-existing issue, but is it safe to let devres free the
device context without canceling pending work if we take this goto?

The driver initializes md_work, tx_work, and rs_work for each port. During
uart_add_one_port(), the serial core synchronously invokes callbacks like
set_mctrl() and rs485_config(), which can schedule these work items.

If an error occurs and the ports are removed in out_uart, there is no call to
cancel_work_sync(). Could asynchronously executing work items dereference the
freed memory?

> +		} 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 isn't a bug introduced by this patch, but could this registration order
cause an unhandled interrupt storm?

Calling uart_add_one_port() exposes the TTY device to userspace. If a
userspace program opens it immediately, max310x_startup() can enable TX/RX
interrupts on the device.

Because devm_request_threaded_irq() is called outside and after this loop,
there is a window where the device can generate interrupts without a
registered handler.

Since the interrupt line is shared, would the kernel forcefully disable the
IRQ line if an interrupt fires during this window?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601-b4-max310x-rs485-dt-v2-0-a105105f8e70@vaisala.com?part=3