Message-ID: <CAH5fLggZ-7Rb=98JF+KkieY_H13mRVLB+T1umOrQib3ah78M-Q@mail.gmail.com>
Date: Wed, 22 Jan 2025 14:06:29 +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 1:40 PM Greg Kroah-Hartman
<gregkh@...uxfoundation.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@...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?
>
> 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?

Alice

Powered by blists - more mailing lists