Re: [PATCH v2 1/3] serial: max310x: register GPIO controller before adding UART ports

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `out_uart` error path erroneously unregisters other devices' UART ports and causes a NULL pointer dereference due to zero-initialized `port.line` values.
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in UART line allocation allows concurrent probes to acquire the same line, leading to a NULL pointer dereference in the error path.
- [High] Pending workqueues are not cancelled on the probe error path, leading to use-after-free when the device structure is freed.
- [High] The UART port is exposed to userspace before its power state is finalized and before its IRQ handler is registered.
--

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

serial: max310x: register GPIO controller before adding UART ports

The MAX310x exposes four GPIOs per UART port via an in-driver gpio_chip.
This patch splits the per-port setup into two passes around the gpio_chip
registration to prevent -EPROBE_DEFER issues during port registration.

> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index ac7d3f197c3a5..5cb7d01e40466 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c

[ ... ]

> @@ -1434,6 +1425,27 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
>  		goto out_uart;
>  #endif
>  
> +	for (i = 0; i < devtype->nr; i++) {
> +		unsigned int line;
> +
> +		line = find_first_zero_bit(max310x_lines, MAX310X_UART_NRMAX);
> +		if (line == MAX310X_UART_NRMAX) {
> +			ret = -ERANGE;
> +			goto out_uart;

[Severity: High]
This is a pre-existing issue, but does this error path cause a NULL pointer
dereference?

Because devm_kzalloc() zeroes s, s->p[i].port.line is initialized to 0 for
all ports. If we jump to out_uart here (or if devm_gpiochip_add_data()
failed earlier), the cleanup loop unconditionally tests bit 0 for the
uninitialized ports:

max310x_probe() {
    ...
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);
    }
    ...
}

Could this clear the line 0 bit correctly owned by another device, and
attempt to unregister it using this failing device's partially-initialized
uport?

> +		}
> +		s->p[i].port.line = line;
> +
> +		/* Register port */
> +		ret = uart_add_one_port(&max310x_uart, &s->p[i].port);
> +		if (ret)
> +			goto out_uart;

[Severity: High]
This is a pre-existing issue, but does this error path leak pending workqueues?

During uart_add_one_port(), the serial core can invoke callbacks like
rs485_config() or set_mctrl(), which synchronously schedule rs_work or md_work.

If a subsequent port registration fails and we jump to out_uart, the ports
are unregistered but cancel_work_sync() is never called for these workqueues.
When devres subsequently frees the device memory (s), could a delayed execution
of the scheduled work dereference the freed memory leading to a use-after-free?

> +
> +		set_bit(line, max310x_lines);

[Severity: High]
This is a pre-existing issue, but does this create a Time-of-Check to
Time-of-Use (TOCTOU) race condition during line allocation?

The find_first_zero_bit() function is called before uart_add_one_port(),
but the line isn't actually reserved with set_bit() until after
uart_add_one_port() returns.

Could concurrent probes of multiple max310x devices find the same free line,
leading to the second one failing uart_add_one_port() with -EINVAL,
jumping to out_uart, and erroneously clearing the bit correctly set by the
first device?

> +
> +		/* Go to suspend mode */
> +		max310x_power(&s->p[i].port, 0);
> +	}
> +
>  	/* Setup interrupt */
>  	ret = devm_request_threaded_irq(dev, irq, NULL, max310x_ist,
>  					IRQF_ONESHOT | IRQF_SHARED, dev_name(dev), s);

[Severity: High]
This is a pre-existing issue, but does exposing the port before finalized
setup cause missed interrupts or clobbered power states?

Because uart_add_one_port() makes the port visible to userspace, an
immediate open of the port will invoke the .startup callback and turn the
device power on.

However, the probe function immediately calls max310x_power(..., 0), which
would force the active session back into suspend mode. Furthermore, since
devm_request_threaded_irq() happens after all ports are added, could early
userspace activity trigger interrupts that are completely missed?

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