| Message ID | 20191114114532.31551-1-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/ast: Fix potential NULL-pointer read during atomic mode setting | expand |
On Thu, Nov 14, 2019 at 12:45:32PM +0100, Thomas Zimmermann wrote: > When enabling the CRTC after waking up from a power-saving mode, the > primary plane's framebuffer might be NULL, which leads to a stack trace > as shown below. > > [ 632.624608] BUG: kernel NULL pointer dereference, address: 0000000000000048 > [ 632.624631] #PF: supervisor read access in kernel mode > [ 632.624639] #PF: error_code(0x0000) - not-present page > [ 632.624647] PGD 0 P4D 0 > [ 632.624654] Oops: 0000 [#1] SMP PTI > [ 632.624662] CPU: 0 PID: 2082 Comm: gnome-shell Tainted: G E 5.4.0-rc7-1-default+ #114 > [ 632.624673] Hardware name: Sun Microsystems SUN FIRE X2270 M2/SUN FIRE X2270 M2, BIOS 2.05 07/01/2010 > [ 632.624689] RIP: 0010:ast_crtc_helper_atomic_enable+0x7d/0x680 [ast] > [ 632.624698] Code: 48 8b 80 e0 02 00 00 4c 8b 60 10 31 c0 f3 48 ab 48 8b 83 78 04 00 00 4c 89 ef 48 8d 70 18 e8 9a e9 55 ce 48 8b 83 78 04 00 00 <49> 8b 7c 24 48 4c 89 ea 4c 8d 44 24 28 48 8d 4c 24 20 48 8d 70 18 > [ 632.624718] RSP: 0018:ffffbe9ec123fa40 EFLAGS: 00010246 > [ 632.624726] RAX: ffff95a13cfd3400 RBX: ffff95a13cf32000 RCX: 0000000000000000 > [ 632.624735] RDX: 0000000000000000 RSI: ffff95a13cfd34e8 RDI: ffffbe9ec123fb40 > [ 632.624744] RBP: ffffbe9ec123fb80 R08: 0000000000000000 R09: 0000000000000003 > [ 632.624753] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 > [ 632.624762] R13: ffffbe9ec123fa70 R14: ffff95a13beb7000 R15: ffff95a13cf32800 > [ 632.624772] FS: 00007f6d2763e140(0000) GS:ffff95a134000000(0000) knlGS:0000000000000000 > [ 632.624782] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 632.624790] CR2: 0000000000000048 CR3: 00000001192f8004 CR4: 00000000000206f0 > [ 632.624800] Call Trace: > [ 632.624811] ? __lock_acquire+0x409/0x7c0 > [ 632.624830] drm_atomic_helper_commit_modeset_enables+0x1af/0x200 > [ 632.624840] drm_atomic_helper_commit_tail+0x32/0x70 > [ 632.624849] commit_tail+0xc7/0x110 > [ 632.624857] drm_atomic_helper_commit+0x121/0x130 > [ 632.624867] drm_atomic_connector_commit_dpms+0xd7/0x100 > [ 632.624878] set_property_atomic+0xaf/0x110 > [ 632.624890] drm_mode_obj_set_property_ioctl+0xbb/0x190 > [ 632.624899] ? drm_mode_obj_find_prop_id+0x40/0x40 > [ 632.624909] drm_ioctl_kernel+0x86/0xd0 > [ 632.624918] drm_ioctl+0x1e4/0x36b > [ 632.624925] ? drm_mode_obj_find_prop_id+0x40/0x40 > [ 632.624939] do_vfs_ioctl+0x4bd/0x6e0 > [ 632.624949] ksys_ioctl+0x5e/0x90 > [ 632.624957] __x64_sys_ioctl+0x16/0x20 > [ 632.624966] do_syscall_64+0x5a/0x220 > [ 632.624976] entry_SYSCALL_64_after_hwframe+0x49/0xbe > [ 632.624984] RIP: 0033:0x7f6d2b0de387 > [ 632.624991] Code: 00 00 90 48 8b 05 f9 9a 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c9 9a 0c 00 f7 d8 64 89 01 48 > [ 632.625011] RSP: 002b:00007fffb49def38 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 > [ 632.625021] RAX: ffffffffffffffda RBX: 00007fffb49def70 RCX: 00007f6d2b0de387 > [ 632.625030] RDX: 00007fffb49def70 RSI: 00000000c01864ba RDI: 0000000000000009 > [ 632.625040] RBP: 00000000c01864ba R08: 0000000000000000 R09: 00000000c0c0c0c0 > [ 632.625049] R10: 0000000000000030 R11: 0000000000000246 R12: 000055bc367eb920 > [ 632.625058] R13: 0000000000000009 R14: 0000000000000002 R15: 0000000000000000 > [ 632.625071] Modules linked in: ebtable_filter(E) ebtables(E) ip6table_filter(E) ip6_tables(E) iptable_filter(E) ip_tables(E) x_tables(E) af_packet(E) scsi_transport_iscsi(E) dmi_sysfs(E) msr(E) xfs(E) intel_powerclamp(E) coretemp(E) k) > [ 632.625185] CR2: 0000000000000048 > > The STR is > > * start gdm and wait for it to switch off the display > * wake up the display by pressing a key > > The fix avoids dereferencing the pointer and postpones modesetting until > there's a framebuffer bound to the CRTC. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Fixes: b48e1b6ffd28 ("drm/ast: Add CRTC helpers for atomic modesetting") > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Dave Airlie <airlied@redhat.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: "Y.C. Chen" <yc_chen@aspeedtech.com> > Cc: Sam Ravnborg <sam@ravnborg.org> This looks like papering over the real bug, which is lack of atomic_check in the primary plane. You probably want to call drm_atomic_helper_check_plane_state similar to what the simpler kms helper does. That should stop this kind of stuff. Specifically you then want to check whether your primary plane is visible (compute into plane_state->visible) and reject the atomic commit if that's not the case. -Daniel > --- > drivers/gpu/drm/ast/ast_mode.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c > index 4725ec911a66..b173a8e5c00b 100644 > --- a/drivers/gpu/drm/ast/ast_mode.c > +++ b/drivers/gpu/drm/ast/ast_mode.c > @@ -774,7 +774,8 @@ static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc, > &adjusted_mode, &vbios_mode); > if (!succ) > return -EINVAL; > - } > + } else > + state->mode_changed = true; > > return 0; > } > @@ -824,6 +825,9 @@ ast_crtc_helper_atomic_enable(struct drm_crtc *crtc, > struct ast_vbios_mode_info vbios_mode; > bool succ; > > + if (!fb) > + return; > + > memset(&adjusted_mode, 0, sizeof(adjusted_mode)); > drm_mode_copy(&adjusted_mode, &crtc->state->adjusted_mode); > > @@ -832,6 +836,13 @@ ast_crtc_helper_atomic_enable(struct drm_crtc *crtc, > if (WARN_ON_ONCE(!succ)) > return; > > + /* > + * TODO: Some of the registers' values depend on the framebuffer > + * configuration. To resolve this, fully separate the mode > + * setting from the framebuffer update. The vbios mode info > + * should be gathered by atomic_check, the framebuffer regs > + * should be set in primary plane's update function. > + */ > ast_set_vbios_mode_reg(crtc, &adjusted_mode, &vbios_mode); > ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x06); > ast_set_std_reg(crtc, &adjusted_mode, &vbios_mode); > -- > 2.23.0 >
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 4725ec911a66..b173a8e5c00b 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -774,7 +774,8 @@ static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc, &adjusted_mode, &vbios_mode); if (!succ) return -EINVAL; - } + } else + state->mode_changed = true; return 0; } @@ -824,6 +825,9 @@ ast_crtc_helper_atomic_enable(struct drm_crtc *crtc, struct ast_vbios_mode_info vbios_mode; bool succ; + if (!fb) + return; + memset(&adjusted_mode, 0, sizeof(adjusted_mode)); drm_mode_copy(&adjusted_mode, &crtc->state->adjusted_mode); @@ -832,6 +836,13 @@ ast_crtc_helper_atomic_enable(struct drm_crtc *crtc, if (WARN_ON_ONCE(!succ)) return; + /* + * TODO: Some of the registers' values depend on the framebuffer + * configuration. To resolve this, fully separate the mode + * setting from the framebuffer update. The vbios mode info + * should be gathered by atomic_check, the framebuffer regs + * should be set in primary plane's update function. + */ ast_set_vbios_mode_reg(crtc, &adjusted_mode, &vbios_mode); ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x06); ast_set_std_reg(crtc, &adjusted_mode, &vbios_mode);
When enabling the CRTC after waking up from a power-saving mode, the primary plane's framebuffer might be NULL, which leads to a stack trace as shown below. [ 632.624608] BUG: kernel NULL pointer dereference, address: 0000000000000048 [ 632.624631] #PF: supervisor read access in kernel mode [ 632.624639] #PF: error_code(0x0000) - not-present page [ 632.624647] PGD 0 P4D 0 [ 632.624654] Oops: 0000 [#1] SMP PTI [ 632.624662] CPU: 0 PID: 2082 Comm: gnome-shell Tainted: G E 5.4.0-rc7-1-default+ #114 [ 632.624673] Hardware name: Sun Microsystems SUN FIRE X2270 M2/SUN FIRE X2270 M2, BIOS 2.05 07/01/2010 [ 632.624689] RIP: 0010:ast_crtc_helper_atomic_enable+0x7d/0x680 [ast] [ 632.624698] Code: 48 8b 80 e0 02 00 00 4c 8b 60 10 31 c0 f3 48 ab 48 8b 83 78 04 00 00 4c 89 ef 48 8d 70 18 e8 9a e9 55 ce 48 8b 83 78 04 00 00 <49> 8b 7c 24 48 4c 89 ea 4c 8d 44 24 28 48 8d 4c 24 20 48 8d 70 18 [ 632.624718] RSP: 0018:ffffbe9ec123fa40 EFLAGS: 00010246 [ 632.624726] RAX: ffff95a13cfd3400 RBX: ffff95a13cf32000 RCX: 0000000000000000 [ 632.624735] RDX: 0000000000000000 RSI: ffff95a13cfd34e8 RDI: ffffbe9ec123fb40 [ 632.624744] RBP: ffffbe9ec123fb80 R08: 0000000000000000 R09: 0000000000000003 [ 632.624753] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 [ 632.624762] R13: ffffbe9ec123fa70 R14: ffff95a13beb7000 R15: ffff95a13cf32800 [ 632.624772] FS: 00007f6d2763e140(0000) GS:ffff95a134000000(0000) knlGS:0000000000000000 [ 632.624782] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 632.624790] CR2: 0000000000000048 CR3: 00000001192f8004 CR4: 00000000000206f0 [ 632.624800] Call Trace: [ 632.624811] ? __lock_acquire+0x409/0x7c0 [ 632.624830] drm_atomic_helper_commit_modeset_enables+0x1af/0x200 [ 632.624840] drm_atomic_helper_commit_tail+0x32/0x70 [ 632.624849] commit_tail+0xc7/0x110 [ 632.624857] drm_atomic_helper_commit+0x121/0x130 [ 632.624867] drm_atomic_connector_commit_dpms+0xd7/0x100 [ 632.624878] set_property_atomic+0xaf/0x110 [ 632.624890] drm_mode_obj_set_property_ioctl+0xbb/0x190 [ 632.624899] ? drm_mode_obj_find_prop_id+0x40/0x40 [ 632.624909] drm_ioctl_kernel+0x86/0xd0 [ 632.624918] drm_ioctl+0x1e4/0x36b [ 632.624925] ? drm_mode_obj_find_prop_id+0x40/0x40 [ 632.624939] do_vfs_ioctl+0x4bd/0x6e0 [ 632.624949] ksys_ioctl+0x5e/0x90 [ 632.624957] __x64_sys_ioctl+0x16/0x20 [ 632.624966] do_syscall_64+0x5a/0x220 [ 632.624976] entry_SYSCALL_64_after_hwframe+0x49/0xbe [ 632.624984] RIP: 0033:0x7f6d2b0de387 [ 632.624991] Code: 00 00 90 48 8b 05 f9 9a 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c9 9a 0c 00 f7 d8 64 89 01 48 [ 632.625011] RSP: 002b:00007fffb49def38 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 [ 632.625021] RAX: ffffffffffffffda RBX: 00007fffb49def70 RCX: 00007f6d2b0de387 [ 632.625030] RDX: 00007fffb49def70 RSI: 00000000c01864ba RDI: 0000000000000009 [ 632.625040] RBP: 00000000c01864ba R08: 0000000000000000 R09: 00000000c0c0c0c0 [ 632.625049] R10: 0000000000000030 R11: 0000000000000246 R12: 000055bc367eb920 [ 632.625058] R13: 0000000000000009 R14: 0000000000000002 R15: 0000000000000000 [ 632.625071] Modules linked in: ebtable_filter(E) ebtables(E) ip6table_filter(E) ip6_tables(E) iptable_filter(E) ip_tables(E) x_tables(E) af_packet(E) scsi_transport_iscsi(E) dmi_sysfs(E) msr(E) xfs(E) intel_powerclamp(E) coretemp(E) k) [ 632.625185] CR2: 0000000000000048 The STR is * start gdm and wait for it to switch off the display * wake up the display by pressing a key The fix avoids dereferencing the pointer and postpones modesetting until there's a framebuffer bound to the CRTC. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Fixes: b48e1b6ffd28 ("drm/ast: Add CRTC helpers for atomic modesetting") Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Dave Airlie <airlied@redhat.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: "Y.C. Chen" <yc_chen@aspeedtech.com> Cc: Sam Ravnborg <sam@ravnborg.org> --- drivers/gpu/drm/ast/ast_mode.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)