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

[Christian Schrefl <chrisi.schrefl@gmail.com>]

On 23.01.25 5:00 PM, Greg Kroah-Hartman wrote:
> 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.

The struct miscdevice is "embedded" in the Rust MiscDeviceRegistration, 
so it should be fine if I understand you correctly.

> 
>> 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 :)

So keep the current approach?

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

Sure I'll change it to use container_of.

> 
> thanks,
> 
> greg k-h