Re: [PATCH 3/3] input: joystick: stinger: port Gravis Stinger driver to safe Rust

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential
issues to consider:
- [High] Input: stinger - built-in Rust input/serio fails link when
  INPUT/SERIO=m/n
- [Medium] Input: stinger - depends on RUST drops non-Rust support and
  breaks defconfig
- [Medium] Input: stinger - missing MODULE_DEVICE_TABLE breaks module
  autoloading
- [Medium] Input: stinger - missing input_dev parent in connect() breaks
  sysfs hierarchy
- [Medium] Input: stinger - connect() registers input_dev before
  serio_open()
- [Medium] Input: stinger - driver registration uses helper THIS_MODULE
  instead of owner
- [Medium] rust: input: unsound set_phys() and set_abs_params() on
  AllocatedDevice
- [Medium] rust: serio: missing sentinel guarantee on Driver::ID_TABLE
  slice
- [Medium] rust: serio: missing Opaque wrapper for serio_driver in
  Registration
- [Low] rust: serio: negated to_errno() in
  Registration::connect_callback()
--

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

input: joystick: stinger: port Gravis Stinger driver to safe Rust

This commit replaces the C Gravis Stinger serial gamepad driver
(drivers/input/joystick/stinger.c) with a Rust implementation
(drivers/input/joystick/stinger.rs) and makes CONFIG_JOYSTICK_STINGER
depend on CONFIG_RUST.

> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> index 7755e5b454d2c..e672a3bc6dccf 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -188,6 +188,7 @@ config JOYSTICK_SPACEBALL
>  	  module will be called spaceball.
>  
>  config JOYSTICK_STINGER
> +	depends on RUST
>  	tristate "Gravis Stinger gamepad"
>  	select SERIO
>  	help

[Severity: Medium]
Does adding depends on RUST directly to JOYSTICK_STINGER while deleting
drivers/input/joystick/stinger.c cause a hardware support regression on
non-Rust configurations and architectures without HAVE_RUST?

For example, arch/powerpc/configs/ppc6xx_defconfig enables
CONFIG_JOYSTICK_STINGER=m without CONFIG_RUST, so the driver is silently
dropped from that defconfig. Documentation/input/joydev/joystick.rst also
still states that the device is supported by the stinger.c module.

[Severity: High]
Can enabling CONFIG_RUST=y with CONFIG_INPUT=m or CONFIG_SERIO=m (such as
CONFIG_JOYSTICK_STINGER=m, which only selects SERIO under if INPUT) break
the vmlinux link?

CONFIG_RUST is a bool option, so rust/kernel.o and rust/helpers/helpers.o
are always linked into vmlinux, and both unconditionally compile the input
and serio abstractions and C helpers without guarding on CONFIG_INPUT=y or
CONFIG_SERIO=y. When CONFIG_INPUT or CONFIG_SERIO is modular or disabled,
built-in code in rust/kernel/input.rs and rust/helpers/{input,serio}.c
references input_allocate_device(), input_register_device(), input_event(),
__serio_register_driver(), and serio_unregister_driver(), which are either
in loadable modules or not built at all.

[ ... ]

> diff --git a/drivers/input/joystick/stinger.rs b/drivers/input/joystick/stinger.rs
> new file mode 100644
> index 0000000000000..daa1994636315
> --- /dev/null
> +++ b/drivers/input/joystick/stinger.rs

[ ... ]

> +impl serio::Driver for StingerDriver {
> +    type Data = KBox<Stinger>;
> +
> +    const ID_TABLE: &'static [serio::DeviceId] = &[
> +        serio::DeviceId::new(
> +            bindings::SERIO_RS232 as u8,
> +            bindings::SERIO_ANY as u8,
> +            bindings::SERIO_ANY as u8,
> +            bindings::SERIO_STINGER as u8,
> +        ),
> +        serio::DeviceId::new(0, 0, 0, 0),
> +    ];

[Severity: Medium]
Does dropping MODULE_DEVICE_TABLE(serio, stinger_serio_ids) without a
replacement module_device_table! or module alias entry break automatic
module loading when CONFIG_JOYSTICK_STINGER=m?

Without the exported device table symbol, modpost no longer emits
MODULE_ALIAS("serio:ty02pr1Eid*ex*") into stinger.ko, so udev/kmod cannot
autoload the module when serio_uevent() reports a matching serio port.

[Severity: Medium]
Can a safe implementation of serio::Driver omit the trailing zero entry in
ID_TABLE and cause an out-of-bounds read in serio_match_port()?

In rust/kernel/serio.rs, Driver::ID_TABLE is typed as a plain slice
&'static [DeviceId] rather than a compile-time zero-terminated array, and
Registration::new() passes T::ID_TABLE.as_ptr() directly to the C serio
core:

rust/kernel/serio.rs:Registration::new() {
    let drv = bindings::serio_driver {
        description: name.as_char_ptr().cast(),
        id_table: T::ID_TABLE.as_ptr().cast::<bindings::serio_device_id>(),
}

Because serio_match_port() walks ids++ until both ids->type and ids->proto
are zero, any safe serio::Driver that omits the sentinel entry triggers an
out-of-bounds read past the slice.

> +
> +    fn connect(dev: &mut serio::Device) -> Result<Self::Data> {
> +        let mut input_alloc = AllocatedDevice::new()?;
> +
> +        input_alloc.set_name(DRIVER_DESC);
> +        input_alloc.set_phys(dev.phys().as_ptr());
> +        input_alloc.set_id(bus::RS232, bindings::SERIO_STINGER as u16, 0x0001, 0x0100);

[Severity: Medium]
Does StingerDriver::connect() register the input device with a NULL parent
instead of &serio->dev?

The C driver set input_dev->dev.parent = &serio->dev before calling
input_register_device(), whereas AllocatedDevice::new() wraps
input_allocate_device() without setting dev.parent. Registering with a NULL
parent places the device under /sys/devices/virtual/input/ instead of under
the serio port in sysfs, which breaks udev parent matching (such as ID_PATH
and seat assignment) and suspend/resume ordering.

[Severity: Medium]
Are AllocatedDevice::set_phys() and AllocatedDevice::set_abs_params() sound
as safe functions in rust/kernel/input.rs?

rust/kernel/input.rs:AllocatedDevice {
    pub fn set_phys(&mut self, phys: *const u8) {
        // SAFETY: `self.ptr` is valid.
        unsafe {
            (*self.ptr.as_ptr()).phys = phys.cast();
        }
    }
    ...
    pub fn set_abs_params(&mut self, axis: u32, min: i32, max: i32, fuzz: i32, flat: i32) {
        // SAFETY: `self.ptr` is valid.
        unsafe {
            bindings::input_set_abs_params(self.ptr.as_ptr(), axis, min, max, fuzz, flat);
        }
    }
}

set_phys() accepts an arbitrary raw pointer from safe code without any
validity, lifetime, or NUL-termination guarantee, even though the input core
dereferences dev->phys as a C string in input_dev_uevent(), sysfs, procfs,
and EVIOCGPHYS for the lifetime of the device. set_abs_params() passes an
unvalidated axis index to input_set_abs_params(), which executes
__set_bit(axis, dev->absbit) and indexes dev->absinfo[axis] without checking
that axis < ABS_CNT, allowing out-of-bounds writes from safe Rust.

> +
> +        // Configure supported buttons
> +        for btn in [
> +            bindings::BTN_A,
> +            bindings::BTN_B,
> +            bindings::BTN_C,
> +            bindings::BTN_X,
> +            bindings::BTN_Y,
> +            bindings::BTN_Z,
> +            bindings::BTN_TL,
> +            bindings::BTN_TR,
> +            bindings::BTN_START,
> +            bindings::BTN_SELECT,
> +        ] {
> +            input_alloc.set_capability(event_type::KEY, btn);
> +        }
> +
> +        // Configure 2-axis joystick with flat margin of 4
> +        input_alloc.set_capability(event_type::ABS, bindings::ABS_X);
> +        input_alloc.set_capability(event_type::ABS, bindings::ABS_Y);
> +        input_alloc.set_abs_params(bindings::ABS_X, -64, 64, 0, 4);
> +        input_alloc.set_abs_params(bindings::ABS_Y, -64, 64, 0, 4);
> +
> +        let registered = input_alloc.register()?;
> +
> +        let stinger = KBox::new(
> +            Stinger {
> +                input: registered,
> +                idx: 0,
> +                data: [0; STINGER_MAX_LENGTH],
> +            },
> +            GFP_KERNEL,
> +        )?;
> +
> +        Ok(stinger)
> +    }

[Severity: Medium]
Does StingerDriver::connect() publish the input device before the serio
port is opened?

In the C driver, stinger_connect() set the serio drvdata and called
serio_open() before calling input_register_device(). Here,
input_alloc.register() exposes the device to the input core and userspace
before KBox::new() allocates Stinger and before
Registration::connect_callback() calls serio_set_drvdata() and
serio_open():

rust/kernel/serio.rs:Registration::connect_callback() {
    let data = match T::connect(&mut dev) {
        Ok(d) => d,
        Err(e) => return e.to_errno(),
    };

    let raw_data = data.into_foreign();
    unsafe {
        bindings::serio_set_drvdata(serio, raw_data.cast());
    }
    let ret = unsafe { bindings::serio_open(serio, drv) };
}

Userspace can therefore open the input node before the underlying serio port
is opened, and a failure in KBox::new() or serio_open() registers and
immediately unregisters the input device, emitting spurious uevents.

[Severity: Low]
In Registration::connect_callback() in rust/kernel/serio.rs, does the null
check return a positive value instead of a negative errno?

rust/kernel/serio.rs:Registration::connect_callback() {
    let Some(serio_nn) = NonNull::new(serio) else {
        return -(crate::error::code::EINVAL.to_errno());
    };
}

Error::to_errno() already returns a negative errno (-22), so negating it
returns +22 instead of -EINVAL.

[ ... ]

> +struct StingerModule {
> +    _reg: serio::Registration<StingerDriver>,
> +}

[Severity: Medium]
Does dropping serio::Registration<T> violate Rust aliasing rules on
bindings::serio_driver?

rust/kernel/serio.rs:Registration {
    drv: Pin<KBox<bindings::serio_driver>>,
    _p: PhantomData<T>,
}

rust/kernel/serio.rs:Registration::drop() {
    unsafe {
        bindings::serio_unregister_driver(self.drv.as_mut().get_mut());
    }
}

Once serio_register_driver() publishes the driver, the C serio core and
sysfs attributes (such as bind_mode_store(), which writes
serio_drv->manual_bind without locking) hold pointers to the struct and may
read or mutate it concurrently. Storing bindings::serio_driver without
kernel::types::Opaque and calling self.drv.as_mut().get_mut() in drop()
creates an exclusive &mut reference while C still has active aliases.

> +
> +impl kernel::Module for StingerModule {
> +    fn init(_module: &'static ThisModule) -> Result<Self> {
> +        let reg = serio::Registration::<StingerDriver>::new(DRIVER_NAME)?;
> +        pr_info!("Gravis Stinger safe Rust driver initialized\n");
> +        Ok(Self { _reg: reg })
> +    }
> +}

[Severity: Medium]
Does serio::Registration::new() register the driver with the helper
translation unit's THIS_MODULE and KBUILD_MODNAME instead of the driver
module's owner and name?

StingerModule::init() ignores _module, and Registration::new() calls
rust_helper_serio_register_driver():

rust/helpers/serio.c:rust_helper_serio_register_driver() {
	return __serio_register_driver(drv, THIS_MODULE, KBUILD_MODNAME);
}

Because rust/helpers/serio.c is compiled into the built-in helpers object
rather than into stinger.ko, THIS_MODULE is NULL and KBUILD_MODNAME is
"helpers". When CONFIG_JOYSTICK_STINGER=m, drv->driver.owner is NULL and
drv->driver.mod_name is "helpers", so the driver core cannot pin the module
reference count and /sys/bus/serio/drivers/stinger/module linkage is
broken. Registration::new() should accept &'static ThisModule and pass
module.as_ptr() and module name to __serio_register_driver().

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