linux-kernel - Re: [PATCH 2/3] rust: miscdevice: Add additional data to MiscDeviceRegistration

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: <2025012337-wired-sensually-5c49@gregkh>
Date: Thu, 23 Jan 2025 17:00:57 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Christian Schrefl <chrisi.schrefl@...il.com>
Cc: 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>,
	Arnd Bergmann <arnd@...db.de>, Lee Jones <lee@...nel.org>,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] rust: miscdevice: Add additional data to
 MiscDeviceRegistration

On Thu, Jan 23, 2025 at 04:52:26PM +0100, Christian Schrefl wrote:
> 
> 
> On 22.01.25 10:28 AM, Greg Kroah-Hartman wrote:
> > On Sun, Jan 19, 2025 at 11:11:14PM +0100, Christian Schrefl wrote:
> >> When using the Rust miscdevice bindings, you generally embed the
> >> MiscDeviceRegistration within another struct:
> >>
> >> struct MyDriverData {
> >>     data: SomeOtherData,
> >>     misc: MiscDeviceRegistration<MyMiscFile>
> >> }
> >>
> >> In the `fops->open` callback of the miscdevice, you are given a
> >> reference to the registration, which allows you to access its fields.
> >> For example, as of commit 284ae0be4dca ("rust: miscdevice: Provide
> >> accessor to pull out miscdevice::this_device") you can access the
> >> internal `struct device`. However, there is still no way to access the
> >> `data` field in the above example, because you only have a reference to
> >> the registration.
> > 
> > What's wrong with the driver_data pointer in the misc device structure?
> > Shouldn't you be in control of that as you are a misc driver owner?  Or
> > does the misc core handle this I can't recall at the moment, sorry.
> 
> 
> I don't know the internals of (C) miscdevice good enough to know where I'm
> allowed to store something, since there is no private_data field.

You are right, I was wrong here, sorry.  A misc device either needs to
be "stand alone" or embedded into something else.

> Not sure how the lifetimes of the whole device and device->driver_data are.
> But even that instead we use that we will need a rust abstraction for that to
> allow safe drivers.

Agreed, so let's make it work properly :)

> > 
> >> Using container_of is also not possible to do safely. For example, if
> >> the destructor of `MyDriverData` runs, then the destructor of `data`
> >> would run before the miscdevice is deregistered, so using container_of
> >> to access `data` from `fops->open` could result in a UAF. A similar
> >> problem can happen on initialization if `misc` is not the last field to
> >> be initialized.
> >>
> >> To provide a safe way to access user-defined data stored next to the
> >> `struct miscdevice`, make `MiscDeviceRegistration` into a container that
> >> can store a user-provided piece of data. This way, `fops->open` can
> >> access that data via the registration, since the data is stored inside
> >> the registration.
> > 
> > "next to" feels odd, that's what a container_of is for, but be careful
> > as to who owns the lifecycle of the object you are trying to get to.
> > You can't have multiple objects with different lifecycles in the same
> > structure (i.e. don't mix a misc device and a platform device together).
> > 
> > So a real example here would be good to see, can you post your driver at
> > the same time so that we can see what you are doing and perhaps provide
> > a better way to do it?
> 
> 
> The `struct miscdevice` is currently the first item in the 
> `MiscDeviceRegistration` so the  `struct miscdevice` and the 
> `MiscDeviceRegistration` have the same address.
> I can use container_of! if people think that more understandable.

You always have to use container_of! in case things move around.  If the
location is the same place, then the compiler just optimizes it all away
and doesn't do any pointer math so it's fine.

thanks,

greg k-h