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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2025012236-tall-untried-1d7c@gregkh>
Date: Wed, 22 Jan 2025 13:40:17 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Alice Ryhl <aliceryhl@...gle.com>
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 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".

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

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.

thanks,

greg k-h