* [PATCH v2] drm/ast: Fix potential NULL-pointer read during atomic mode setting
@ 2019-11-27 7:31 Thomas Zimmermann
2019-11-27 10:49 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Zimmermann @ 2019-11-27 7:31 UTC (permalink / raw)
To: kraxel, airlied, daniel.vetter, yc_chen, sam; +Cc: Thomas Zimmermann, dri-devel
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@suse.de>
Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
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 | 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 */
+
+ 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);
+ if (ret)
+ return ret;
+
return 0;
}
--
2.23.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] drm/ast: Fix potential NULL-pointer read during atomic mode setting
2019-11-27 7:31 [PATCH v2] drm/ast: Fix potential NULL-pointer read during atomic mode setting Thomas Zimmermann
@ 2019-11-27 10:49 ` Daniel Vetter
2019-11-27 12:31 ` Thomas Zimmermann
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2019-11-27 10:49 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: daniel.vetter, dri-devel, kraxel, airlied, sam
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@suse.de>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 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 | 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.
> +
> + 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] drm/ast: Fix potential NULL-pointer read during atomic mode setting
2019-11-27 10:49 ` Daniel Vetter
@ 2019-11-27 12:31 ` Thomas Zimmermann
2019-11-27 12:38 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Zimmermann @ 2019-11-27 12:31 UTC (permalink / raw)
To: Daniel Vetter; +Cc: daniel.vetter, dri-devel, kraxel, airlied, sam
[-- Attachment #1.1.1: Type: text/plain, Size: 8131 bytes --]
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@suse.de>
>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> 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 | 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
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] drm/ast: Fix potential NULL-pointer read during atomic mode setting
2019-11-27 12:31 ` Thomas Zimmermann
@ 2019-11-27 12:38 ` Daniel Vetter
2019-11-27 12:43 ` Thomas Zimmermann
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2019-11-27 12:38 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: daniel.vetter, dri-devel, kraxel, airlied, sam
On Wed, Nov 27, 2019 at 01:31:25PM +0100, Thomas Zimmermann wrote:
> 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@suse.de>
> >> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> 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 | 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?
Nope. Instead of the stuff above you have do this:
if (!state->crtc)
return 0;
crtc_state = drm_atomic_get_existing_crtc_state(s->state, s->crtc);
mode = &crtc_state->adjusted_mode;
which is what atmel has, except the redundant fb check dropped. Same for
the cursor version below too.
-Daniel
>
> 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] drm/ast: Fix potential NULL-pointer read during atomic mode setting
2019-11-27 12:38 ` Daniel Vetter
@ 2019-11-27 12:43 ` Thomas Zimmermann
0 siblings, 0 replies; 5+ messages in thread
From: Thomas Zimmermann @ 2019-11-27 12:43 UTC (permalink / raw)
To: Daniel Vetter; +Cc: daniel.vetter, dri-devel, kraxel, airlied, sam
[-- Attachment #1.1.1: Type: text/plain, Size: 9224 bytes --]
Am 27.11.19 um 13:38 schrieb Daniel Vetter:
> On Wed, Nov 27, 2019 at 01:31:25PM +0100, Thomas Zimmermann wrote:
>> 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@suse.de>
>>>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>> 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 | 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?
>
> Nope. Instead of the stuff above you have do this:
>
> if (!state->crtc)
> return 0;
>
> crtc_state = drm_atomic_get_existing_crtc_state(s->state, s->crtc);
> mode = &crtc_state->adjusted_mode;
>
> which is what atmel has, except the redundant fb check dropped. Same for
> the cursor version below too.
Thanks for clarifying.
> -Daniel
>
>>
>> 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
>>
>
>
>
>
--
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
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-11-27 12:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-27 7:31 [PATCH v2] drm/ast: Fix potential NULL-pointer read during atomic mode setting Thomas Zimmermann
2019-11-27 10:49 ` Daniel Vetter
2019-11-27 12:31 ` Thomas Zimmermann
2019-11-27 12:38 ` Daniel Vetter
2019-11-27 12:43 ` Thomas Zimmermann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.