We tested Javier's fix, issue couldn't be reproduced anymore. https://lore.kernel.org/dri-devel/20220502135014.377945-1-javierm@xxxxxxxxxx/T/#u But this fix doesn't cover all FB driver's interface: .get_unmapped_area = get_fb_unmapped_area, .fsync = fb_deferred_io_fsync, file_fb_info(file) NULL checking should be added in these two interface functions(get_fb_unmapped_area and fb_deferred_io_fsync) as well? Regards, Junxiao -----Original Message----- From: Thomas Zimmermann <tzimmermann@xxxxxxx> Sent: Tuesday, May 3, 2022 3:16 PM To: Chang, Junxiao <junxiao.chang@xxxxxxxxx>; linux-fbdev@xxxxxxxxxxxxxxx Cc: lethal@xxxxxxxxxxxx; patchwork-bot@xxxxxxxxxx; deller@xxxxxx; Li, Lili <lili.li@xxxxxxxxx>; Javier Martinez Canillas <javierm@xxxxxxxxxx> Subject: Re: [PATCH] video: fbdev: don't remove firmware fb device if it is busy (cc'ing Javier) Hi Am 03.05.22 um 02:38 schrieb Chang, Junxiao: > Hi Thomas, > > We reproduced this issue with Yocto image + 5.18-rc3 kernel. > If you could try Yocto image and enable psplash, the problem could be reproduced almost every time if psplash is launched before Intel i915 registers 2nd framebuffer. > > During Yocto booting up, it launches psplash which opens EFI firmware framebuffer and plays animation. When it exits, fb_release in kernel might access memory which has been released. > > Overall process looks like: > 1. EFI registers framebuffer in efifb_probe, framebuffer_alloc is > called to allocate "struct fb_info" memory; 2. In userspace, psplash > is started to play boot animation, it opens framebuffer driver. In > fb_open function, it saves fb_info memory to file->private_data; 3. > When Intel i915 driver is probed, it registers 2nd framebuffer, and > will remove conflicting framebuffer; 4. In do_remove_conflicting_framebuffers, it calls "platform_device_unregister" to remove EFI platform framebuffer device; 5. In EFI framebuffer's efifb_remove function, it calls framebuffer_release to release "struct fb_info" memory which is still use and stored in file->private_data; 6. When psplash user space application exits, it calls fb_release function, and accesses to fb_info memory, but it has been released in step 5. Kernel panic happens. Thanks for the information. We only had a similar report about RPi devices, but we never encountered this problem before. > > Our patch is to check whether EFI framebuffer is opened at that time. If it is free(registered_fb[i]->count == 1), go ahead to remove EFI platform device. Or else, just unregister framebuffer to avoid kernel panic. Javier posted a possible fix for the root cause of this problem, but we're still looking for testers. If you have the opportunity, could you please test the patch at [1] and report back on the results. Best regards Thomas [1] https://lore.kernel.org/dri-devel/20220502135014.377945-1-javierm@xxxxxxxxxx/T/#u > > Thanks, > Junxiao > > -----Original Message----- > From: Thomas Zimmermann <tzimmermann@xxxxxxx> > Sent: Monday, May 2, 2022 3:24 PM > To: Chang, Junxiao <junxiao.chang@xxxxxxxxx>; > linux-fbdev@xxxxxxxxxxxxxxx > Cc: lethal@xxxxxxxxxxxx; patchwork-bot@xxxxxxxxxx; deller@xxxxxx; Li, > Lili <lili.li@xxxxxxxxx> > Subject: Re: [PATCH] video: fbdev: don't remove firmware fb device if > it is busy > > Hi > > Am 30.04.22 um 04:57 schrieb Junxiao Chang: >> When firmware framebuffer is in use, don't remove its platform device. >> Or else its fb_info buffer is released by platform remove hook while >> device is still opened. This introduces use after free issue. > > When exactly does this happen? Do you have a reliable way of reproducing it? > > Best regards > Thomas > >> >> A kernel panic example: >> CPU: 2 PID: 3425 Comm: psplash Tainted: G U W 5.18.0-rc3 >> Hardware name: Intel Client Platform/ADP-S DDR5 UDIMM CRB >> RIP: 0010:native_queued_spin_lock_slowpath+0x1c7/0x210 >> RSP: 0018:ffffb3a0c0c2fdb0 EFLAGS: 00010206 >> RAX: 002dc074ff5c0988 RBX: ffff92e987a5d818 RCX: ffff92e989ba9f40 >> RDX: 0000000000002067 RSI: ffffffff864344f1 RDI: ffffffff8644183c >> RBP: ffff92f10f4abd40 R08: 0000000000000001 R09: ffff92e986dc2188 ... >> Call Trace: >> <TASK> >> _raw_spin_lock+0x2c/0x30 >> __mutex_lock.constprop.0+0x175/0x4f0 >> ? _raw_spin_unlock+0x15/0x30 >> ? list_lru_add+0x124/0x160 >> fb_release+0x1b/0x60 >> __fput+0x89/0x240 >> task_work_run+0x59/0x90 >> do_exit+0x343/0xaf0 >> do_group_exit+0x2d/0x90 >> __x64_sys_exit_group+0x14/0x20 >> do_syscall_64+0x40/0x90 >> entry_SYSCALL_64_after_hwframe+0x44/0xae >> >> Fixes: 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced >> removal") >> Signed-off-by: Junxiao Chang <junxiao.chang@xxxxxxxxx> >> Signed-off-by: Lili Li <lili.li@xxxxxxxxx> >> --- >> drivers/video/fbdev/core/fbmem.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/video/fbdev/core/fbmem.c >> b/drivers/video/fbdev/core/fbmem.c >> index a6bb0e438216..ff9b9830b398 100644 >> --- a/drivers/video/fbdev/core/fbmem.c >> +++ b/drivers/video/fbdev/core/fbmem.c >> @@ -1586,7 +1586,9 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a, >> * framebuffer as before without warning. >> */ >> do_unregister_framebuffer(registered_fb[i]); >> - } else if (dev_is_platform(device)) { >> + } else if (dev_is_platform(device) && >> + refcount_read(®istered_fb[i]->count) == 1) { >> + /* Remove platform device if it is not in use. */ >> registered_fb[i]->forced_out = true; >> platform_device_unregister(to_platform_device(device)); >> } else { > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) > Geschäftsführer: Ivo Totev -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
