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: <aDn7K1FPlFPUsjo0@pollux>
Date: Fri, 30 May 2025 20:38:35 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Christian Schrefl <chrisi.schrefl@...il.com>
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,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/7] rust: miscdevice: properly support device drivers

On Fri, May 30, 2025 at 07:35:56PM +0200, Christian Schrefl wrote:
> On 30.05.25 4:24 PM, Danilo Krummrich wrote:

<snip>

> > +#[pinned_drop]
> > +impl<T: MiscDevice> PinnedDrop for RawDeviceRegistration<T> {
> > +    fn drop(self: Pin<&mut Self>) {
> > +        // SAFETY: We know that the device is registered by the type invariants.
> > +        unsafe { bindings::misc_deregister(self.inner.get()) };
> > +    }> +}

<snip>

> >  #[pinned_drop]
> >  impl<T: MiscDevice> PinnedDrop for MiscDeviceRegistration<T> {
> >      fn drop(self: Pin<&mut Self>) {
> > -        // SAFETY: We know that the device is registered by the type invariants.
> > -        unsafe { bindings::misc_deregister(self.inner.get()) };
> > -
> >          // SAFETY: `self.data` is valid for dropping.
> >          unsafe { core::ptr::drop_in_place(self.data.get()) };
> 
> I think this can race for a use after free.
> The data gets freed in this `Drop` impl but the `miscdevice_deregister` call will only
> happen in the `DeviceRegistrationInner` `Drop` implementatation, since the fields 
> will only be dropped after the `drop` function has executed.

Yes and no. :-) The fun part is that the drop order actually depends on whether
we have a parent device and use Devres or whether we have no parent device.

In the first case the drop order is correct by chance, due to Devres revoking
things when the parent device in unbound, which happens before, since the faux
device is dropped first.

But in general you're right, this needs to be fixed.

> Either  inner: DeviceRegistrationInner<T> needs to be wrapped in a ManuallyDrop and
> dropped manually,
> or the Data needs to be wrapped in a type that will automatically drop it (this would
> be fine with `UnsafePinned`).

There's also option 3), which is moving the drop_in_place() for the registration
data into RawDeviceRegistration::drop(), right below misc_deregister(), until we
have `UnsafePinned`.

(This is fine, since RawDeviceRegistration already has (and requires) a pointer
to the registration data and hence is always embedded in a
MiscDeviceRegistration.)