On Mon, Dec 09, 2024 at 01:53:42PM +0100, Alice Ryhl wrote: > 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. Ah, yeah, that's going to get messy and will be a problem if someone has the file open then. > 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? Nope, that too is a window of a problem, luckily you "should" only register the misc device after you know the device is safe to use as once it is registered, it could be used so it "should" be the last thing you do in probe. So yes, you are right, and we do know about these issues (again see the talk I mentioned and some previous ones for many years at plumbers conferences by different people.) It's just up to someone to do the work to fix them. If you think we can prevent the race in the rust side, wonderful, I'm all for that being a valid fix. thanks, greg k-h
