Re: [PATCH v2 2/2] rust: miscdevice: access the `struct miscdevice` from fops->open()

[Alice Ryhl]

On Mon, Dec 9, 2024 at 1:08 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Dec 09, 2024 at 01:00:05PM +0100, Alice Ryhl wrote:
> > On Mon, Dec 9, 2024 at 12:53 PM Greg Kroah-Hartman
> > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Mon, Dec 09, 2024 at 12:38:32PM +0100, Alice Ryhl wrote:
> > > > On Mon, Dec 9, 2024 at 12:10 PM Greg Kroah-Hartman
> > > > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > On Mon, Dec 09, 2024 at 11:50:57AM +0100, Alice Ryhl wrote:
> > > > > > On Mon, Dec 9, 2024 at 9:48 AM Greg Kroah-Hartman
> > > > > > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > On Mon, Dec 09, 2024 at 07:27:47AM +0000, Alice Ryhl wrote:
> > > > > > > > Providing access to the underlying `struct miscdevice` is useful for
> > > > > > > > various reasons. For example, this allows you access the miscdevice's
> > > > > > > > internal `struct device` for use with the `dev_*` printing macros.
> > > > > > > >
> > > > > > > > Note that since the underlying `struct miscdevice` could get freed at
> > > > > > > > any point after the fops->open() call, only the open call is given
> > > > > > > > access to it. To print from other calls, they should take a refcount on
> > > > > > > > the device to keep it alive.
> > > > > > >
> > > > > > > The lifespan of the miscdevice is at least from open until close, so
> > > > > > > it's safe for at least then (i.e. read/write/ioctl/etc.)
> > > > > >
> > > > > > How is that enforced? What happens if I call misc_deregister while
> > > > > > there are open fds?
> > > > >
> > > > > You shouldn't be able to do that as the code that would be calling
> > > > > misc_deregister() (i.e. in a module unload path) would not work because
> > > > > the module reference count is incremented at this point in time due to
> > > > > the file operation module reference.
> > > >
> > > > Oh .. so misc_deregister must only be called when the module is being unloaded?
> > >
> > > Traditionally yes, that's when it is called.  Do you see it happening in
> > > any other place in the kernel today?
> >
> > I had not looked, but I know that Binder allows dynamically creating
> > and removing its devices at runtime. It happens to be the case that
> > this is only supported when binderfs is used, which is when it doesn't
> > use miscdevice, so technically Binder does not call misc_deregister()
> > outside of module unload, but following its example it's not hard to
> > imagine that such removals could happen.
>
> That's why those are files and not misc devices :)

I grepped for misc_deregister and the first driver I looked at is
drivers/misc/bcm-vk which seems to allow dynamic deregistration if the
pci device is removed.

Another tricky path is error cleanup in its probe function.
Technically, if probe fails after registering the misc device, there's
a brief moment where you could open the miscdevice before it gets
removed in the cleanup path, which seems to me that it could lead to
UAF?

Or is there something I'm missing?


Alice