| Message ID | 20220505220540.366218-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Headers | show |
| Series | fbdev: Fix use-after-free caused by wrong fb_info cleanup in drivers | expand |
On 06.05.2022 00:05, 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. > > To prevent this, move the framebuffer_release() call to fb_ops.fb_destroy > instead of doing it in the driver's .remove callback. > > Strictly speaking, the code flow in the driver is still wrong because all > the hardware cleanupd (i.e: iounmap) should be done in .remove while the > software cleanup (i.e: releasing the framebuffer) should be done in the > .fb_destroy handler. But this at least makes to match the behavior before > commit 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal"). > > Fixes: 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal") > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > > (no changes since v1) > > drivers/video/fbdev/efifb.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c > index ea42ba6445b2..cfa3dc0b4eee 100644 > --- a/drivers/video/fbdev/efifb.c > +++ b/drivers/video/fbdev/efifb.c > @@ -243,6 +243,10 @@ static void efifb_show_boot_graphics(struct fb_info *info) > static inline void efifb_show_boot_graphics(struct fb_info *info) {} > #endif > > +/* > + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end > + * of unregister_framebuffer() or fb_release(). Do any cleanup here. > + */ > static void efifb_destroy(struct fb_info *info) > { > if (efifb_pci_dev) > @@ -254,6 +258,9 @@ static void efifb_destroy(struct fb_info *info) > else > memunmap(info->screen_base); > } > + > + framebuffer_release(info); > + > if (request_mem_succeeded) > release_mem_region(info->apertures->ranges[0].base, > info->apertures->ranges[0].size); You are releasing info, then you are using it. I suspect it is responsible for multiple failures of Intel CI [1]. [1]: http://gfx-ci.fi.intel.com/tree/drm-tip/CI_DRM_11615/fi-adl-ddr5/boot0.txt Regards Andrzej > @@ -620,9 +627,9 @@ static int efifb_remove(struct platform_device *pdev) > { > struct fb_info *info = platform_get_drvdata(pdev); > > + /* efifb_destroy takes care of info cleanup */ > unregister_framebuffer(info); > sysfs_remove_groups(&pdev->dev.kobj, efifb_groups); > - framebuffer_release(info); > > return 0; > }
Hello Andrzej, On 5/6/22 15:07, Andrzej Hajda wrote: > On 06.05.2022 00:05, Javier Martinez Canillas wrote: [snip] >> + >> + framebuffer_release(info); >> + >> if (request_mem_succeeded) >> release_mem_region(info->apertures->ranges[0].base, >> info->apertures->ranges[0].size); > > You are releasing info, then you are using it. > > I suspect it is responsible for multiple failures of Intel CI [1]. > Yes, it is :( sorry about the mess. Ville already reported this to me. I'll post a patch in a minute. I wonder how this didn't happen before since .remove() happens before .fb_destroy() AFAIU...
Greeting,
FYI, we noticed the following commit (built with gcc-11):
commit: c6a2b1a99968f3fd74b6b16250d871e5b239e5a8 ("[PATCH v3 3/4] fbdev: efifb: Cleanup fb_info in .fb_destroy rather than .remove")
url: https://github.com/intel-lab-lkp/linux/commits/Javier-Martinez-Canillas/fbdev-Fix-use-after-free-caused-by-wrong-fb_info-cleanup-in-drivers/20220506-060830
base: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git 107c948d1d3e61d10aee9d0f7c3d81bbee9842af
patch link: https://lore.kernel.org/lkml/20220505220540.366218-1-javierm@redhat.com
in testcase: igt
version: igt-x86_64-eddc67c5-1_20220430
with following parameters:
group: group-07
ucode: 0xec
on test machine: 12 threads 1 sockets Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz with 32G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <oliver.sang@intel.com>
[ 25.615590][ T318] BUG: KASAN: use-after-free in efifb_destroy (drivers/video/fbdev/efifb.c:265)
[ 25.622534][ T318] Read of size 8 at addr ffff888143978358 by task systemd-udevd/318
[ 25.630335][ T318]
[ 25.632531][ T318] CPU: 8 PID: 318 Comm: systemd-udevd Not tainted 5.18.0-rc5-00019-gc6a2b1a99968 #1
[ 25.641707][ T318] Hardware name: Dell Inc. OptiPlex 7060/0C96W1, BIOS 1.4.2 06/11/2019
[ 25.641712][ T318] Call Trace:
[ 25.641714][ T318] <TASK>
[ 25.641715][ T318] ? efifb_destroy (drivers/video/fbdev/efifb.c:265)
[ 25.641722][ T318] dump_stack_lvl (lib/dump_stack.c:107 (discriminator 1))
[ 25.641726][ T318] print_address_description+0x1f/0x200
[ 25.641732][ T318] print_report.cold (mm/kasan/report.c:430)
[ 25.641736][ T318] ? _raw_spin_lock_irqsave (arch/x86/include/asm/atomic.h:202 include/linux/atomic/atomic-instrumented.h:543 include/asm-generic/qspinlock.h:82 include/linux/spinlock.h:185 include/linux/spinlock_api_smp.h:111 kernel/locking/spinlock.c:162)
[ 25.687074][ T318] kasan_report (mm/kasan/report.c:162 mm/kasan/report.c:493)
[ 25.691341][ T318] ? efifb_destroy (drivers/video/fbdev/efifb.c:265)
[ 25.695954][ T318] efifb_destroy (drivers/video/fbdev/efifb.c:265)
[ 25.700394][ T318] ? put_fb_info (arch/x86/include/asm/atomic.h:190 include/linux/atomic/atomic-instrumented.h:177 include/linux/refcount.h:272 include/linux/refcount.h:315 include/linux/refcount.h:333 drivers/video/fbdev/core/fbmem.c:79)
[ 25.704662][ T318] efifb_remove (drivers/video/fbdev/efifb.c:632)
[ 25.714635][ T318] device_release_driver_internal (drivers/base/dd.c:1202 drivers/base/dd.c:1223)
1;39mLSB: Load k[ 25.720538][ T318] ? klist_put (include/linux/kref.h:66 lib/klist.c:206 lib/klist.c:217)
[ 25.726157][ T318] bus_remove_device (drivers/base/bus.c:530)
[ 25.730943][ T318] device_del (drivers/base/core.c:3593)
ernel image with[ 25.735119][ T318] ? __device_link_del (drivers/base/core.c:3548)
[ 25.741431][ T318] ? _printk (kernel/printk/printk.c:2288)
[ 25.745350][ T318] ? record_print_text.cold (kernel/printk/printk.c:2288)
[ 25.751663][ T318] platform_device_del (drivers/base/platform.c:760)
[ 25.757138][ T318] platform_device_unregister (drivers/base/platform.c:547 drivers/base/platform.c:790)
[ 25.762523][ T318] do_remove_conflicting_framebuffers (drivers/video/fbdev/core/fbmem.c:1591)
[ 25.768768][ T318] remove_conflicting_framebuffers (drivers/video/fbdev/core/fbmem.c:1780)
[ 25.774666][ T318] remove_conflicting_pci_framebuffers (drivers/video/fbdev/core/fbmem.c:1879)
[ 25.780995][ T318] ? __mutex_unlock_slowpath+0x2c0/0x2c0
[ 25.787501][ T318] ? memcpy (mm/kasan/shadow.c:65 (discriminator 1))
[ 25.791331][ T318] drm_aperture_remove_conflicting_pci_framebuffers (drivers/gpu/drm/drm_aperture.c:349) drm
[ 25.799343][ T318] i915_driver_hw_probe (drivers/gpu/drm/i915/i915_driver.c:588) i915
[ 25.805153][ T318] i915_driver_probe (drivers/gpu/drm/i915/i915_driver.c:854) i915
[ 25.810660][ T318] ? __mutex_unlock_slowpath+0x2c0/0x2c0
[ 25.817167][ T318] ? i915_print_iommu_status (drivers/gpu/drm/i915/i915_driver.c:824) i915
[ 25.823380][ T318] ? drm_privacy_screen_get (drivers/gpu/drm/drm_privacy_screen.c:167) drm
[ 25.829320][ T318] i915_pci_probe (drivers/gpu/drm/i915/i915_pci.c:1191) i915
[ 25.834593][ T318] ? i915_pci_remove (drivers/gpu/drm/i915/i915_pci.c:1191) i915
[ 25.839932][ T318] ? _raw_read_unlock_irqrestore (kernel/locking/spinlock.c:161)
[ 25.845575][ T318] ? __cond_resched (kernel/sched/core.c:8177)
[ 25.850098][ T318] ? i915_pci_remove (drivers/gpu/drm/i915/i915_pci.c:1191) i915
[ 25.855450][ T318] local_pci_probe (drivers/pci/pci-driver.c:323)
[ 25.859973][ T318] pci_call_probe (drivers/pci/pci-driver.c:391)
[ 25.864496][ T318] ? _raw_spin_lock (arch/x86/include/asm/atomic.h:202 include/linux/atomic/atomic-instrumented.h:543 include/asm-generic/qspinlock.h:82 include/linux/spinlock.h:185 include/linux/spinlock_api_smp.h:134 kernel/locking/spinlock.c:154)
[ 25.869102][ T318] ? pci_pm_suspend_noirq (drivers/pci/pci-driver.c:351)
[ 25.874308][ T318] ? pci_assign_irq (drivers/pci/setup-irq.c:25)
[ 25.878919][ T318] ? pci_match_device (drivers/pci/pci-driver.c:107 drivers/pci/pci-driver.c:158)
[ 25.883784][ T318] ? kernfs_put (arch/x86/include/asm/atomic.h:123 (discriminator 1) include/linux/atomic/atomic-instrumented.h:576 (discriminator 1) fs/kernfs/dir.c:513 (discriminator 1))
[ 25.887959][ T318] pci_device_probe (drivers/pci/pci-driver.c:460)
[ 25.892575][ T318] ? pci_dma_configure (drivers/pci/pci-driver.c:1620)
[ 25.897448][ T318] really_probe (drivers/base/dd.c:542 drivers/base/dd.c:621)
[ 25.901803][ T318] __driver_probe_device (drivers/base/dd.c:752)
[ 25.906936][ T318] driver_probe_device (drivers/base/dd.c:782)
[ 25.911806][ T318] __driver_attach (drivers/base/dd.c:1142)
[ 25.916420][ T318] ? __device_attach_driver (drivers/base/dd.c:1094)
[ 25.921804][ T318] bus_for_each_dev (drivers/base/bus.c:301)
[ 25.926500][ T318] ? subsys_dev_iter_exit (drivers/base/bus.c:290)
[ 25.931541][ T318] ? klist_add_tail (include/linux/list.h:69 include/linux/list.h:102 lib/klist.c:104 lib/klist.c:137)
[ 25.936234][ T318] bus_add_driver (drivers/base/bus.c:619)
[ 25.940756][ T318] driver_register (drivers/base/driver.c:171)
[ 25.945365][ T318] i915_init (drivers/gpu/drm/i915/i915_driver.c:1004) i915
[ 25.950136][ T318] ? 0xffffffffc0b7d000
[ 25.954138][ T318] do_one_initcall (init/main.c:1298)
[ 25.958665][ T318] ? trace_event_raw_event_initcall_level (init/main.c:1289)
[ 25.965258][ T318] ? __asan_register_globals (mm/kasan/generic.c:214 (discriminator 3) mm/kasan/generic.c:226 (discriminator 3))
[ 25.970556][ T318] ? kasan_unpoison (mm/kasan/shadow.c:108 mm/kasan/shadow.c:142)
[ 25.975079][ T318] do_init_module (kernel/module.c:3731)
[ 25.979599][ T318] __do_sys_finit_module (kernel/module.c:4222)
[ 25.984720][ T318] ? __ia32_sys_init_module (kernel/module.c:4190)
[ 25.989927][ T318] ? __seccomp_filter (arch/x86/include/asm/bitops.h:214 include/asm-generic/bitops/instrumented-non-atomic.h:135 kernel/seccomp.c:351 kernel/seccomp.c:378 kernel/seccomp.c:410 kernel/seccomp.c:1183)
[ 25.994794][ T318] ? vm_mmap_pgoff (mm/util.c:523)
[ 25.999405][ T318] do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80)
[ 26.003668][ T318] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:115)
[ 26.009398][ T318] RIP: 0033:0x7f280dd1b989
[ 26.013659][ T318] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d d7 64 0c 00 f7 d8 64 89 01 48
All code
========
0: 00 c3 add %al,%bl
2: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1)
9: 00 00 00
c: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1)
11: 48 89 f8 mov %rdi,%rax
14: 48 89 f7 mov %rsi,%rdi
17: 48 89 d6 mov %rdx,%rsi
1a: 48 89 ca mov %rcx,%rdx
1d: 4d 89 c2 mov %r8,%r10
20: 4d 89 c8 mov %r9,%r8
23: 4c 8b 4c 24 08 mov 0x8(%rsp),%r9
28: 0f 05 syscall
2a:* 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax <-- trapping instruction
30: 73 01 jae 0x33
32: c3 retq
33: 48 8b 0d d7 64 0c 00 mov 0xc64d7(%rip),%rcx # 0xc6511
3a: f7 d8 neg %eax
3c: 64 89 01 mov %eax,%fs:(%rcx)
3f: 48 rex.W
Code starting with the faulting instruction
===========================================
0: 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax
6: 73 01 jae 0x9
8: c3 retq
9: 48 8b 0d d7 64 0c 00 mov 0xc64d7(%rip),%rcx # 0xc64e7
10: f7 d8 neg %eax
12: 64 89 01 mov %eax,%fs:(%rcx)
15: 48 rex.W
[ 26.033004][ T318] RSP: 002b:00007ffcd7609228 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[ 26.041236][ T318] RAX: ffffffffffffffda RBX: 0000561117222e00 RCX: 00007f280dd1b989
[ 26.049030][ T318] RDX: 0000000000000000 RSI: 00007f280dc20cad RDI: 0000000000000018
[ 26.056823][ T318] RBP: 00007f280dc20cad R08: 0000000000000000 R09: 0000000000000000
[ 26.064618][ T318] R10: 0000000000000018 R11: 0000000000000246 R12: 0000000000000000
[ 26.072414][ T318] R13: 000056111726f6d0 R14: 0000000000020000 R15: 0000561117222e00
[ 26.080214][ T318] </TASK>
[ 26.083093][ T318]
[ 26.085278][ T318] Allocated by task 1:
[ 26.089189][ T318] kasan_save_stack (mm/kasan/common.c:39)
[ 26.093708][ T318] __kasan_kmalloc (mm/kasan/common.c:45 mm/kasan/common.c:436 mm/kasan/common.c:515 mm/kasan/common.c:524)
[ 26.098143][ T318] framebuffer_alloc (drivers/video/fbdev/core/fbsysfs.c:49)
[ 26.102833][ T318] efifb_probe.cold (drivers/video/fbdev/efifb.c:461)
[ 26.107611][ T318] platform_probe (drivers/base/platform.c:1416)
[ 26.112048][ T318] really_probe (drivers/base/dd.c:542 drivers/base/dd.c:621)
[ 26.116399][ T318] __driver_probe_device (drivers/base/dd.c:752)
[ 26.121521][ T318] driver_probe_device (drivers/base/dd.c:782)
[ 26.126389][ T318] __device_attach_driver (drivers/base/dd.c:900)
[ 26.131606][ T318] bus_for_each_drv (drivers/base/bus.c:385 drivers/base/bus.c:426)
[ 26.136302][ T318] __device_attach (drivers/base/dd.c:970)
[ 26.140917][ T318] bus_probe_device (drivers/base/bus.c:489)
[ 26.145617][ T318] device_add (drivers/base/core.c:3412)
[ 26.149882][ T318] platform_device_add (drivers/base/platform.c:713)
[ 26.154842][ T318] sysfb_init (drivers/firmware/sysfb.c:72)
[ 26.158844][ T318] do_one_initcall (init/main.c:1298)
[ 26.163364][ T318] do_initcalls (init/main.c:1370 init/main.c:1387)
[ 26.167709][ T318] kernel_init_freeable (init/main.c:1617)
[ 26.172747][ T318] kernel_init (init/main.c:1504)
[ 26.176921][ T318] ret_from_fork (arch/x86/entry/entry_64.S:304)
[ 26.181184][ T318]
[ 26.183379][ T318] Freed by task 318:
[ 26.187123][ T318] kasan_save_stack (mm/kasan/common.c:39)
[ 26.191644][ T318] kasan_set_track (mm/kasan/common.c:45)
[ 26.196082][ T318] kasan_set_free_info (mm/kasan/generic.c:372)
[ 26.200864][ T318] __kasan_slab_free (mm/kasan/common.c:368 mm/kasan/common.c:328 mm/kasan/common.c:374)
[ 26.205644][ T318] kfree (mm/slub.c:1754 mm/slub.c:3510 mm/slub.c:4552)
[ 26.209302][ T318] efifb_destroy (drivers/video/fbdev/efifb.c:264)
[ 26.213651][ T318] efifb_remove (drivers/video/fbdev/efifb.c:632)
[ 26.217829][ T318] platform_remove (drivers/base/platform.c:1438)
[ 26.222265][ T318] device_release_driver_internal (drivers/base/dd.c:1202 drivers/base/dd.c:1223)
[ 26.228165][ T318] bus_remove_device (drivers/base/bus.c:530)
[ 26.232943][ T318] device_del (drivers/base/core.c:3593)
[ 26.237116][ T318] platform_device_del (drivers/base/platform.c:760)
[ 26.242585][ T318] platform_device_unregister (drivers/base/platform.c:547 drivers/base/platform.c:790)
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
sudo bin/lkp install job.yaml # job file is attached in this email
bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run
sudo bin/lkp run generated-yaml-file
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c index ea42ba6445b2..cfa3dc0b4eee 100644 --- a/drivers/video/fbdev/efifb.c +++ b/drivers/video/fbdev/efifb.c @@ -243,6 +243,10 @@ static void efifb_show_boot_graphics(struct fb_info *info) static inline void efifb_show_boot_graphics(struct fb_info *info) {} #endif +/* + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end + * of unregister_framebuffer() or fb_release(). Do any cleanup here. + */ static void efifb_destroy(struct fb_info *info) { if (efifb_pci_dev) @@ -254,6 +258,9 @@ static void efifb_destroy(struct fb_info *info) else memunmap(info->screen_base); } + + framebuffer_release(info); + if (request_mem_succeeded) release_mem_region(info->apertures->ranges[0].base, info->apertures->ranges[0].size); @@ -620,9 +627,9 @@ static int efifb_remove(struct platform_device *pdev) { struct fb_info *info = platform_get_drvdata(pdev); + /* efifb_destroy takes care of info cleanup */ unregister_framebuffer(info); sysfs_remove_groups(&pdev->dev.kobj, efifb_groups); - framebuffer_release(info); return 0; }
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. To prevent this, move the framebuffer_release() call to fb_ops.fb_destroy instead of doing it in the driver's .remove callback. Strictly speaking, the code flow in the driver is still wrong because all the hardware cleanupd (i.e: iounmap) should be done in .remove while the software cleanup (i.e: releasing the framebuffer) should be done in the .fb_destroy handler. But this at least makes to match the behavior before commit 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal"). Fixes: 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal") Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- (no changes since v1) drivers/video/fbdev/efifb.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)