Message-ID: <81cffd91-6b0b-4f09-a5c8-e1697975502e@gmail.com>
Date: Thu, 23 Jan 2025 16:52:26 +0100
From: Christian Schrefl <chrisi.schrefl@...il.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
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 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.

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.

> 
>> 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.

> 
> thanks,
> 
> greg k-h

Powered by blists - more mailing lists