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

[Alice Ryhl]

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.

> > > Yeah, it's a horrid hack, and one day we will put "real" revoke logic in
> > > here to detach the misc device from the file operations if this were to
> > > happen.  It's a very very common anti-pattern that many subsystems have
> > > that is a bug that we all have been talking about for a very very long
> > > time.  Wolfram even has a plan for how to fix it all up (see his Japan
> > > LinuxCon talk from 2 years ago), but I don't think anyone is doing the
> > > work on it :(
> > >
> > > The media and drm layers have internal hacks/work-arounds to try to
> > > handle this issue, but luckily for us, the odds of a misc device being
> > > dynamically removed from the system is pretty low.
> > >
> > > Once / if ever, we get the revoke type logic implemented, then we can
> > > apply that to the misc device code and follow it through to the rust
> > > side if needed.
> >
> > If dynamically deregistering is not safe, then we need to change the
> > Rust abstractions to prevent it.
>
> Dynamically deregistering is not unsafe, it's just that I don't think
> you will physically ever have the misc_deregister() path called if a
> file handle is open.  Same should be the case for rust code, it should
> "just work" without any extra code to do so.

Well, if I give files access to the struct miscdevice in all fops
hooks, then deregistering does become unsafe since accessing it in an
ioctl after deregistering would be a UAF. I'd like to prevent the user
from doing that.

Alice