Message-ID: <CAH5fLgh7QyVSgqJZM-krHmF87c76a84Cg6PyT47TYjMsN9iwZg@mail.gmail.com>
Date: Wed, 22 Jan 2025 11:11:07 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Christian Schrefl <chrisi.schrefl@...il.com>, 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>, 
	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 Wed, Jan 22, 2025 at 10:28 AM Greg Kroah-Hartman
<gregkh@...uxfoundation.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?

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

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.

Alice

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

Powered by blists - more mailing lists