[PATCH v2] drm/ast: Fix potential NULL-pointer read during atomic mode setting
Thomas Zimmermann
tzimmermann at suse.de
Wed Nov 27 12:31:25 UTC 2019
Hi Daniel
Am 27.11.19 um 11:49 schrieb Daniel Vetter:
> On Wed, Nov 27, 2019 at 08:31:09AM +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 implements atomic_check for planes and rejects configurations
>> with an invisible primary plane.
>>
>> v2:
>> * do an atomic check for plane
>> * reject invisible primary planes
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> Suggested-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Fixes: b48e1b6ffd28 ("drm/ast: Add CRTC helpers for atomic modesetting")
>> Cc: Gerd Hoffmann <kraxel at redhat.com>
>> Cc: Dave Airlie <airlied at redhat.com>
>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Cc: "Y.C. Chen" <yc_chen at aspeedtech.com>
>> Cc: Sam Ravnborg <sam at ravnborg.org>
>> ---
>> drivers/gpu/drm/ast/ast_mode.c | 50 +++++++++++++++++++++++++++++++++-
>> 1 file changed, 49 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>> index 4725ec911a66..8e7bb8ce8130 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -31,6 +31,7 @@
>> #include <linux/export.h>
>> #include <linux/pci.h>
>>
>> +#include <drm/drm_atomic.h>
>> #include <drm/drm_atomic_helper.h>
>> #include <drm/drm_atomic_state_helper.h>
>> #include <drm/drm_crtc.h>
>> @@ -556,6 +557,31 @@ static const uint32_t ast_primary_plane_formats[] = {
>> int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
>> struct drm_plane_state *state)
>> {
>> + struct drm_crtc *crtc;
>> + struct drm_crtc_state *crtc_state;
>> + int ret;
>> +
>> + if (state->crtc)
>> + crtc = state->crtc;
>> + else if (plane->state && plane->state->crtc)
>> + crtc = plane->state->crtc;
>> + else
>> + return 0; /* disabling an already disabled plane */
>> +
>> + crtc_state = drm_atomic_get_new_crtc_state(state->state, crtc);
>> + if (WARN_ON_ONCE(!crtc_state))
>> + return -EINVAL; /* BUG: no new CRTC state allocated */
>
> The above is a lot more complicated than necessary, see e.g.
> atmel_hlcdc_plane_atomic_check. And even that is too complicated, since
> crtc is set iff fb is set, you only need to check one of them.
Thanks for your review. This comment sounds as if it's at the wrong
position?
Best regards
Thomas
>
>> +
>> + ret = drm_atomic_helper_check_plane_state(state, crtc_state,
>> + DRM_PLANE_HELPER_NO_SCALING,
>> + DRM_PLANE_HELPER_NO_SCALING,
>> + false, true);
>> + if (ret)
>> + return ret;
>> +
>> + if (!state->visible)
>> + return -EINVAL; /* primary plane must be visible */
>> +
>> return 0;
>> }
>>
>> @@ -567,7 +593,7 @@ void ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
>> struct drm_gem_vram_object *gbo;
>> s64 gpu_addr;
>>
>> - if (!crtc || !state->fb)
>> + if (!crtc || !state->fb || !state->visible)
>> return;
>>
>> gbo = drm_gem_vram_of_gem(state->fb->obj[0]);
>> @@ -660,6 +686,28 @@ ast_cursor_plane_helper_prepare_fb(struct drm_plane *plane,
>> static int ast_cursor_plane_helper_atomic_check(struct drm_plane *plane,
>> struct drm_plane_state *state)
>> {
>> + struct drm_crtc *crtc;
>> + struct drm_crtc_state *crtc_state;
>> + int ret;
>> +
>> + if (state->crtc)
>> + crtc = state->crtc;
>> + else if (plane->state && plane->state->crtc)
>> + crtc = plane->state->crtc;
>> + else
>> + return 0; /* disabling an already disabled plane */
>> +
>> + crtc_state = drm_atomic_get_new_crtc_state(state->state, crtc);
>> + if (WARN_ON_ONCE(!crtc_state))
>> + return -EINVAL; /* BUG: no new CRTC state allocated */
>> +
>> + ret = drm_atomic_helper_check_plane_state(state, crtc_state,
>> + DRM_PLANE_HELPER_NO_SCALING,
>> + DRM_PLANE_HELPER_NO_SCALING,
>> + false, true);
>
> Pretty sure you want your cursor to be positionable ...
> -Daniel
>
>> + if (ret)
>> + return ret;
>> +
>> return 0;
>> }
>>
>> --
>> 2.23.0
>>
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20191127/25eb6545/attachment-0001.sig>
More information about the dri-devel
mailing list