Re: [PATCH RFC 06/12] rust: serial: Add minimal serial-core polling console support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] rust: serial: use-after-free of uart_state on Driver::drop() with open tty
- [Medium] rust: serial: xmit_buf memory leak on TTY open when startup() fails
- [Medium] rust: serial: console_setup() and configure() corrupt console->ispeed
- [Low] rust: serial: missing INVARIANT comment in PortData::with_port()
- [Low] rust: serial: missing #[inline] annotations in serial abstractions
--

commit b586ab9682dee028e1eb2b244ed7a9b0fb3bdd94
Author: Kwanghoon Son <asdfkwang@gmail.com>

rust: serial: Add minimal serial-core polling console support

This commit adds serial-core abstractions in Rust for memory-mapped, two-wire
polling console drivers, including Driver, SerialOps, Port, LockedPort, and
Termios. It manages port lifetime via devres and provides callback adapters
for console output and termios configuration.

> diff --git a/rust/kernel/serial.rs b/rust/kernel/serial.rs
> new file mode 100644
> index 0000000000000..f24c685d8592f
> --- /dev/null
> +++ b/rust/kernel/serial.rs
> @@ -0,0 +1,505 @@

[ ... ]

> +#[pinned_drop]
> +impl<T: SerialOps> PinnedDrop for Driver<T> {
> +    fn drop(self: Pin<&mut Self>) {
> +        // SAFETY: Registration succeeded and every port holds an Arc to this driver.
> +        unsafe { bindings::uart_unregister_driver(self.inner.get()) };
> +    }
> +}

[Severity: Critical]
Can dropping Driver<T> after port removal cause a use-after-free of
drv->state when a TTY file descriptor is still open?

Driver::new() registers a struct tty_driver via uart_register_driver() and
sets console.device to uart_console_device, while register_port() publishes
the TTY device via uart_add_one_port(). Although SerialAdapter::startup()
returns -EOPNOTSUPP, uart_port_startup() in drivers/tty/serial/serial_core.c
turns a negative return value into 1 when capable(CAP_SYS_ADMIN) is true:

drivers/tty/serial/serial_core.c:uart_port_startup() {
	...
	retval = uport->ops->startup(uport);
	if (retval == 0) {
		...
	}
	if (retval && capable(CAP_SYS_ADMIN))
		return 1;

	return retval;
}

uart_open() -> tty_port_open() -> uart_port_activate() -> uart_startup()
then converts that positive return value into 0, allowing open() to succeed
with TTY_IO_ERROR set. When console_on_rootfs() opens /dev/console at boot,
or when a privileged userspace process opens /dev/console or the TTY device
node, it obtains an open struct tty_struct with tty->driver_data =
&drv->state[line] and tty->port = &drv->state[line].port. Even for
unprivileged callers, tty_init_dev() attaches tty->port before uart_open()
fails and tty_release() runs.

Because uart_register_driver() leaves tty_driver->owner set to serial_core's
THIS_MODULE rather than drv->owner, open TTY file descriptors do not pin the
driver module or Arc<Driver<T>>. When a device unbinds and Registration::drop()
calls uart_remove_one_port(), serial_core only calls tty_port_tty_vhangup()
(which does not free open tty_struct instances and skips /dev/console
redirected files) and waits on state->refcount (which only tracks transient
uart_port_ref() sections).

When the last Arc<Driver<T>> is then dropped, Driver::drop() calls
uart_unregister_driver(), which immediately frees drv->state:

drivers/tty/serial/serial_core.c:uart_unregister_driver() {
	...
	for (i = 0; i < drv->nr; i++)
		tty_port_destroy(&drv->state[i].port);
	kfree(drv->state);
}

When the open /dev/console or TTY file descriptor is later used or closed in
tty_release() -> uart_close() and release_tty() (which sets
tty->port->itty = NULL and calls tty_buffer_cancel_work(tty->port)), won't
the TTY layer dereference and mutate the freed drv->state memory?

[ ... ]

> +    /// # Safety
> +    /// `ptr` must be a PortData<T> port in a serial-core callback. Registration
> +    /// owns its allocation until uart_remove_one_port returns; removal waits for
> +    /// console_srcu readers and serial-core users before this port is freed.
> +    /// Other device-managed resources may already have been revoked.
> +    unsafe fn with_port<R>(
> +        ptr: *mut bindings::uart_port,
> +        f: impl for<'a> FnOnce(&'a Self, &Port<'a>) -> R,
> +    ) -> R {
> +        // SAFETY: The private_data type is fixed by Driver<T>/register_port<T>.
> +        let state = unsafe { &*(*ptr).private_data.cast::<Self>() };
> +        f(
> +            state,
> +            &Port {
> +                raw: ptr,
> +                device: &state.device,
> +            },
> +        )
> +    }
> +}

[Severity: Low]
Since Port<'a> declares a type invariants section in its documentation,
should this Port construction in PortData::with_port() have an // INVARIANT:
comment explaining why raw and device satisfy the invariants?

[ ... ]

> +    // Required TTY operations remain inactive for this output-only console.
> +    extern "C" fn inactive(_ptr: *mut bindings::uart_port) {}
> +
> +    extern "C" fn startup(_ptr: *mut bindings::uart_port) -> i32 {
> +        EOPNOTSUPP.to_errno()
> +    }

[Severity: Medium]
Does returning -EOPNOTSUPP from SerialAdapter::startup() leak
state->port.xmit_buf whenever the TTY or /dev/console is opened?

When a process opens /dev/console (including console_on_rootfs() during boot)
or the registered TTY device node, uart_open() -> tty_port_open() ->
uart_port_activate() -> uart_startup() -> uart_port_startup() allocates a
page for state->port.xmit_buf before invoking uport->ops->startup(uport):

drivers/tty/serial/serial_core.c:uart_port_startup() {
	...
	retval = uart_alloc_xmit_buf(&state->port);
	if (retval)
		return retval;

	retval = uport->ops->startup(uport);
	...
}

Because SerialAdapter::startup() returns -EOPNOTSUPP, uart_port_startup()
returns a non-zero value without freeing state->port.xmit_buf. In
tty_port_open(), a non-zero return from port->ops->activate() causes an early
return before tty_port_set_initialized(port, true) is called:

drivers/tty/tty_port.c:tty_port_open() {
	...
	if (!tty_port_initialized(port)) {
		clear_bit(TTY_IO_ERROR, &tty->flags);
		retval = port->ops->activate(port, tty);
		if (retval) {
			mutex_unlock(&port->mutex);
			return retval;
		}
		tty_port_set_initialized(port, true);
	}
	...
}

Because tty_port_initialized(port) remains false, neither
tty_port_shutdown_locked() on close nor serial_core_remove_one_port() on port
removal calls uart_shutdown() to free state->port.xmit_buf:

drivers/tty/serial/serial_core.c:serial_core_remove_one_port() {
	...
	scoped_guard(mutex, &port->mutex) {
		port->console = false;

		if (tty_port_initialized(port))
			uart_shutdown(NULL, state);
	}
	...
}

And when Driver::drop() later calls uart_unregister_driver(), drv->state is
freed without freeing state->port.xmit_buf.

> diff --git a/rust/kernel/serial/console.rs b/rust/kernel/serial/console.rs
> new file mode 100644
> index 0000000000000..3562191eb7a93
> --- /dev/null
> +++ b/rust/kernel/serial/console.rs
> @@ -0,0 +1,107 @@

[ ... ]

> +    /// # Safety
> +    ///
> +    /// Console core must hold console_list_lock and supply this driver's live
> +    /// console. `options` must be null or a valid NUL-terminated option string.
> +    pub(super) unsafe extern "C" fn console_setup(
> +        console: *mut bindings::console,
> +        options: *mut crate::ffi::c_char,
> +    ) -> i32 {
> +        // SAFETY: Console core holds console_list_lock; serial core has published the port
> +        // before registering this console. uart_set_options calls our termios adapter.
> +        unsafe {
> +            let port = Self::console_port(console);
> +            if port.is_null() {
> +                return ENODEV.to_errno();
> +            }
> +            let mut baud = (*console).ispeed as i32;
> +            let (mut parity, mut bits, mut flow) = (i32::from(b'n'), 8, i32::from(b'n'));
> +            if !options.is_null() {
> +                bindings::uart_parse_options(options, &mut baud, &mut parity, &mut bits, &mut flow);
> +            }
> +            bindings::uart_set_options(port, console, baud, parity, bits, flow)
> +        }
> +    }

[Severity: Medium]
Can ConsoleAdapter::console_setup() and LockedPort::configure() lose
config.default_baud and overwrite console->ispeed and console->ospeed with 0?

First, if non-null console options do not start with a positive baud rate,
uart_parse_options() calls simple_strtoul() and sets baud to 0, overwriting
the initial value read from (*console).ispeed rather than preserving
config.default_baud.

Second, uart_set_options() in drivers/tty/serial/serial_core.c constructs a
zeroed dummy ktermios and calls port->ops->set_termios(port, &termios, &dummy):

drivers/tty/serial/serial_core.c:uart_set_options() {
	struct ktermios termios, dummy;
	...
	memset(&termios, 0, sizeof(struct ktermios));
	memset(&dummy, 0, sizeof(struct ktermios));
	...
	port->ops->set_termios(port, &termios, &dummy);
	...
	co->cflag = termios.c_cflag;
	co->ispeed = termios.c_ispeed;
	co->ospeed = termios.c_ospeed;

	return 0;
}

When a driver's set_termios callback calls LockedPort::baud_rate() with an
out-of-range baud rate (or baud == 0 from uart_parse_options()),
uart_get_baud_rate() falls back to the old termios (&dummy, which has baud
rate 0), encodes 0 into new.0, and clamps its return value to at least 9600.

When the driver then passes that non-zero baud rate to
LockedPort::configure(), the check:

    if bindings::tty_termios_baud_rate(&new.0) != 0 {
        bindings::tty_termios_encode_baud_rate(&mut new.0, baud, baud);
    }

sees tty_termios_baud_rate(&new.0) == 0 and skips encoding the actual baud
rate into new.0. uart_set_options() then copies termios.c_ispeed (0) and
termios.c_ospeed (0) into co->ispeed and co->ospeed, so any subsequent
console_setup() or port resume reads (*console).ispeed == 0 instead of
config.default_baud.

> diff --git a/rust/kernel/serial/termios.rs b/rust/kernel/serial/termios.rs
> new file mode 100644
> index 0000000000000..449662d9b6b5e
> --- /dev/null
> +++ b/rust/kernel/serial/termios.rs
> @@ -0,0 +1,104 @@

[ ... ]

> +    /// Whether parity generation and checking are enabled.
> +    pub fn parity_enabled(&self) -> bool {
> +        self.0.c_cflag & bindings::PARENB != 0
> +    }
> +
> +    /// Whether odd parity, or mark parity in mark/space mode, is selected.
> +    pub fn odd_parity(&self) -> bool {
> +        self.0.c_cflag & bindings::PARODD != 0
> +    }
> +
> +    /// Whether two stop bits are requested.
> +    pub fn two_stop_bits(&self) -> bool {
> +        self.0.c_cflag & bindings::CSTOPB != 0
> +    }
> +
> +    /// Disables mark/space parity without changing parity enable or odd/even selection.
> +    pub fn disable_mark_space_parity(&mut self) {
> +        self.0.c_cflag &= !bindings::CMSPAR;
> +    }
> +
> +    /// Disables RTS/CTS hardware flow control.
> +    pub fn disable_hardware_flow_control(&mut self) {
> +        self.0.c_cflag &= !bindings::CRTSCTS;
> +    }
> +
> +    /// Data bits per character.
> +    pub fn char_size(&self) -> u32 {
> +        // SAFETY: c_cflag is a valid termios control flag value.
> +        unsafe { bindings::tty_get_char_size(self.0.c_cflag).into() }
> +    }
> +}
> +
> +impl LockedPort<'_> {
> +    /// Selects a supported baud rate using serial-core termios rules.
> +    ///
> +    /// Returns `None` if serial core fails to select a rate. Termios changes
> +    /// made by serial core are preserved, including on failure.
> +    pub fn baud_rate(
> +        &self,
> +        new: &mut Termios,
> +        old: Option<&Termios>,
> +        min: u32,
> +        max: u32,
> +    ) -> Option<NonZeroU32> {
> +        // SAFETY: The port is locked, and the termios references are valid for this callback.
> +        let baud = unsafe {
> +            bindings::uart_get_baud_rate(
> +                self.raw,
> +                &mut new.0,
> +                old.map_or(core::ptr::null(), |t| &t.0),
> +                min,
> +                max,
> +            )
> +        };
> +        NonZeroU32::new(baud)
> +    }
> +
> +    /// Updates serial-core timeout and the actual console baud rate.
> +    pub fn configure(&self, new: &mut Termios, baud: NonZeroU32) {
> +        let baud = baud.get();
> +        // SAFETY: All port fields are accessed under the UART lock. new is exclusively borrowed.
> +        unsafe {
> +            bindings::uart_update_timeout(self.raw, new.0.c_cflag, baud);
> +            if bindings::tty_termios_baud_rate(&new.0) != 0 {
> +                bindings::tty_termios_encode_baud_rate(&mut new.0, baud, baud);
> +            }
> +        }
> +    }
> +}

[Severity: Low]
This isn't a bug, but should the small accessor methods and C binding
wrappers in the kernel crate (Termios::from_raw, Termios::from_raw_mut,
Termios::parity_enabled, Termios::odd_parity, Termios::two_stop_bits,
Termios::disable_mark_space_parity, Termios::disable_hardware_flow_control,
Termios::char_size, LockedPort::baud_rate, and LockedPort::configure here, as
well as Port::device and LockedPort::device in rust/kernel/serial.rs) be
annotated with #[inline]?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-rust-bl808-console-v1-0-b736ee920078@gmail.com?part=6