* [PATCH RFC 1/1] x86: fix bad memory access in fb_is_primary_device()
@ 2016-02-15 9:30 Alexander Popov
2016-02-16 7:55 ` Ingo Molnar
0 siblings, 1 reply; 9+ messages in thread
From: Alexander Popov @ 2016-02-15 9:30 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Alexander Popov,
x86, linux-kernel
Currently the code in fb_is_primary_device() contains to_pci_dev() macro
which is applied to dev from struct fb_info. In some cases this causes
bad memory access when fb_is_primary_device() handles fb_info of efifb.
The reason is that fb dev of efifb is embedded into struct platform_device
but not into struct pci_dev.
We can fix this by checking fb dev bus name in fb_is_primary_device().
It seems that this bug reveals some bigger problem with to_pci_dev(),
to_platform_device() and others, which just do container_of() and
don't check whether struct device is a part of the appropriate structure.
Should we do something more about it?
KASan report:
==================================================================
BUG: KASAN: slab-out-of-bounds in fb_is_primary_device+0x58/0x70 at addr ffff8803b68ca8a8
Read of size 8 by task swapper/0/1
=============================================================================
BUG kmalloc-1024 (Not tainted): kasan: bad access detected
-----------------------------------------------------------------------------
Disabling lock debugging due to kernel taint
INFO: Allocated in platform_device_alloc+0x2c/0xa0 age=183 cpu=0 pid=1
___slab_alloc+0x486/0x4e0
__slab_alloc+0x20/0x40
__kmalloc+0x1d7/0x240
platform_device_alloc+0x2c/0xa0
platform_device_register_full+0x3b/0x200
sysfb_init+0xd4/0x11e
do_one_initcall+0x122/0x290
kernel_init_freeable+0x2e9/0x3ad
kernel_init+0x13/0x100
ret_from_fork+0x3f/0x70
INFO: Slab 0xffffea000eda3200 objects=24 used=24 fp=0x (null) flags=0x5fff8000004080
INFO: Object 0xffff8803b68ca450 @offset=9296 fp=0xffff8803b68ca760
Bytes b4 ffff8803b68ca440: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca450: 60 a7 8c b6 03 88 ff ff 00 00 00 00 00 00 00 00 `...............
Object ffff8803b68ca460: 20 6c 6f 82 ff ff ff ff 10 5d a8 b6 03 88 ff ff lo......]......
Object ffff8803b68ca470: 00 60 b3 b8 00 88 ff ff 30 b1 c8 b9 03 88 ff ff .`......0.......
Object ffff8803b68ca480: d0 e7 6d b7 03 88 ff ff 30 6c 6f 82 ff ff ff ff ..m.....0lo.....
Object ffff8803b68ca490: 30 b1 c8 b9 03 88 ff ff 40 31 6f 82 ff ff ff ff 0.......@1o.....
Object ffff8803b68ca4a0: c0 a7 ad b6 03 88 ff ff 03 00 00 00 07 00 00 00 ................
Object ffff8803b68ca4b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca4c0: 00 00 00 00 00 00 00 00 c8 a4 8c b6 03 88 ff ff ................
Object ffff8803b68ca4d0: c8 a4 8c b6 03 88 ff ff 00 00 ca b7 03 88 ff ff ................
Object ffff8803b68ca4e0: 00 00 00 00 00 00 00 00 60 6a 6f 82 ff ff ff ff ........`jo.....
Object ffff8803b68ca4f0: 48 0b 68 82 ff ff ff ff 90 f5 4f b2 03 88 ff ff H.h.......O.....
Object ffff8803b68ca500: 00 80 ea b0 03 88 ff ff 00 00 00 00 80 00 00 00 ................
Object ffff8803b68ca510: 00 00 00 00 00 00 00 00 b8 d3 8c b6 03 88 ff ff ................
Object ffff8803b68ca520: f8 d8 8c b6 03 88 ff ff ff ff ff 7f 00 00 00 00 ................
Object ffff8803b68ca530: 00 00 00 00 00 00 00 00 38 a5 8c b6 03 88 ff ff ........8.......
Object ffff8803b68ca540: 38 a5 8c b6 03 88 ff ff 00 00 00 00 00 00 00 00 8...............
Object ffff8803b68ca550: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca560: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca570: e0 62 7c 81 ff ff ff ff 60 a4 8c b6 03 88 ff ff .b|.....`.......
Object ffff8803b68ca580: 00 00 00 00 ff ff ff ff ff ff ff ff 00 00 00 00 ................
Object ffff8803b68ca590: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca5a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca5b0: e0 ff ff ff 0f 00 00 00 b8 a5 8c b6 03 88 ff ff ................
Object ffff8803b68ca5c0: b8 a5 8c b6 03 88 ff ff 70 69 7c 81 ff ff ff ff ........pi|.....
Object ffff8803b68ca5d0: 00 00 00 00 00 00 00 00 d8 a5 8c b6 03 88 ff ff ................
Object ffff8803b68ca5e0: d8 a5 8c b6 03 88 ff ff 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca5f0: 00 00 00 00 00 00 00 00 81 00 00 00 00 00 00 00 ................
Object ffff8803b68ca600: 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca610: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca620: 00 00 00 00 00 00 00 00 a0 7a fb ff 00 00 00 00 .........z......
Object ffff8803b68ca630: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca640: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca650: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca660: 60 a6 8c b6 03 88 ff ff 60 a6 8c b6 03 88 ff ff `.......`.......
Object ffff8803b68ca670: ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca690: 00 00 00 00 00 00 00 00 98 a6 8c b6 03 88 ff ff ................
Object ffff8803b68ca6a0: 98 a6 8c b6 03 88 ff ff 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca6b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca6c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca6d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca6e0: e0 a6 8c b6 03 88 ff ff e0 a6 8c b6 03 88 ff ff ................
Object ffff8803b68ca6f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca710: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca720: 30 8a 7b 81 ff ff ff ff 00 00 00 00 00 00 00 00 0.{.............
Object ffff8803b68ca730: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca740: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca750: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca760: 65 66 69 2d 66 72 61 6d 65 62 75 66 66 65 72 00 efi-framebuffer.
Object ffff8803b68ca770: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca790: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca7a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca7b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca7c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca7d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca7e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca7f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca800: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca810: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca820: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca830: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
Object ffff8803b68ca840: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
CPU: 0 PID: 1 Comm: swapper/0 Tainted: G B 4.4.1 #10
Hardware name: Gigabyte Technology Co., Ltd. To be filled by O.E.M./Q87M-D2H, BIOS F7 01/17/2014
ffff8803b68c8000 00000000cdadd1cd ffff8803b7caf7c8 ffffffff815bab5c
ffff8803b9c07180 ffff8803b7caf7f8 ffffffff8130fbf9 ffff8803b9c07180
ffffea000eda3200 ffff8803b68ca450 1ffff10076f95f28 ffff8803b7caf820
Call Trace:
[<ffffffff815bab5c>] dump_stack+0x44/0x58
[<ffffffff8130fbf9>] print_trailer+0xf9/0x150
[<ffffffff813154e4>] object_err+0x34/0x40
[<ffffffff81317bc1>] kasan_report_error+0x221/0x540
[<ffffffff816878c9>] ? fb_alloc_cmap_gfp+0x139/0x190
[<ffffffff8130f123>] ? set_track+0x83/0x140
[<ffffffff81318478>] kasan_report+0x58/0x60
[<ffffffff81a0a148>] ? fb_is_primary_device+0x58/0x70
[<ffffffff81316d8d>] __asan_load8+0x5d/0x70
[<ffffffff81a0a148>] fb_is_primary_device+0x58/0x70
[<ffffffff8168505a>] register_framebuffer+0xda/0x5a0
[<ffffffff816878c9>] ? fb_alloc_cmap_gfp+0x139/0x190
[<ffffffff81684f80>] ? remove_conflicting_framebuffers+0x50/0x50
[<ffffffff813172c6>] ? kasan_unpoison_shadow+0x36/0x50
[<ffffffff813172c6>] ? kasan_unpoison_shadow+0x36/0x50
[<ffffffff81317496>] ? memcpy+0x36/0x40
[<ffffffff816874dc>] ? fb_copy_cmap+0x12c/0x190
[<ffffffff8168787d>] ? fb_alloc_cmap_gfp+0xed/0x190
[<ffffffff81693f1c>] efifb_probe+0xbac/0xc20
[<ffffffff81693370>] ? efifb_setcolreg+0xe0/0xe0
[<ffffffff8140ac15>] ? sysfs_do_create_link_sd.isra.2+0x85/0xc0
[<ffffffff8169b543>] ? acpi_dev_pm_attach+0x2e/0xe2
[<ffffffff817b9233>] platform_drv_probe+0x53/0xc0
[<ffffffff817b5fd2>] driver_probe_device+0x2f2/0x6a0
[<ffffffff817b6380>] ? driver_probe_device+0x6a0/0x6a0
[<ffffffff817b6439>] __driver_attach+0xb9/0xc0
[<ffffffff817b26b3>] bus_for_each_dev+0xf3/0x170
[<ffffffff817b25c0>] ? subsys_dev_iter_exit+0x10/0x10
[<ffffffff8131733e>] ? kasan_kmalloc+0x5e/0x70
[<ffffffff815eadf4>] ? __list_add+0x74/0xf0
[<ffffffff817b545b>] driver_attach+0x2b/0x30
[<ffffffff817b4d17>] bus_add_driver+0x307/0x3d0
[<ffffffff8290157c>] ? vesafb_driver_init+0x14/0x14
[<ffffffff817b70a3>] driver_register+0xd3/0x190
[<ffffffff817b910c>] __platform_driver_register+0x6c/0x80
[<ffffffff8290158e>] efifb_driver_init+0x12/0x14
[<ffffffff81002192>] do_one_initcall+0x122/0x290
[<ffffffff81002070>] ? try_to_run_init_process+0x40/0x40
[<ffffffff828a5a1b>] ? repair_env_string+0x2f/0x73
[<ffffffff81118766>] ? parse_args+0x356/0x600
[<ffffffff828a6584>] kernel_init_freeable+0x2e9/0x3ad
[<ffffffff828a629b>] ? start_kernel+0x59a/0x59a
[<ffffffff81098259>] ? native_load_sp0+0x29/0x40
[<ffffffff8112812a>] ? finish_task_switch+0xaa/0x350
[<ffffffff81c069f0>] ? rest_init+0x90/0x90
[<ffffffff81c06a03>] kernel_init+0x13/0x100
[<ffffffff81c069f0>] ? rest_init+0x90/0x90
[<ffffffff81c1824f>] ret_from_fork+0x3f/0x70
[<ffffffff81c069f0>] ? rest_init+0x90/0x90
Memory state around the buggy address:
ffff8803b68ca780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff8803b68ca800: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff8803b68ca880: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
^
ffff8803b68ca900: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff8803b68ca980: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
==================================================================
Signed-off-by: Alexander Popov <alpopov@ptsecurity.com>
---
arch/x86/video/fbdev.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/x86/video/fbdev.c b/arch/x86/video/fbdev.c
index d5644bb..4999f78 100644
--- a/arch/x86/video/fbdev.c
+++ b/arch/x86/video/fbdev.c
@@ -18,11 +18,12 @@ int fb_is_primary_device(struct fb_info *info)
struct pci_dev *default_device = vga_default_device();
struct resource *res = NULL;
- if (device)
- pci_dev = to_pci_dev(device);
-
- if (!pci_dev)
+ if (!device || !device->bus ||
+ !device->bus->name || strcmp(device->bus->name, "pci")) {
return 0;
+ }
+
+ pci_dev = to_pci_dev(device);
if (default_device) {
if (pci_dev == default_device)
--
1.9.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH RFC 1/1] x86: fix bad memory access in fb_is_primary_device()
2016-02-15 9:30 [PATCH RFC 1/1] x86: fix bad memory access in fb_is_primary_device() Alexander Popov
@ 2016-02-16 7:55 ` Ingo Molnar
2016-02-16 13:49 ` Matt Fleming
0 siblings, 1 reply; 9+ messages in thread
From: Ingo Molnar @ 2016-02-16 7:55 UTC (permalink / raw)
To: Alexander Popov, Matt Fleming, Arnd Bergmann, Tomi Valkeinen
Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel
(I've Cc:-ed the EFI-FB and FB gents. Mail quoted below.)
* Alexander Popov <alpopov@ptsecurity.com> wrote:
> Currently the code in fb_is_primary_device() contains to_pci_dev() macro
> which is applied to dev from struct fb_info. In some cases this causes
> bad memory access when fb_is_primary_device() handles fb_info of efifb.
> The reason is that fb dev of efifb is embedded into struct platform_device
> but not into struct pci_dev.
>
> We can fix this by checking fb dev bus name in fb_is_primary_device().
>
> It seems that this bug reveals some bigger problem with to_pci_dev(),
> to_platform_device() and others, which just do container_of() and
> don't check whether struct device is a part of the appropriate structure.
> Should we do something more about it?
>
> KASan report:
>
> ==================================================================
> BUG: KASAN: slab-out-of-bounds in fb_is_primary_device+0x58/0x70 at addr ffff8803b68ca8a8
> Read of size 8 by task swapper/0/1
> =============================================================================
> BUG kmalloc-1024 (Not tainted): kasan: bad access detected
> -----------------------------------------------------------------------------
>
> Disabling lock debugging due to kernel taint
> INFO: Allocated in platform_device_alloc+0x2c/0xa0 age=183 cpu=0 pid=1
> ___slab_alloc+0x486/0x4e0
> __slab_alloc+0x20/0x40
> __kmalloc+0x1d7/0x240
> platform_device_alloc+0x2c/0xa0
> platform_device_register_full+0x3b/0x200
> sysfb_init+0xd4/0x11e
> do_one_initcall+0x122/0x290
> kernel_init_freeable+0x2e9/0x3ad
> kernel_init+0x13/0x100
> ret_from_fork+0x3f/0x70
> INFO: Slab 0xffffea000eda3200 objects=24 used=24 fp=0x (null) flags=0x5fff8000004080
> INFO: Object 0xffff8803b68ca450 @offset=9296 fp=0xffff8803b68ca760
>
> Bytes b4 ffff8803b68ca440: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca450: 60 a7 8c b6 03 88 ff ff 00 00 00 00 00 00 00 00 `...............
> Object ffff8803b68ca460: 20 6c 6f 82 ff ff ff ff 10 5d a8 b6 03 88 ff ff lo......]......
> Object ffff8803b68ca470: 00 60 b3 b8 00 88 ff ff 30 b1 c8 b9 03 88 ff ff .`......0.......
> Object ffff8803b68ca480: d0 e7 6d b7 03 88 ff ff 30 6c 6f 82 ff ff ff ff ..m.....0lo.....
> Object ffff8803b68ca490: 30 b1 c8 b9 03 88 ff ff 40 31 6f 82 ff ff ff ff 0.......@1o.....
> Object ffff8803b68ca4a0: c0 a7 ad b6 03 88 ff ff 03 00 00 00 07 00 00 00 ................
> Object ffff8803b68ca4b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca4c0: 00 00 00 00 00 00 00 00 c8 a4 8c b6 03 88 ff ff ................
> Object ffff8803b68ca4d0: c8 a4 8c b6 03 88 ff ff 00 00 ca b7 03 88 ff ff ................
> Object ffff8803b68ca4e0: 00 00 00 00 00 00 00 00 60 6a 6f 82 ff ff ff ff ........`jo.....
> Object ffff8803b68ca4f0: 48 0b 68 82 ff ff ff ff 90 f5 4f b2 03 88 ff ff H.h.......O.....
> Object ffff8803b68ca500: 00 80 ea b0 03 88 ff ff 00 00 00 00 80 00 00 00 ................
> Object ffff8803b68ca510: 00 00 00 00 00 00 00 00 b8 d3 8c b6 03 88 ff ff ................
> Object ffff8803b68ca520: f8 d8 8c b6 03 88 ff ff ff ff ff 7f 00 00 00 00 ................
> Object ffff8803b68ca530: 00 00 00 00 00 00 00 00 38 a5 8c b6 03 88 ff ff ........8.......
> Object ffff8803b68ca540: 38 a5 8c b6 03 88 ff ff 00 00 00 00 00 00 00 00 8...............
> Object ffff8803b68ca550: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca560: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca570: e0 62 7c 81 ff ff ff ff 60 a4 8c b6 03 88 ff ff .b|.....`.......
> Object ffff8803b68ca580: 00 00 00 00 ff ff ff ff ff ff ff ff 00 00 00 00 ................
> Object ffff8803b68ca590: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca5a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca5b0: e0 ff ff ff 0f 00 00 00 b8 a5 8c b6 03 88 ff ff ................
> Object ffff8803b68ca5c0: b8 a5 8c b6 03 88 ff ff 70 69 7c 81 ff ff ff ff ........pi|.....
> Object ffff8803b68ca5d0: 00 00 00 00 00 00 00 00 d8 a5 8c b6 03 88 ff ff ................
> Object ffff8803b68ca5e0: d8 a5 8c b6 03 88 ff ff 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca5f0: 00 00 00 00 00 00 00 00 81 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca600: 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca610: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca620: 00 00 00 00 00 00 00 00 a0 7a fb ff 00 00 00 00 .........z......
> Object ffff8803b68ca630: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca640: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca650: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca660: 60 a6 8c b6 03 88 ff ff 60 a6 8c b6 03 88 ff ff `.......`.......
> Object ffff8803b68ca670: ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca690: 00 00 00 00 00 00 00 00 98 a6 8c b6 03 88 ff ff ................
> Object ffff8803b68ca6a0: 98 a6 8c b6 03 88 ff ff 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca6b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca6c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca6d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca6e0: e0 a6 8c b6 03 88 ff ff e0 a6 8c b6 03 88 ff ff ................
> Object ffff8803b68ca6f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca710: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca720: 30 8a 7b 81 ff ff ff ff 00 00 00 00 00 00 00 00 0.{.............
> Object ffff8803b68ca730: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca740: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca750: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca760: 65 66 69 2d 66 72 61 6d 65 62 75 66 66 65 72 00 efi-framebuffer.
> Object ffff8803b68ca770: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca790: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca7a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca7b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca7c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca7d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca7e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca7f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca800: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca810: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca820: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca830: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> Object ffff8803b68ca840: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> CPU: 0 PID: 1 Comm: swapper/0 Tainted: G B 4.4.1 #10
> Hardware name: Gigabyte Technology Co., Ltd. To be filled by O.E.M./Q87M-D2H, BIOS F7 01/17/2014
> ffff8803b68c8000 00000000cdadd1cd ffff8803b7caf7c8 ffffffff815bab5c
> ffff8803b9c07180 ffff8803b7caf7f8 ffffffff8130fbf9 ffff8803b9c07180
> ffffea000eda3200 ffff8803b68ca450 1ffff10076f95f28 ffff8803b7caf820
> Call Trace:
> [<ffffffff815bab5c>] dump_stack+0x44/0x58
> [<ffffffff8130fbf9>] print_trailer+0xf9/0x150
> [<ffffffff813154e4>] object_err+0x34/0x40
> [<ffffffff81317bc1>] kasan_report_error+0x221/0x540
> [<ffffffff816878c9>] ? fb_alloc_cmap_gfp+0x139/0x190
> [<ffffffff8130f123>] ? set_track+0x83/0x140
> [<ffffffff81318478>] kasan_report+0x58/0x60
> [<ffffffff81a0a148>] ? fb_is_primary_device+0x58/0x70
> [<ffffffff81316d8d>] __asan_load8+0x5d/0x70
> [<ffffffff81a0a148>] fb_is_primary_device+0x58/0x70
> [<ffffffff8168505a>] register_framebuffer+0xda/0x5a0
> [<ffffffff816878c9>] ? fb_alloc_cmap_gfp+0x139/0x190
> [<ffffffff81684f80>] ? remove_conflicting_framebuffers+0x50/0x50
> [<ffffffff813172c6>] ? kasan_unpoison_shadow+0x36/0x50
> [<ffffffff813172c6>] ? kasan_unpoison_shadow+0x36/0x50
> [<ffffffff81317496>] ? memcpy+0x36/0x40
> [<ffffffff816874dc>] ? fb_copy_cmap+0x12c/0x190
> [<ffffffff8168787d>] ? fb_alloc_cmap_gfp+0xed/0x190
> [<ffffffff81693f1c>] efifb_probe+0xbac/0xc20
> [<ffffffff81693370>] ? efifb_setcolreg+0xe0/0xe0
> [<ffffffff8140ac15>] ? sysfs_do_create_link_sd.isra.2+0x85/0xc0
> [<ffffffff8169b543>] ? acpi_dev_pm_attach+0x2e/0xe2
> [<ffffffff817b9233>] platform_drv_probe+0x53/0xc0
> [<ffffffff817b5fd2>] driver_probe_device+0x2f2/0x6a0
> [<ffffffff817b6380>] ? driver_probe_device+0x6a0/0x6a0
> [<ffffffff817b6439>] __driver_attach+0xb9/0xc0
> [<ffffffff817b26b3>] bus_for_each_dev+0xf3/0x170
> [<ffffffff817b25c0>] ? subsys_dev_iter_exit+0x10/0x10
> [<ffffffff8131733e>] ? kasan_kmalloc+0x5e/0x70
> [<ffffffff815eadf4>] ? __list_add+0x74/0xf0
> [<ffffffff817b545b>] driver_attach+0x2b/0x30
> [<ffffffff817b4d17>] bus_add_driver+0x307/0x3d0
> [<ffffffff8290157c>] ? vesafb_driver_init+0x14/0x14
> [<ffffffff817b70a3>] driver_register+0xd3/0x190
> [<ffffffff817b910c>] __platform_driver_register+0x6c/0x80
> [<ffffffff8290158e>] efifb_driver_init+0x12/0x14
> [<ffffffff81002192>] do_one_initcall+0x122/0x290
> [<ffffffff81002070>] ? try_to_run_init_process+0x40/0x40
> [<ffffffff828a5a1b>] ? repair_env_string+0x2f/0x73
> [<ffffffff81118766>] ? parse_args+0x356/0x600
> [<ffffffff828a6584>] kernel_init_freeable+0x2e9/0x3ad
> [<ffffffff828a629b>] ? start_kernel+0x59a/0x59a
> [<ffffffff81098259>] ? native_load_sp0+0x29/0x40
> [<ffffffff8112812a>] ? finish_task_switch+0xaa/0x350
> [<ffffffff81c069f0>] ? rest_init+0x90/0x90
> [<ffffffff81c06a03>] kernel_init+0x13/0x100
> [<ffffffff81c069f0>] ? rest_init+0x90/0x90
> [<ffffffff81c1824f>] ret_from_fork+0x3f/0x70
> [<ffffffff81c069f0>] ? rest_init+0x90/0x90
>
> Memory state around the buggy address:
> ffff8803b68ca780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ffff8803b68ca800: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> >ffff8803b68ca880: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ^
> ffff8803b68ca900: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ffff8803b68ca980: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> ==================================================================
>
> Signed-off-by: Alexander Popov <alpopov@ptsecurity.com>
> ---
> arch/x86/video/fbdev.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/video/fbdev.c b/arch/x86/video/fbdev.c
> index d5644bb..4999f78 100644
> --- a/arch/x86/video/fbdev.c
> +++ b/arch/x86/video/fbdev.c
> @@ -18,11 +18,12 @@ int fb_is_primary_device(struct fb_info *info)
> struct pci_dev *default_device = vga_default_device();
> struct resource *res = NULL;
>
> - if (device)
> - pci_dev = to_pci_dev(device);
> -
> - if (!pci_dev)
> + if (!device || !device->bus ||
> + !device->bus->name || strcmp(device->bus->name, "pci")) {
> return 0;
> + }
> +
> + pci_dev = to_pci_dev(device);
>
> if (default_device) {
> if (pci_dev == default_device)
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC 1/1] x86: fix bad memory access in fb_is_primary_device()
2016-02-16 7:55 ` Ingo Molnar
@ 2016-02-16 13:49 ` Matt Fleming
2016-02-16 15:18 ` Peter Jones
0 siblings, 1 reply; 9+ messages in thread
From: Matt Fleming @ 2016-02-16 13:49 UTC (permalink / raw)
To: Ingo Molnar
Cc: Alexander Popov, Arnd Bergmann, Tomi Valkeinen, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, x86, linux-kernel, Peter Jones,
linux-efi
[ Including Peter, the efifb maintainer. Original email is here,
http://marc.info/?l=linux-kernel&m=145552936131335&w=2
I've snipped some of the quoted text ]
On Tue, 16 Feb, at 08:55:22AM, Ingo Molnar wrote:
>
> (I've Cc:-ed the EFI-FB and FB gents. Mail quoted below.)
>
> * Alexander Popov <alpopov@ptsecurity.com> wrote:
>
> > Currently the code in fb_is_primary_device() contains to_pci_dev() macro
> > which is applied to dev from struct fb_info. In some cases this causes
> > bad memory access when fb_is_primary_device() handles fb_info of efifb.
> > The reason is that fb dev of efifb is embedded into struct platform_device
> > but not into struct pci_dev.
> >
> > We can fix this by checking fb dev bus name in fb_is_primary_device().
> >
> > It seems that this bug reveals some bigger problem with to_pci_dev(),
> > to_platform_device() and others, which just do container_of() and
> > don't check whether struct device is a part of the appropriate structure.
> > Should we do something more about it?
> >
> > KASan report:
[...]
> >
> > Signed-off-by: Alexander Popov <alpopov@ptsecurity.com>
> > ---
> > arch/x86/video/fbdev.c | 9 +++++----
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/video/fbdev.c b/arch/x86/video/fbdev.c
> > index d5644bb..4999f78 100644
> > --- a/arch/x86/video/fbdev.c
> > +++ b/arch/x86/video/fbdev.c
> > @@ -18,11 +18,12 @@ int fb_is_primary_device(struct fb_info *info)
> > struct pci_dev *default_device = vga_default_device();
> > struct resource *res = NULL;
> >
> > - if (device)
> > - pci_dev = to_pci_dev(device);
> > -
> > - if (!pci_dev)
> > + if (!device || !device->bus ||
> > + !device->bus->name || strcmp(device->bus->name, "pci")) {
> > return 0;
> > + }
> > +
> > + pci_dev = to_pci_dev(device);
> >
> > if (default_device) {
> > if (pci_dev == default_device)
> > --
> > 1.9.1
> >
I wonder if this issue could explain some of the efifb issues we've
seen reported on bugzilla.kernel.org in the past where switching from
efifb to some other framebuffer device caused hangs during boot. I'm
struggling to find the relevant bugzilla entries now, though.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC 1/1] x86: fix bad memory access in fb_is_primary_device()
2016-02-16 13:49 ` Matt Fleming
@ 2016-02-16 15:18 ` Peter Jones
2016-03-09 12:46 ` Alexander Popov
0 siblings, 1 reply; 9+ messages in thread
From: Peter Jones @ 2016-02-16 15:18 UTC (permalink / raw)
To: Matt Fleming
Cc: Ingo Molnar, Alexander Popov, Arnd Bergmann, Tomi Valkeinen,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
linux-efi
On Tue, Feb 16, 2016 at 01:49:18PM +0000, Matt Fleming wrote:
> [ Including Peter, the efifb maintainer. Original email is here,
>
> http://marc.info/?l=linux-kernel&m=145552936131335&w=2
>
> I've snipped some of the quoted text ]
>
> On Tue, 16 Feb, at 08:55:22AM, Ingo Molnar wrote:
> >
> > (I've Cc:-ed the EFI-FB and FB gents. Mail quoted below.)
> >
> > * Alexander Popov <alpopov@ptsecurity.com> wrote:
> >
> > > Currently the code in fb_is_primary_device() contains to_pci_dev() macro
> > > which is applied to dev from struct fb_info. In some cases this causes
> > > bad memory access when fb_is_primary_device() handles fb_info of efifb.
> > > The reason is that fb dev of efifb is embedded into struct platform_device
> > > but not into struct pci_dev.
> > >
> > > We can fix this by checking fb dev bus name in fb_is_primary_device().
> > >
> > > It seems that this bug reveals some bigger problem with to_pci_dev(),
> > > to_platform_device() and others, which just do container_of() and
> > > don't check whether struct device is a part of the appropriate structure.
> > > Should we do something more about it?
> > >
> > > KASan report:
>
> [...]
>
> > >
> > > Signed-off-by: Alexander Popov <alpopov@ptsecurity.com>
> > > ---
> > > arch/x86/video/fbdev.c | 9 +++++----
> > > 1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/x86/video/fbdev.c b/arch/x86/video/fbdev.c
> > > index d5644bb..4999f78 100644
> > > --- a/arch/x86/video/fbdev.c
> > > +++ b/arch/x86/video/fbdev.c
> > > @@ -18,11 +18,12 @@ int fb_is_primary_device(struct fb_info *info)
> > > struct pci_dev *default_device = vga_default_device();
> > > struct resource *res = NULL;
> > >
> > > - if (device)
> > > - pci_dev = to_pci_dev(device);
> > > -
> > > - if (!pci_dev)
> > > + if (!device || !device->bus ||
> > > + !device->bus->name || strcmp(device->bus->name, "pci")) {
> > > return 0;
> > > + }
> > > +
> > > + pci_dev = to_pci_dev(device);
> > >
> > > if (default_device) {
> > > if (pci_dev == default_device)
> > > --
> > > 1.9.1
> > >
>
> I wonder if this issue could explain some of the efifb issues we've
> seen reported on bugzilla.kernel.org in the past where switching from
> efifb to some other framebuffer device caused hangs during boot. I'm
> struggling to find the relevant bugzilla entries now, though.
It's possible it could, but I don't have them handy either. I've also
wondered if some of them were due to bad data from the firmware - at
plugfests we've seen some cases where the actual video mode as measured
with a ruler is clearly not what the firmware claims it to be, so it's
entirely possible we're occasionally told a memory region that is not
what's actually mapped, or that's mapped but is only partially backed
by the actual frame buffer memory.
But aside from that diversion, I think Alexander has a legitimate
question about use of to_pci_dev(). If I ask the question: can we fix
this in efifb by making it live on a pci_dev, I have a couple of
fundamental problems:
1) technically it doesn't have to be a pci_dev at all (but, practically,
so far it always is on PCI...)
2) From EFI, we can't necessarily pin it down to a single PCI device
even if it is PCI. Before we do EFI's ExitBootServices() call, we
can try to find the PCI_IO handle our GOP instance is connected to,
but not all firmware GOP drivers use that, so it doesn't always work.
And even if it did, there can be more than one instance pointing to
the same memory with different PCI devices - lots of laptops have
this sort of thing.
3) Ignoring the EFI side and just focusing on PCI, if there's two
devices configured that could do scanout, it can be mapped to one
device's BAR but the other device be the actual device using it. In
this case either choice is probably wrong for something, and the
things that have the information to resolve which one don't include
efifb - they're the drivers we'll likely hand off to later.
So it's most likely right for efifb to be embedded in a platform_device
instead of a pci_dev. Which leads back to Alexander's question - if it
isn't in a pci_dev, that means fb_is_primary_device() needs to not
assume it is. So the patch appears correct, but so is the question -
should to_pci_dev() be checking this and returning NULL here?
--
Peter
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC 1/1] x86: fix bad memory access in fb_is_primary_device()
2016-02-16 15:18 ` Peter Jones
@ 2016-03-09 12:46 ` Alexander Popov
2016-03-25 11:29 ` Alexander Popov
0 siblings, 1 reply; 9+ messages in thread
From: Alexander Popov @ 2016-03-09 12:46 UTC (permalink / raw)
To: Peter Jones, Matt Fleming
Cc: Ingo Molnar, Arnd Bergmann, Tomi Valkeinen, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, x86, linux-kernel, linux-efi,
Alexander Popov
On 16.02.2016 18:18, Peter Jones wrote:
> On Tue, Feb 16, 2016 at 01:49:18PM +0000, Matt Fleming wrote:
>> [ Including Peter, the efifb maintainer. Original email is here,
>>
>> http://marc.info/?l=linux-kernel&m=145552936131335&w=2
>>
>> I've snipped some of the quoted text ]
>>
>> On Tue, 16 Feb, at 08:55:22AM, Ingo Molnar wrote:
>>>
>>> (I've Cc:-ed the EFI-FB and FB gents. Mail quoted below.)
>>>
>>> * Alexander Popov <alpopov@ptsecurity.com> wrote:
>>>
>>>> Currently the code in fb_is_primary_device() contains to_pci_dev() macro
>>>> which is applied to dev from struct fb_info. In some cases this causes
>>>> bad memory access when fb_is_primary_device() handles fb_info of efifb.
>>>> The reason is that fb dev of efifb is embedded into struct platform_device
>>>> but not into struct pci_dev.
>>>>
>>>> We can fix this by checking fb dev bus name in fb_is_primary_device().
>>>>
>>>> It seems that this bug reveals some bigger problem with to_pci_dev(),
>>>> to_platform_device() and others, which just do container_of() and
>>>> don't check whether struct device is a part of the appropriate structure.
>>>> Should we do something more about it?
>>>>
>>>> KASan report:
>>
>> [...]
>>
>>>>
>>>> Signed-off-by: Alexander Popov <alpopov@ptsecurity.com>
>>>> ---
>>>> arch/x86/video/fbdev.c | 9 +++++----
>>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/x86/video/fbdev.c b/arch/x86/video/fbdev.c
>>>> index d5644bb..4999f78 100644
>>>> --- a/arch/x86/video/fbdev.c
>>>> +++ b/arch/x86/video/fbdev.c
>>>> @@ -18,11 +18,12 @@ int fb_is_primary_device(struct fb_info *info)
>>>> struct pci_dev *default_device = vga_default_device();
>>>> struct resource *res = NULL;
>>>>
>>>> - if (device)
>>>> - pci_dev = to_pci_dev(device);
>>>> -
>>>> - if (!pci_dev)
>>>> + if (!device || !device->bus ||
>>>> + !device->bus->name || strcmp(device->bus->name, "pci")) {
>>>> return 0;
>>>> + }
>>>> +
>>>> + pci_dev = to_pci_dev(device);
>>>>
>>>> if (default_device) {
>>>> if (pci_dev == default_device)
>>>> --
>>>> 1.9.1
>>>>
>>
>> I wonder if this issue could explain some of the efifb issues we've
>> seen reported on bugzilla.kernel.org in the past where switching from
>> efifb to some other framebuffer device caused hangs during boot. I'm
>> struggling to find the relevant bugzilla entries now, though.
>
> It's possible it could, but I don't have them handy either. I've also
> wondered if some of them were due to bad data from the firmware - at
> plugfests we've seen some cases where the actual video mode as measured
> with a ruler is clearly not what the firmware claims it to be, so it's
> entirely possible we're occasionally told a memory region that is not
> what's actually mapped, or that's mapped but is only partially backed
> by the actual frame buffer memory.
>
> But aside from that diversion, I think Alexander has a legitimate
> question about use of to_pci_dev(). If I ask the question: can we fix
> this in efifb by making it live on a pci_dev, I have a couple of
> fundamental problems:
>
> 1) technically it doesn't have to be a pci_dev at all (but, practically,
> so far it always is on PCI...)
> 2) From EFI, we can't necessarily pin it down to a single PCI device
> even if it is PCI. Before we do EFI's ExitBootServices() call, we
> can try to find the PCI_IO handle our GOP instance is connected to,
> but not all firmware GOP drivers use that, so it doesn't always work.
> And even if it did, there can be more than one instance pointing to
> the same memory with different PCI devices - lots of laptops have
> this sort of thing.
> 3) Ignoring the EFI side and just focusing on PCI, if there's two
> devices configured that could do scanout, it can be mapped to one
> device's BAR but the other device be the actual device using it. In
> this case either choice is probably wrong for something, and the
> things that have the information to resolve which one don't include
> efifb - they're the drivers we'll likely hand off to later.
>
> So it's most likely right for efifb to be embedded in a platform_device
> instead of a pci_dev. Which leads back to Alexander's question - if it
> isn't in a pci_dev, that means fb_is_primary_device() needs to not
> assume it is. So the patch appears correct, but so is the question -
> should to_pci_dev() be checking this and returning NULL here?
The discussion has suspended. May I activate it again?
So there are two ways to fix the bad memory access in fb_is_primary_device().
The first one is proposed in my patch. Checking the bus name string doesn't
look good but I didn't manage to come up with anything better.
The second way is changing to_pci_dev() similarly. It may return NULL or
call BUG() when struct device is a part of an inappropriate structure.
Which way is better? Do we need to do anything with other similar macros?
Thanks.
Best regards,
Alexander
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC 1/1] x86: fix bad memory access in fb_is_primary_device()
2016-03-09 12:46 ` Alexander Popov
@ 2016-03-25 11:29 ` Alexander Popov
2016-03-29 11:53 ` Matt Fleming
2016-03-31 13:31 ` Ingo Molnar
0 siblings, 2 replies; 9+ messages in thread
From: Alexander Popov @ 2016-03-25 11:29 UTC (permalink / raw)
To: Peter Jones, Matt Fleming, Ingo Molnar, Arnd Bergmann,
Tomi Valkeinen, Thomas Gleixner, Ingo Molnar, H. Peter Anvin
Cc: x86, linux-kernel, linux-efi, Alexander Popov
On 09.03.2016 15:46, Alexander Popov wrote:
> On 16.02.2016 18:18, Peter Jones wrote:
>> On Tue, Feb 16, 2016 at 01:49:18PM +0000, Matt Fleming wrote:
>>> [ Including Peter, the efifb maintainer. Original email is here,
>>>
>>> http://marc.info/?l=linux-kernel&m=145552936131335&w=2
>>>
>>> I've snipped some of the quoted text ]
>>>
>>> On Tue, 16 Feb, at 08:55:22AM, Ingo Molnar wrote:
>>>>
>>>> (I've Cc:-ed the EFI-FB and FB gents. Mail quoted below.)
>>>>
>>>> * Alexander Popov <alpopov@ptsecurity.com> wrote:
>>>>
>>>>> Currently the code in fb_is_primary_device() contains to_pci_dev() macro
>>>>> which is applied to dev from struct fb_info. In some cases this causes
>>>>> bad memory access when fb_is_primary_device() handles fb_info of efifb.
>>>>> The reason is that fb dev of efifb is embedded into struct platform_device
>>>>> but not into struct pci_dev.
>>>>>
>>>>> We can fix this by checking fb dev bus name in fb_is_primary_device().
>>>>>
>>>>> It seems that this bug reveals some bigger problem with to_pci_dev(),
>>>>> to_platform_device() and others, which just do container_of() and
>>>>> don't check whether struct device is a part of the appropriate structure.
>>>>> Should we do something more about it?
>>>>>
>>>>> KASan report:
>>>
>>> [...]
>>>
>>>>>
>>>>> Signed-off-by: Alexander Popov <alpopov@ptsecurity.com>
>>>>> ---
>>>>> arch/x86/video/fbdev.c | 9 +++++----
>>>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/video/fbdev.c b/arch/x86/video/fbdev.c
>>>>> index d5644bb..4999f78 100644
>>>>> --- a/arch/x86/video/fbdev.c
>>>>> +++ b/arch/x86/video/fbdev.c
>>>>> @@ -18,11 +18,12 @@ int fb_is_primary_device(struct fb_info *info)
>>>>> struct pci_dev *default_device = vga_default_device();
>>>>> struct resource *res = NULL;
>>>>>
>>>>> - if (device)
>>>>> - pci_dev = to_pci_dev(device);
>>>>> -
>>>>> - if (!pci_dev)
>>>>> + if (!device || !device->bus ||
>>>>> + !device->bus->name || strcmp(device->bus->name, "pci")) {
>>>>> return 0;
>>>>> + }
>>>>> +
>>>>> + pci_dev = to_pci_dev(device);
>>>>>
>>>>> if (default_device) {
>>>>> if (pci_dev == default_device)
>>>>> --
>>>>> 1.9.1
>>>>>
>>>
>>> I wonder if this issue could explain some of the efifb issues we've
>>> seen reported on bugzilla.kernel.org in the past where switching from
>>> efifb to some other framebuffer device caused hangs during boot. I'm
>>> struggling to find the relevant bugzilla entries now, though.
>>
>> It's possible it could, but I don't have them handy either.
[...]
>> So it's most likely right for efifb to be embedded in a platform_device
>> instead of a pci_dev. Which leads back to Alexander's question - if it
>> isn't in a pci_dev, that means fb_is_primary_device() needs to not
>> assume it is. So the patch appears correct, but so is the question -
>> should to_pci_dev() be checking this and returning NULL here?
>
> The discussion has suspended. May I activate it again?
>
> So there are two ways to fix the bad memory access in fb_is_primary_device().
>
> The first one is proposed in my patch. Checking the bus name string doesn't
> look good but I didn't manage to come up with anything better.
>
> The second way is changing to_pci_dev() similarly. It may return NULL or
> call BUG() when struct device is a part of an inappropriate structure.
>
> Which way is better? Do we need to do anything with other similar macros?
Excuse me, there is no reply for a long time. Did I touch any taboo topic?
Hope to fix this bug. Thanks.
Best regards,
Alexander
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC 1/1] x86: fix bad memory access in fb_is_primary_device()
2016-03-25 11:29 ` Alexander Popov
@ 2016-03-29 11:53 ` Matt Fleming
2016-03-29 12:12 ` Matt Fleming
2016-03-31 13:31 ` Ingo Molnar
1 sibling, 1 reply; 9+ messages in thread
From: Matt Fleming @ 2016-03-29 11:53 UTC (permalink / raw)
To: Alexander Popov
Cc: Peter Jones, Ingo Molnar, Arnd Bergmann, Tomi Valkeinen,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
linux-efi
On Fri, 25 Mar, at 02:29:21PM, Alexander Popov wrote:
>
> Excuse me, there is no reply for a long time. Did I touch any taboo topic?
> Hope to fix this bug. Thanks.
Unless anyone complains soonish I'll send this to tip as part of the
EFI urgent queue.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC 1/1] x86: fix bad memory access in fb_is_primary_device()
2016-03-29 11:53 ` Matt Fleming
@ 2016-03-29 12:12 ` Matt Fleming
0 siblings, 0 replies; 9+ messages in thread
From: Matt Fleming @ 2016-03-29 12:12 UTC (permalink / raw)
To: Alexander Popov
Cc: Peter Jones, Ingo Molnar, Arnd Bergmann, Tomi Valkeinen,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
linux-efi, Vitaly Kuznetsov
On Tue, 29 Mar, at 12:53:47PM, Matt Fleming wrote:
> On Fri, 25 Mar, at 02:29:21PM, Alexander Popov wrote:
> >
> > Excuse me, there is no reply for a long time. Did I touch any taboo topic?
> > Hope to fix this bug. Thanks.
>
> Unless anyone complains soonish I'll send this to tip as part of the
> EFI urgent queue.
Oh, looks like this issue was already fixed here,
https://lkml.kernel.org/r/tip-743146db071c4a828159211a295d12ff4f61752f@git.kernel.org
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFC 1/1] x86: fix bad memory access in fb_is_primary_device()
2016-03-25 11:29 ` Alexander Popov
2016-03-29 11:53 ` Matt Fleming
@ 2016-03-31 13:31 ` Ingo Molnar
1 sibling, 0 replies; 9+ messages in thread
From: Ingo Molnar @ 2016-03-31 13:31 UTC (permalink / raw)
To: Alexander Popov
Cc: Peter Jones, Matt Fleming, Arnd Bergmann, Tomi Valkeinen,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel,
linux-efi
* Alexander Popov <alpopov@ptsecurity.com> wrote:
> On 09.03.2016 15:46, Alexander Popov wrote:
> > On 16.02.2016 18:18, Peter Jones wrote:
> >> On Tue, Feb 16, 2016 at 01:49:18PM +0000, Matt Fleming wrote:
> >>> [ Including Peter, the efifb maintainer. Original email is here,
> >>>
> >>> http://marc.info/?l=linux-kernel&m=145552936131335&w=2
> >>>
> >>> I've snipped some of the quoted text ]
> >>>
> >>> On Tue, 16 Feb, at 08:55:22AM, Ingo Molnar wrote:
> >>>>
> >>>> (I've Cc:-ed the EFI-FB and FB gents. Mail quoted below.)
> >>>>
> >>>> * Alexander Popov <alpopov@ptsecurity.com> wrote:
> >>>>
> >>>>> Currently the code in fb_is_primary_device() contains to_pci_dev() macro
> >>>>> which is applied to dev from struct fb_info. In some cases this causes
> >>>>> bad memory access when fb_is_primary_device() handles fb_info of efifb.
> >>>>> The reason is that fb dev of efifb is embedded into struct platform_device
> >>>>> but not into struct pci_dev.
> >>>>>
> >>>>> We can fix this by checking fb dev bus name in fb_is_primary_device().
> >>>>>
> >>>>> It seems that this bug reveals some bigger problem with to_pci_dev(),
> >>>>> to_platform_device() and others, which just do container_of() and
> >>>>> don't check whether struct device is a part of the appropriate structure.
> >>>>> Should we do something more about it?
> >>>>>
> >>>>> KASan report:
> >>>
> >>> [...]
> >>>
> >>>>>
> >>>>> Signed-off-by: Alexander Popov <alpopov@ptsecurity.com>
> >>>>> ---
> >>>>> arch/x86/video/fbdev.c | 9 +++++----
> >>>>> 1 file changed, 5 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/x86/video/fbdev.c b/arch/x86/video/fbdev.c
> >>>>> index d5644bb..4999f78 100644
> >>>>> --- a/arch/x86/video/fbdev.c
> >>>>> +++ b/arch/x86/video/fbdev.c
> >>>>> @@ -18,11 +18,12 @@ int fb_is_primary_device(struct fb_info *info)
> >>>>> struct pci_dev *default_device = vga_default_device();
> >>>>> struct resource *res = NULL;
> >>>>>
> >>>>> - if (device)
> >>>>> - pci_dev = to_pci_dev(device);
> >>>>> -
> >>>>> - if (!pci_dev)
> >>>>> + if (!device || !device->bus ||
> >>>>> + !device->bus->name || strcmp(device->bus->name, "pci")) {
> >>>>> return 0;
> >>>>> + }
> >>>>> +
> >>>>> + pci_dev = to_pci_dev(device);
> >>>>>
> >>>>> if (default_device) {
> >>>>> if (pci_dev == default_device)
> >>>>> --
> >>>>> 1.9.1
> >>>>>
> >>>
> >>> I wonder if this issue could explain some of the efifb issues we've
> >>> seen reported on bugzilla.kernel.org in the past where switching from
> >>> efifb to some other framebuffer device caused hangs during boot. I'm
> >>> struggling to find the relevant bugzilla entries now, though.
> >>
> >> It's possible it could, but I don't have them handy either.
>
> [...]
>
> >> So it's most likely right for efifb to be embedded in a platform_device
> >> instead of a pci_dev. Which leads back to Alexander's question - if it
> >> isn't in a pci_dev, that means fb_is_primary_device() needs to not
> >> assume it is. So the patch appears correct, but so is the question -
> >> should to_pci_dev() be checking this and returning NULL here?
> >
> > The discussion has suspended. May I activate it again?
> >
> > So there are two ways to fix the bad memory access in fb_is_primary_device().
> >
> > The first one is proposed in my patch. Checking the bus name string doesn't
> > look good but I didn't manage to come up with anything better.
> >
> > The second way is changing to_pci_dev() similarly. It may return NULL or
> > call BUG() when struct device is a part of an inappropriate structure.
> >
> > Which way is better? Do we need to do anything with other similar macros?
>
> Excuse me, there is no reply for a long time. Did I touch any taboo topic?
> Hope to fix this bug. Thanks.
No need to worry, it's all upstream already.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-03-31 13:31 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-15 9:30 [PATCH RFC 1/1] x86: fix bad memory access in fb_is_primary_device() Alexander Popov
2016-02-16 7:55 ` Ingo Molnar
2016-02-16 13:49 ` Matt Fleming
2016-02-16 15:18 ` Peter Jones
2016-03-09 12:46 ` Alexander Popov
2016-03-25 11:29 ` Alexander Popov
2016-03-29 11:53 ` Matt Fleming
2016-03-29 12:12 ` Matt Fleming
2016-03-31 13:31 ` Ingo Molnar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).