linux-kernel - Re: [RFC PATCH 1/2] rust: LED abstraction

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAH5fLggju9ZYPD7LRTZKXJ9dhuLJ0uAS-USAokeoSvjOiN1v=w@mail.gmail.com>
Date: Mon, 18 Nov 2024 11:22:31 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Fiona Behrens <me@...enk.dev>
Cc: Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>, linux-leds@...r.kernel.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>, 
	Trevor Gross <tmgross@...ch.edu>, FUJITA Tomonori <fujita.tomonori@...il.com>, 
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [RFC PATCH 1/2] rust: LED abstraction

On Wed, Oct 9, 2024 at 12:58 PM Fiona Behrens <me@...enk.dev> wrote:
> +impl<'a, T> Led<T>
> +where
> +    T: Operations + 'a,
> +{
> +    /// Register a new LED with a predefine name.
> +    pub fn register_with_name(
> +        name: &'a CStr,
> +        device: Option<&'a Device>,
> +        config: &'a LedConfig,
> +        data: T,
> +    ) -> impl PinInit<Self, Error> + 'a {
> +        try_pin_init!( Self {
> +            led <- Opaque::try_ffi_init(move |place: *mut bindings::led_classdev| {
> +            // SAFETY: `place` is a pointer to a live allocation, so erasing is valid.
> +            unsafe { place.write_bytes(0, 1) };
> +
> +            // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
> +            unsafe { Self::build_with_name(place, name) };
> +
> +            // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
> +            unsafe { Self::build_config(place, config) };
> +
> +            // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
> +            unsafe { Self::build_vtable(place) };
> +
> +        let dev = device.map(|dev| dev.as_raw()).unwrap_or(ptr::null_mut());
> +            // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
> +        crate::error::to_result(unsafe {
> +            bindings::led_classdev_register_ext(dev, place, ptr::null_mut())
> +        })
> +            }),
> +            data: data,
> +        })
> +    }
> +
> +    /// Add nameto the led_classdev.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` has to be valid.
> +    unsafe fn build_with_name(ptr: *mut bindings::led_classdev, name: &'a CStr) {
> +        // SAFETY: `ptr` is pointing to a live allocation, so the deref is safe.
> +        let name_ptr = unsafe { ptr::addr_of_mut!((*ptr).name) };
> +        // SAFETY: `name_ptr` points to a valid allocation and we have exclusive access.
> +        unsafe { ptr::write(name_ptr, name.as_char_ptr()) };
> +    }
> +
> +    /// Add config to led_classdev.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` has to be valid.
> +    unsafe fn build_config(ptr: *mut bindings::led_classdev, config: &'a LedConfig) {
> +        // SAFETY: `ptr` is pointing to a live allocation, so the deref is safe.
> +        let color_ptr = unsafe { ptr::addr_of_mut!((*ptr).color) };
> +        // SAFETY: `color_ptr` points to a valid allocation and we have exclusive access.
> +        unsafe { ptr::write(color_ptr, config.color.into()) };
> +    }
> +}

This usage of lifetimes looks incorrect to me. It looks like you are
trying to say that the references must be valid for longer than the
Led<T>, but what you are writing here does not enforce that. The Led
struct must be annotated with the 'a lifetime if you want that, but
I'm inclined to say you should not go for the lifetime solution in the
first place.

Alice