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

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

On 22.01.25 2:06 PM, Alice Ryhl wrote:
> On Wed, Jan 22, 2025 at 1:40 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>>
>> On Wed, Jan 22, 2025 at 11:11:07AM +0100, Alice Ryhl wrote:
>>> On Wed, Jan 22, 2025 at 10:28 AM Greg Kroah-Hartman
>>> <gregkh@linuxfoundation.org> 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.
>>>
>>> There's no such pointer in struct miscdevice?
>>
>> struct miscdevice->struct device->driver_data;
>>
>> But in digging, this might not be "allowed" for a misc driver to do, I
>> can't remember anymore, sorry.
>>
>>>>> 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).
>>>
>>> Ultimately, this is intended to solve the problem that container_of
>>> solves in C. Actually using container_of runs into the challenge that
>>> you have no way of knowing if those other fields are still valid.
>>
>> You "know" they are valid if you have a pointer to your structure,
>> right?  Someone sent that to you, so by virtue of that it has to be
>> valid.
>>
>>> If
>>> the destructor of the struct starts running, then it might have
>>> destroyed some of the fields, but not yet have destroyed the
>>> miscdevice field. Since you can't know if the rest of the struct is
>>> still valid, it's not safe to use container_of.
>>
>> That's a different issue, that's a lifetime issue of "can I look at
>> these other fields at this point in time and trust them", it's not a
>> "these are not valid thing to look at".
> 
> The memory containing the field can't have been deallocated yet, but
> if we provide a safe way to get access to those fields after their
> destructor runs, then that's still a problem. If there's a field of
> type `KBox<MyType>`, i.e. a pointer to a kmalloc allocation, then the
> destructor will have freed that allocation. We cannot let you access
> the field that holds the KBox after that happens because the pointer
> will be dangling.
> 
>>> Storing those other fields within the registration solves this issue.
>>> The registration ensures that the miscdevice is deregistered before
>>> the `data` field is destroyed. This means that if it's safe to access
>>> the miscdevice field, then it's also safe to access the `data` field,
>>> and that's the guarantee we need.
>>
>> I'm confused.  A misc device should be embedded inside of something
>> else.  And the misc device controls the lifespan of that "something
>> else" structure, right?  So by virtue of the misc device being alive,
>> everything else in there should be ok.
> 
> This patch proposes that the way you write
> 
> struct MyDriverData {
>     // lifetime of these fields is controlled by misc
>     field1: Foo,
>     field2: Bar,
>     misc: MiscDeviceRegistration<MyMiscFile>
> }
> 
> is
> 
> struct MyDriverData {
>     misc: MiscDeviceRegistration<MyMiscFile, Inner>
> }
> struct Inner {
>     // lifetime of these fields is controlled by misc
>     field1: Foo,
>     field2: Bar,
> }
> 
> in memory all three fields gets laid out next to each other.
> 
>> Now individual misc devices might want to do different things but if
>> they are being torn down, that means all references are dropped so any
>> "container_of()" like cast-back should not even be possible as there
>> should not be a pointer that you can cast-back from here.
>>
>> Or am I confused?  I think a real example would be good to see here.
> 
> Christian, do you mind sharing the example you showed to me?

Sure

#[pin_data(PinnedDrop)]
struct LedPwmDriver {
    pdev: platform::Device,
    mapping: Devres<IoMem<MAPPING_SIZE, true>>,
    #[pin]
    miscdev: MiscDeviceRegistration<RustMiscDevice>,
}

impl platform::Driver for LedPwmDriver {
    type IdInfo = ();
    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
    fn probe(pdev: &mut platform::Device, info: Option<&Self::IdInfo>) -> Result<Pin<KBox<Self>>> {
        dev_dbg!(pdev.as_ref(), "Probe Rust Platform driver sample.\n");
        if info.is_some() {
            dev_info!(pdev.as_ref(), "Probed with info\n");
        }
        let mapping = pdev
            .ioremap_resource_sized::<MAPPING_SIZE, true>(pdev.resource(0).ok_or(ENXIO)?)
            .map_err(|_| ENXIO)?;

        // Initialize the HW
        mapping.try_access().ok_or(ENXIO)?.writel(0xFF, 0x0);

        let options = MiscDeviceOptions { name: c_str!("led0") };
        let drvdata = KBox::try_pin_init(
            try_pin_init!(Self {
                pdev: pdev.clone(),
                mapping,
                miscdev <- MiscDeviceRegistration::register(options),
            }),
            GFP_KERNEL,
        )?;
        Ok(drvdata)
    }
}

#[pinned_drop]
impl PinnedDrop for LedPwmDriver {
    fn drop(self: Pin<&mut Self>) {
        dev_info!(self.pdev.as_ref(), "Remove Rust Platform driver sample.\n");

        if let Some(res) = self.mapping.try_access() {
            res.writel(0x00, 0x0);
        }
    }
}

#[pin_data(PinnedDrop)]
struct RustMiscDevice {
    dev: ARef<Device>,
    mapping: &'static Devres<IoMem<MAPPING_SIZE, true>>,
}
#[vtable]
impl MiscDevice for RustMiscDevice {
    type Ptr = Pin<KBox<Self>>;
    fn open(_file: &File, misc: &MiscDeviceRegistration<Self>) -> Result<Pin<KBox<Self>>> {
        let dev = ARef::from(misc.device());
        // SAFETY:
        // Stuff?
        let ledpwm = unsafe { &*container_of!(misc, LedPwmDriver, miscdev) };
        dev_info!(dev, "Opening Rust Misc Device Sample\n");
        let res = ledpwm.mapping.try_access().ok_or(ENXIO)?;
        res.writel(0x80, 0x0);
        KBox::try_pin_init(
            try_pin_init! {
                RustMiscDevice {
                    dev: dev,
                    mapping: &ledpwm.mapping
                }
            },
            GFP_KERNEL,
        )
    }
}

#[pinned_drop]
impl PinnedDrop for RustMiscDevice {
    fn drop(self: Pin<&mut Self>) {
        dev_info!(self.dev, "Exiting the Rust Misc Device Sample\n");

        if let Some(res) = self.mapping.try_access() {
            res.writel(0x10, 0x0);
        }
    }
}


I removed or simplified many irrelevant parts, the full driver can be found here:

https://github.com/onestacked/linux/blob/c9e2ef858e4537d33625cdf2b5d20ef4acfa9424/drivers/misc/ledpwm.rs

This driver was only an attempt to the assignment of a University course in Rust mostly to see if was 
possible or which abstractions are missing.

The driver using the patch can be found here:
https://github.com/onestacked/linux/blob/rust_ledpwm_driver/drivers/misc/ledpwm.rs


Cheers
Christian