| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c553saxi6iq67tew5emn6ofghjb6haaosyi2hv5zzz66ol7pup@5fwshcz3jxh5>
Date: Thu, 13 Mar 2025 17:54:57 +0100
From: Benjamin Tissoires <bentiss@...nel.org>
To: Rahul Rameshbabu <sergeantsagara@...tonmail.com>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
linux-input@...r.kernel.org, dri-devel@...ts.freedesktop.org, Jiri Kosina <jikos@...nel.org>,
Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>, Benno Lossin <benno.lossin@...ton.me>,
Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>
Subject: Re: [PATCH RFC 1/3] rust: core abstractions for HID drivers
On Mar 13 2025, Rahul Rameshbabu wrote:
> These abstractions enable the development of HID drivers in Rust by binding
> with the HID core C API. They provide Rust types that map to the
> equivalents in C. In this initial draft, only hid_device and hid_device_id
> are provided direct Rust type equivalents. hid_driver is specially wrapped
> with a custom Driver type. The module_hid_driver! macro provides analogous
> functionality to its C equivalent.
>
> Future work for these abstractions would include more bindings for common
> HID-related types, such as hid_field, hid_report_enum, and hid_report.
Yes, but you can also bypass this as a first step in the same way we do
for HID-BPF: we just consider everything to be a stream of bytes, and
we only care about .report_fixup() and .raw_event().
> Providing Rust equivalents to useful core HID functions will also be
> necessary for HID driver development in Rust.
Yeah, you'll need the back and forth communication with the HID device,
but this can come in later.
>
> Some concerns with this initial draft
> - The need for a DeviceId and DeviceIdShallow type.
> + DeviceIdShallow is used to guarantee the safety requirement for the
> Sync trait.
> - The create_hid_driver call in the module_hid_driver! macro does not use
> Pin semantics for passing the ID_TABLE. I could not get Pin semantics
> to work in a const fn. I get a feeling this might be safe but need help
> reviewing this.
>
> Signed-off-by: Rahul Rameshbabu <sergeantsagara@...tonmail.com>
> ---
> drivers/hid/Kconfig | 8 ++
> rust/bindings/bindings_helper.h | 1 +
> rust/kernel/hid.rs | 245 ++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 2 +
> 4 files changed, 256 insertions(+)
> create mode 100644 rust/kernel/hid.rs
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index b53eb569bd49..e085964c7ffc 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -714,6 +714,14 @@ config HID_MEGAWORLD_FF
> Say Y here if you have a Mega World based game controller and want
> to have force feedback support for it.
>
> +config RUST_HID_ABSTRACTIONS
> + bool "Rust HID abstractions support"
> + depends on RUST
> + depends on HID=y
naive question: does it has to be 'y', some distributions are using 'm'.
> + help
> + Adds support needed for HID drivers written in Rust. It provides a
> + wrapper around the C hid core.
> +
> config HID_REDRAGON
> tristate "Redragon keyboards"
> default !EXPERT
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 55354e4dec14..e2e95afe9f4a 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -16,6 +16,7 @@
> #include <linux/file.h>
> #include <linux/firmware.h>
> #include <linux/fs.h>
> +#include <linux/hid.h>
> #include <linux/jiffies.h>
> #include <linux/jump_label.h>
> #include <linux/mdio.h>
> diff --git a/rust/kernel/hid.rs b/rust/kernel/hid.rs
> new file mode 100644
> index 000000000000..f13476b49e7d
> --- /dev/null
> +++ b/rust/kernel/hid.rs
> @@ -0,0 +1,245 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2025 Rahul Rameshbabu <sergeantsagara@...tonmail.com>
> +
> +use crate::{error::*, prelude::*, types::Opaque};
> +use core::marker::PhantomData;
> +
> +#[repr(transparent)]
> +pub struct Device(Opaque<bindings::hid_device>);
> +
> +impl Device {
> + unsafe fn from_ptr<'a>(ptr: *mut bindings::hid_device) -> &'a mut Self {
> + let ptr = ptr.cast::<Self>();
> +
> + unsafe { &mut *ptr }
> + }
> +
> + pub fn vendor(&self) -> u32 {
> + let hdev = self.0.get();
> +
> + unsafe { (*hdev).vendor }
> + }
> +
> + pub fn product(&self) -> u32 {
> + let hdev = self.0.get();
> +
> + unsafe { (*hdev).product }
> + }
I know this is a RFC, but there are a lot more of interesting fields
you'll want to export.
Can/Should this be automated somehow?
> +}
> +
> +#[repr(transparent)]
> +pub struct DeviceIdShallow(Opaque<bindings::hid_device_id>);
> +
> +// SAFETY: `DeviceIdShallow` doesn't expose any &self method to access internal data, so it's safe to
> +// share `&DriverVTable` across execution context boundaries.
> +unsafe impl Sync for DeviceIdShallow {}
> +
> +impl DeviceIdShallow {
I have a hard time understanding the "DeviceId" part.
In struct hid_device, we have a .id field which is incremented for every
new device. I assume this is different, but this still confuses me.
If that's the rust way of doing it that's fine of course.
[few minutes later] Oh, so you are mapping struct hid_device_id :)
Why dropping the 'HID' in front?
I guess the docs would explain that in the actual submission.
> + pub const fn new() -> Self {
> + DeviceIdShallow(Opaque::new(bindings::hid_device_id {
> + // SAFETY: The rest is zeroed out to initialize `struct hid_device_id`,
> + // sets `Option<&F>` to be `None`.
> + ..unsafe { ::core::mem::MaybeUninit::<bindings::hid_device_id>::zeroed().assume_init() }
> + }))
> + }
> +
> + pub const fn new_usb(vendor: u32, product: u32) -> Self {
We probably need the group here as well.
> + DeviceIdShallow(Opaque::new(bindings::hid_device_id {
> + bus: 0x3, /* BUS_USB */
group???
> + vendor: vendor,
> + product: product,
> + // SAFETY: The rest is zeroed out to initialize `struct hid_device_id`,
> + // sets `Option<&F>` to be `None`.
> + ..unsafe { ::core::mem::MaybeUninit::<bindings::hid_device_id>::zeroed().assume_init() }
> + }))
> + }
> +
> + const unsafe fn as_ptr(&self) -> *const bindings::hid_device_id {
> + self.0.get()
> + }
> +}
> +
> +#[repr(transparent)]
> +pub struct DeviceId(Opaque<bindings::hid_device_id>);
> +
> +impl DeviceId {
> + unsafe fn from_ptr<'a>(ptr: *mut bindings::hid_device_id) -> &'a mut Self {
> + let ptr = ptr.cast::<Self>();
> +
> + unsafe { &mut *ptr }
> + }
> +
> + unsafe fn from_const_ptr<'a>(ptr: *const bindings::hid_device_id) -> &'a Self {
> + let ptr = ptr.cast::<Self>();
> +
> + unsafe { &(*ptr) }
> + }
> +
> + pub fn vendor(&self) -> u32 {
> + let hdev_id = self.0.get();
> +
> + unsafe { (*hdev_id).vendor }
> + }
> +
> + pub fn product(&self) -> u32 {
> + let hdev_id = self.0.get();
> +
> + unsafe { (*hdev_id).product }
> + }
Again, you need the group and the bus at least.
> +}
> +
> +/*
> +#[repr(transparent)]
> +pub struct Field(Opaque<bindings::hid_field>);
> +
> +#[repr(transparent)]
> +pub struct ReportEnum(Opaque<bindings::hid_report_enum>);
> +
> +#[repr(transparent)]
> +pub struct Report(Opaque<bindings::hid_report>);
> +*/
> +
> +#[vtable]
> +pub trait Driver {
> + fn probe(_dev: &mut Device, _id: &DeviceId) -> Result {
> + build_error!(VTABLE_DEFAULT_ERROR)
> + }
> +
> + fn remove(_dev: &mut Device) {
> + }
> +}
> +
> +struct Adapter<T: Driver> {
> + _p: PhantomData<T>,
> +}
> +
> +impl<T: Driver> Adapter<T> {
> + unsafe extern "C" fn probe_callback(
> + hdev: *mut bindings::hid_device,
> + hdev_id: *const bindings::hid_device_id,
> + ) -> crate::ffi::c_int {
> + from_result(|| {
> + let dev = unsafe { Device::from_ptr(hdev) };
> + let dev_id = unsafe { DeviceId::from_const_ptr(hdev_id) };
> + T::probe(dev, dev_id)?;
> + Ok(0)
> + })
> + }
> +
> + unsafe extern "C" fn remove_callback(hdev: *mut bindings::hid_device) {
> + let dev = unsafe { Device::from_ptr(hdev) };
> + T::remove(dev);
> + }
> +}
> +
> +#[repr(transparent)]
> +pub struct DriverVTable(Opaque<bindings::hid_driver>);
> +
> +// SAFETY: `DriverVTable` doesn't expose any &self method to access internal data, so it's safe to
> +// share `&DriverVTable` across execution context boundaries.
> +unsafe impl Sync for DriverVTable {}
> +
> +pub const fn create_hid_driver<T: Driver>(
> + name: &'static CStr,
> + id_table: &'static DeviceIdShallow,
> +) -> DriverVTable {
> + DriverVTable(Opaque::new(bindings::hid_driver {
> + name: name.as_char_ptr().cast_mut(),
> + id_table: unsafe { id_table.as_ptr() },
> + probe: if T::HAS_PROBE {
> + Some(Adapter::<T>::probe_callback)
> + } else {
> + None
> + },
> + remove: if T::HAS_REMOVE {
> + Some(Adapter::<T>::remove_callback)
> + } else {
> + None
> + },
> + // SAFETY: The rest is zeroed out to initialize `struct hid_driver`,
> + // sets `Option<&F>` to be `None`.
> + ..unsafe { core::mem::MaybeUninit::<bindings::hid_driver>::zeroed().assume_init() }
> + }))
> +}
> +
> +pub struct Registration {
> + driver: Pin<&'static mut DriverVTable>,
> +}
> +
> +unsafe impl Send for Registration {}
> +
> +impl Registration {
> + pub fn register(
> + module: &'static crate::ThisModule,
> + driver: Pin<&'static mut DriverVTable>,
> + name: &'static CStr,
> + ) -> Result<Self> {
> + to_result(unsafe {
> + bindings::__hid_register_driver(driver.0.get(), module.0, name.as_char_ptr())
> + })?;
> +
> + Ok(Registration { driver })
> + }
> +}
> +
> +impl Drop for Registration {
> + fn drop(&mut self) {
> + unsafe {
> + bindings::hid_unregister_driver(self.driver.0.get())
> + };
> + }
> +}
> +
> +#[macro_export]
> +macro_rules! usb_device {
> + (vendor: $vendor:expr, product: $product:expr $(,)?) => {
> + $crate::hid::DeviceIdShallow::new_usb($vendor, $product)
> + }
> +}
> +
> +#[macro_export]
> +macro_rules! module_hid_driver {
> + (@replace_expr $_t:tt $sub:expr) => {$sub};
> +
> + (@count_devices $($x:expr),*) => {
> + 0usize $(+ $crate::module_hid_driver!(@replace_expr $x 1usize))*
> + };
> +
> + (driver: $driver:ident, id_table: [$($dev_id:expr),+ $(,)?], name: $name:tt, $($f:tt)*) => {
> + struct Module {
> + _reg: $crate::hid::Registration,
> + }
> +
> + $crate::prelude::module! {
> + type: Module,
> + name: $name,
> + $($f)*
> + }
> +
> + const _: () = {
> + static NAME: &$crate::str::CStr = $crate::c_str!($name);
> +
> + static ID_TABLE: [$crate::hid::DeviceIdShallow;
> + $crate::module_hid_driver!(@count_devices $($dev_id),+) + 1] = [
> + $($dev_id),+,
> + $crate::hid::DeviceIdShallow::new(),
> + ];
> +
> + static mut DRIVER: $crate::hid::DriverVTable =
> + $crate::hid::create_hid_driver::<$driver>(NAME, unsafe { &ID_TABLE[0] });
> +
> + impl $crate::Module for Module {
> + fn init(module: &'static $crate::ThisModule) -> Result<Self> {
> + let driver = unsafe { &mut DRIVER };
> + let mut reg = $crate::hid::Registration::register(
> + module,
> + ::core::pin::Pin::static_mut(driver),
> + NAME,
> + )?;
> + Ok(Module { _reg: reg })
> + }
> + }
> + };
> + }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 496ed32b0911..51b8c2689264 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -49,6 +49,8 @@
> #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
> pub mod firmware;
> pub mod fs;
> +#[cfg(CONFIG_RUST_HID_ABSTRACTIONS)]
> +pub mod hid;
> pub mod init;
> pub mod io;
> pub mod ioctl;
> --
> 2.47.2
>
>
Cheers,
Benjamin
Powered by blists - more mailing lists