linux-kernel - Re: [PATCH 5/7] rust: miscdevice: properly support device drivers

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <aDrbAWtDRPP80-xO@pollux>
Date: Sat, 31 May 2025 12:33:37 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Benno Lossin <lossin@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, ojeda@...nel.org,
	alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net,
	bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
	a.hindborg@...nel.org, aliceryhl@...gle.com, tmgross@...ch.edu,
	chrisi.schrefl@...il.com, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/7] rust: miscdevice: properly support device drivers

On Sat, May 31, 2025 at 10:05:28AM +0200, Benno Lossin wrote:
> On Sat May 31, 2025 at 12:17 AM CEST, Danilo Krummrich wrote:
> > On Fri, May 30, 2025 at 10:06:55PM +0200, Benno Lossin wrote:
> >> On Fri May 30, 2025 at 4:24 PM CEST, Danilo Krummrich wrote:
> >> > -impl<T: MiscDevice> MiscDeviceRegistration<T> {
> >> > -    /// Register a misc device.
> >> > -    pub fn register(
> >> > +impl<T: MiscDevice> RawDeviceRegistration<T> {
> >> > +    fn new<'a>(
> >> >          opts: MiscDeviceOptions,
> >> > -        data: impl PinInit<T::RegistrationData, Error>,
> >> > -    ) -> impl PinInit<Self, Error> {
> >> > +        parent: Option<&'a Device<Bound>>,
> >> > +        data: &'a T::RegistrationData,
> >> > +    ) -> impl PinInit<Self, Error> + 'a
> >> > +    where
> >> > +        T: 'a,
> >> > +    {
> >> >          try_pin_init!(Self {
> >> > -            data <- Opaque::pin_init(data),
> >> > +            // INVARIANT: `Self` is always embedded in a `MiscDeviceRegistration<T>`, hence `data`
> >> > +            // is guaranteed to be valid for the entire lifetime of `Self`.
> >> > +            data: NonNull::from(data),
> >> 
> >> Both the argument in the INVARIANT comment and way this works are a bit
> >> flawed.
> >
> > Why is the argument flawed? Let's say we go with your proposal below, what would
> > the safety requirement for RawDeviceRegistration::new and the invariant of
> > RawDeviceRegistration look like? Wouldn't it be the exact same argument?
> 
> So what I write below it not really true. But the argument relies on the
> fact that `data` is pointing to a value that will stay alive for the
> duration of the existence of `self`. That should be a safety requirement
> of `new` (even if we take a reference as an argument).

Ok, that's fair -- I'll add the safety requirement.

> >> Instead, I'd recommend directly taking the `NonNull` as a
> >> parameter. Yes the function will need to be `unsafe`, but the lifetime
> >> that you're creating below only lives for `'a`, but the object might
> >> live much longer. You might still be fine, but I'd just recommend
> >> staying in raw pointer land (or in this case `NonNull`).