Re: [PATCH 1/7] serial: max310x: Look up all channels before adding any port

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] serial: max310x: null pointer dereference in max310x_rs_proc()
  [1]
--

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

serial: max310x: Look up all channels before adding any port

This commit splits the per-channel loop in max310x_probe() so that RS485
device tree properties and GPIOs are looked up for all channels before
registering any UART ports with uart_add_one_port().

> diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
> index a0ed70185555e..bdfdf4a9000bb 100644
> --- a/drivers/tty/serial/max310x.c
> +++ b/drivers/tty/serial/max310x.c
[ ... ]
> @@ -2050,12 +2043,23 @@ static int max310x_probe(struct device *dev, const struct max310x_devtype *devty
>  			device_set_node(dev, saved_fwnode);
>  			of_node_put(port_np);
>  			if (ret)
> -				goto out_uart;
> +				goto out_clk;
>  		} else if (devtype->nr == 1) {
>  			ret = uart_get_rs485_mode(&s->p[i].port);
>  			if (ret)
> -				goto out_uart;
> +				goto out_clk;
>  		}
> +	}
> +
> +	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;
> +		}
> +		s->p[i].port.line = line;
>  
>  		/* Register port */
>  		ret = uart_add_one_port(&max310x_uart, &s->p[i].port);

[Severity: High]
This problem wasn't introduced by this patch, but can jumping to out_uart
after uart_add_one_port() cause a NULL pointer dereference in
max310x_rs_proc() [1]?

When RS485 is enabled in the device tree, uart_add_one_port() calls
max310x_rs485_config(), which schedules s->p[i].rs_work.  If a later step in
max310x_probe() fails (such as registering a subsequent port or requesting the
IRQ), the out_uart cleanup loop calls uart_remove_one_port() without
cancelling rs_work first:

drivers/tty/serial/max310x.c: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);
	}
    ...
}

uart_remove_one_port() clears port.state to NULL, so when max310x_rs_proc()
runs, it dereferences NULL at &one->port.state->port.mutex (and before devres
frees s, or into freed memory afterward).  Should out_uart cancel rs_work
before calling uart_remove_one_port(), as max310x_remove() does?

[1] https://sashiko.dev/#/bug/linux-0992a296-a8ff-4396-990f-c73ea073185b

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-serial-transceiver-supply-v1-0-40bad0f912f2@vaisala.com?part=1