Hello Daniel, On 5/5/22 14:52, Daniel Vetter wrote: > On Wed, May 04, 2022 at 11:57:22PM +0200, Javier Martinez Canillas wrote: >> The driver is calling framebuffer_release() in its .remove callback, but >> this will cause the struct fb_info to be freed too early. Since it could >> be that a reference is still hold to it if user-space opened the fbdev. >> >> This would lead to a use-after-free error if the framebuffer device was >> unregistered but later a user-space process tries to close the fbdev fd. >> >> The correct thing to do is to only unregister the framebuffer in the >> driver's .remove callback, but do any cleanup in the fb_ops.fb_destroy. >> >> Suggested-by: Daniel Vetter <daniel.vetter@xxxxxxxx> >> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx> > > I think this should have a Fixes: line for the patch from Thomas which > changed the remove_conflicting_fb code: > > 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal") > Ok. > I think we should also mention that strictly speaking the code flow is now > wrong, because hw cleanup (like iounmap) should be done from ->remove > while sw cleanup (like calling framebuffer_release()) is the only thing > that should be done from ->fb_destroy. But the current code matches what > was happening before 27599aacbaef so more minimal "fix" > Yes, I tried to make it as small as possible. Thomas pointed out that vesafb has the same issue and I included in v2. I had move things around more there though. > With those details added Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > > Same for the next patch. Thanks. I'll post a v3 adding what you suggested but probably not today. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat