Re: [PATCH 2/3] rust: serio: add serio bus abstractions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential
issues to consider:
- [High] rust: serio: rust_helper_serio_register_driver() uses wrong
  THIS_MODULE
- [High] rust: serio: Driver::ID_TABLE slice lacks sentinel causing OOB
  read
- [Medium] rust: serio: unguarded serio and input build fails when
  CONFIG_SERIO=m/n
- [Medium] rust: serio: connect_callback() negates negative errno and
  opens port late
- [Medium] rust: serio: Registration stores serio_driver without Opaque
  causing UB
- [Low] rust: serio: IrqReturn duplicates kernel::irq::IrqReturn with
  wrong paths
- [Low] rust: serio: Device and Registration bypass generic driver
  infrastructure
- [Low] rust: serio: missing inline attributes, non-vertical imports,
  and core::ffi
--

commit a79cbd7ccd6c2d027f39fc787dc865ce37a52295
Author: clicedvly768 <harepro29@gmail.com>

rust: serio: add serio bus abstractions

This commit adds Rust abstractions and C helper wrappers for the serio
bus subsystem, including DeviceId, Device, IrqReturn, Driver, and
Registration.

> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 7a11f5fa461d2..84ffdb015b6d9 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -66,6 +66,7 @@
>  #include "fs.c"
>  #include "fwctl.c"
>  #include "gpu.c"
> +#include "input.c"
>  #include "interrupt.c"
>  #include "io.c"
>  #include "irq.c"
> @@ -92,6 +93,7 @@
>  #include "scatterlist.c"
>  #include "security.c"
>  #include "serdev.c"
> +#include "serio.c"
>  #include "signal.c"
>  #include "slab.c"
>  #include "spinlock.c"

[ ... ]

> diff --git a/rust/helpers/serio.c b/rust/helpers/serio.c
> new file mode 100644
> index 0000000000000..cd061495d9b1d
> --- /dev/null
> +++ b/rust/helpers/serio.c
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/serio.h>
> +
> +__rust_helper void *rust_helper_serio_get_drvdata(struct serio *serio)
> +{
> +	return serio_get_drvdata(serio);
> +}
> +
> +__rust_helper void rust_helper_serio_set_drvdata(struct serio *serio, void *data)
> +{
> +	serio_set_drvdata(serio, data);
> +}
> +
> +__rust_helper int rust_helper_serio_register_driver(struct serio_driver *drv)
> +{
> +	return __serio_register_driver(drv, THIS_MODULE, KBUILD_MODNAME);
> +}

[Severity: High]
Does evaluating THIS_MODULE and KBUILD_MODNAME inside
rust_helper_serio_register_driver() leave drv->driver.owner as NULL for
loadable Rust serio drivers?

Because rust/helpers/serio.c is compiled into rust/helpers/helpers.o rather
than the registering driver module, THIS_MODULE expands to NULL (when
helpers.o is built-in) and KBUILD_MODNAME expands to "helpers".  When
__serio_register_driver() sets drv->driver.owner = owner, neither the
driver core nor serio_queue_event() (via try_module_get(drv->driver.owner))
can pin the driver module while ports are bound or kseriod events are
queued, allowing the module to be unloaded while still in use and causing a
use-after-free.

Since __serio_register_driver() and serio_unregister_driver() are already
exported non-inline C functions, these two helpers can be dropped and
Registration::new() can take a &'static ThisModule and pass
module.as_ptr() and name.as_char_ptr() directly to
bindings::__serio_register_driver().

> +
> +__rust_helper void rust_helper_serio_unregister_driver(struct serio_driver *drv)
> +{
> +	serio_unregister_driver(drv);
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 559fc79b1e820..c9f82dce8f823 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -86,6 +86,7 @@
>  #[doc(hidden)]
>  pub mod impl_flags;
>  pub mod init;
> +pub mod input;
>  pub mod interop;
>  pub mod interrupt;
>  pub mod io;
> @@ -128,6 +129,7 @@
>  pub mod seq_file;
>  #[cfg(CONFIG_RUST_SERIAL_DEV_BUS_ABSTRACTIONS)]
>  pub mod serdev;
> +pub mod serio;
>  pub mod sizes;
>  #[cfg(CONFIG_SOC_BUS)]
>  pub mod soc;

[Severity: Medium]
Does building with CONFIG_SERIO=m or CONFIG_SERIO=n (or CONFIG_INPUT=m/n)
fail to link vmlinux?

CONFIG_SERIO and CONFIG_INPUT are tristate Kconfig options, and
include/linux/serio.h and include/linux/input.h declare
__serio_register_driver(), serio_unregister_driver(), serio_open(),
serio_close(), and the input_*() functions without inline stubs when
disabled.  Because rust/kernel/lib.rs and rust/helpers/helpers.c are always
built into vmlinux when CONFIG_RUST=y, compiling rust/helpers/serio.c,
rust/helpers/input.c, kernel::serio, and kernel::input without Kconfig
guards causes undefined symbol references at link time whenever
CONFIG_SERIO or CONFIG_INPUT is n or m.

[ ... ]

> diff --git a/rust/kernel/serio.rs b/rust/kernel/serio.rs
> new file mode 100644
> index 0000000000000..3b752ce4b64de
> --- /dev/null
> +++ b/rust/kernel/serio.rs
> @@ -0,0 +1,223 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Serio (Serial I/O) bus abstractions for safe Rust drivers.
> +//!
> +//! Provides a safe Rust interface for serio devices and drivers in the Linux kernel,
> +//! supporting input devices such as serial gamepads (e.g. Gravis Stinger), mice, and keyboards.
> +
> +use crate::{
> +    bindings,
> +    error::{to_result, Result},
> +    prelude::*,
> +    types::ForeignOwnable,
> +};
> +use core::{marker::PhantomData, pin::Pin, ptr::NonNull};
> +
> +/// Identifies supported serio hardware devices for auto-probing and matching.
> +#[repr(transparent)]
> +#[derive(Copy, Clone)]
> +pub struct DeviceId(bindings::serio_device_id);
> +
> +impl DeviceId {
> +    /// Creates a new serio device identifier.
> +    pub const fn new(type_: u8, extra: u8, id: u8, proto: u8) -> Self {
> +        Self(bindings::serio_device_id {
> +            type_,
> +            extra,
> +            id,
> +            proto,
> +        })
> +    }
> +}

[Severity: Low]
This isn't a bug, but there are a few style and convention issues in this
file:

- The imports at the top use horizontal grouping instead of the kernel
  vertical import style.
- Small functions in the kernel crate (DeviceId::new(), IrqReturn::to_c(),
  Device::as_raw(), and Device::phys()) are missing #[inline] attributes.
- connect_callback() and interrupt_callback() use core::ffi::{c_int,
  c_uchar, c_uint} instead of the crate::ffi / prelude C integer types.

> +
> +/// Status returned by a serio driver interrupt handler.
> +#[derive(Copy, Clone, PartialEq, Eq)]
> +pub enum IrqReturn {
> +    /// Interrupt was not handled by this driver.
> +    None,
> +    /// Interrupt was successfully handled.
> +    Handled,
> +}
> +
> +impl IrqReturn {
> +    fn to_c(self) -> bindings::irqreturn_t {
> +        match self {
> +            Self::None => bindings::irqreturn::IRQ_NONE,
> +            Self::Handled => bindings::irqreturn::IRQ_HANDLED,
> +        }
> +    }
> +}

[Severity: Low]
Can this reuse kernel::irq::IrqReturn instead of defining a second
IrqReturn enum?  Also, bindgen generates bindings::irqreturn_IRQ_NONE and
bindings::irqreturn_IRQ_HANDLED rather than bindings::irqreturn::IRQ_*.

> +
> +/// A serio port device instance passed to driver callbacks.
> +pub struct Device {
> +    ptr: NonNull<bindings::serio>,
> +}

[Severity: Low]
Should Device and Registration integrate with the generic device and
driver abstractions in kernel::device and kernel::driver?

Wrapping Opaque<bindings::serio> with a device::DeviceContext parameter and
implementing AsRef<device::Device<Ctx>> (and using driver::RegistrationOps
/ driver::DriverLayout for registration) would allow serio drivers to
obtain a &device::Device for setting parent devices (such as
input_dev->dev.parent = &serio->dev), logging via dev_info! / dev_err!, and
using devres.

[ ... ]

> +/// Trait implemented by serio device drivers.
> +pub trait Driver: Send + Sync + Sized {
> +    /// Context data stored per connected device instance.
> +    type Data: ForeignOwnable + Send + Sync;
> +
> +    /// Device ID match table.
> +    const ID_TABLE: &'static [DeviceId];

[Severity: High]
Can passing a plain &'static [DeviceId] slice to C as drv->id_table cause
an out-of-bounds read in serio_match_port()?

drivers/input/serio/serio.c:serio_match_port() walks the table until it
reaches a zeroed sentinel entry:

drivers/input/serio/serio.c:serio_match_port() {
	while (ids->type || ids->proto) {
		if ((ids->type == SERIO_ANY || ids->type == serio->id.type) &&
...
		ids++;
	}
}

Because Driver is a safe trait and &[DeviceId] does not enforce a trailing
zero sentinel (and an empty slice passes a dangling pointer from
as_ptr()), any safe Driver implementation that does not manually append a
zeroed DeviceId entry will cause serio_match_port() to read past the end of
ID_TABLE when probing a non-matching port.  DeviceId should implement
kernel::device_id::RawDeviceId and use kernel::device_id::IdTable /
IdArray so the zero terminator is guaranteed at compile time.

[ ... ]

> +    /// # Safety
> +    ///
> +    /// Must only be invoked by the serio subsystem when a matching port is probed.
> +    /// `serio` and `drv` must be valid non-null pointers provided by the kernel.
> +    unsafe extern "C" fn connect_callback(
> +        serio: *mut bindings::serio,
> +        drv: *mut bindings::serio_driver,
> +    ) -> core::ffi::c_int {
> +        let Some(serio_nn) = NonNull::new(serio) else {
> +            return -(crate::error::code::EINVAL.to_errno());
> +        };
> +
> +        let mut dev = Device { ptr: serio_nn };
> +
> +        let data = match T::connect(&mut dev) {
> +            Ok(d) => d,
> +            Err(e) => return e.to_errno(),
> +        };
> +
> +        let raw_data = data.into_foreign();
> +        // SAFETY: `serio` is valid; store driver data pointer.
> +        unsafe {
> +            bindings::serio_set_drvdata(serio, raw_data.cast());
> +        }
> +
> +        // SAFETY: Open serio port to start receiving characters.
> +        let ret = unsafe { bindings::serio_open(serio, drv) };

[Severity: Medium]
Does connect_callback() return a positive errno on NULL and invoke
T::connect() before the port is opened?

Error::to_errno() already returns a negative errno (-22 for EINVAL), so
-(crate::error::code::EINVAL.to_errno()) returns +22 instead of -EINVAL.

In addition, calling T::connect() before serio_set_drvdata() and
serio_open() means the serio port is not open during T::connect(), and a
driver that registers an input device inside T::connect() exposes the input
device to userspace before serio_open() has succeeded.

[ ... ]

> +impl<T: Driver> Drop for Registration<T> {
> +    fn drop(&mut self) {
> +        // SAFETY: Unregister driver with kernel.
> +        unsafe {
> +            bindings::serio_unregister_driver(self.drv.as_mut().get_mut());
> +        }
> +    }
> +}

[Severity: Medium]
Does self.drv.as_mut().get_mut() violate Rust's aliasing rules while the
serio driver is still registered with the kernel?

Registration<T> stores bindings::serio_driver directly in
Pin<KBox<bindings::serio_driver>> without wrapping it in
kernel::types::Opaque.  While the driver is registered, C code in the
serio core holds pointers to struct serio_driver and accesses or mutates
its fields (such as drv->manual_bind in bind_mode_store() and
serio_unregister_driver()), so creating an exclusive &mut
bindings::serio_driver before serio_unregister_driver() completes is
undefined behavior.  Wrapping bindings::serio_driver in Opaque avoids
forming an exclusive reference to C-owned memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106191714.334055-1-harepro29@gmail.com?part=2