+
+ if (plane_state->visible && crtc_state->enable)
Similar here.
+ drm_mode_get_hv_timing(&crtc_state->mode,
+ &clip.x2, &clip.y2);
+
if (!plane_state->visible)
return -EINVAL;
This can now be removed, the plane helper takes care of checking for
plane_state->visible != crtc_state->enable. Please also remove.
We'd need to add a guarantee to
drm_atomic_helper_check_plane_state that
it can cope with crtc_state == NULL, but I think that's a good idea
anyway. Atm it shouldn't end up looking at the
crtc_state pointer in that
case.
It doesn't look at it at the moment
Otherwise we'll just go with your fix, but it feels
all a bit too fragile,
hence why I want to explore more robust options a bit.
At list with the change above it passes my test which failed
before. Although I cannot confirm it works for others, but it
certainly does for me.
-Daniel
Do you want me to send v1 with the code above?
Yes please, with my above cleanup suggestions.
Please see the patch under test attached (I believe it is what
you mean,
with the only change that
if (!plane_state->visible) {
*if (crtc_state)*
WARN_ON(crtc_state->enable);
return 0;
}
check is used).
Whith this patch + additional logs I have:
[ 18.939204] [drm:drm_ioctl [drm]] pid=2105, dev=0xe200, auth=1,
DRM_IOCTL_MODE_ATOMIC
[...]
[ 18.939681] [drm:drm_atomic_set_crtc_for_plane [drm]] Link
plane state
00000000c302cbbf to [NOCRTC]
[ 18.939822] [drm:drm_atomic_set_fb_for_plane [drm]] Set
[NOFB] for plane
state 00000000c302cbbf
[ 18.939963] [drm:drm_atomic_print_state [drm]] checking
000000000bc224e7
[ 18.939988] vdispl vdispl.0: [drm] plane[29]: plane-0
[ 18.940003] vdispl vdispl.0: [drm] crtc=(null)
[ 18.940018] vdispl vdispl.0: [drm] fb=0
[ 18.940032] vdispl vdispl.0: [drm] crtc-pos=0x0+0+0
[ 18.940048] vdispl vdispl.0: [drm]
src-pos=0.000000x0.000000+0.000000+0.000000
[ 18.940067] vdispl vdispl.0: [drm] rotation=1
[ 18.940199] [drm:drm_atomic_check_only [drm]] checking
000000000bc224e7
[ 18.940226] ================================= plane_state->visible 0
crtc_state (null)
[...]
[ 18.978146] [drm:drm_atomic_set_crtc_for_plane [drm]] Link
plane state
000000006bd50580 to [CRTC:30:crtc-0]
[ 18.978292] [drm:drm_atomic_set_fb_for_plane [drm]] Set
[FB:35] for plane
state 000000006bd50580
[ 18.978993] [drm:drm_atomic_set_mode_prop_for_crtc [drm]] Set
[MODE:1024x768] for CRTC state 00000000e5a28f6a
[ 18.979425] [drm:drm_atomic_check_only [drm]] checking
000000000bc224e7
[ 18.979540] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]]
[CRTC:30:crtc-0] mode changed
[ 18.979632] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]]
[CRTC:30:crtc-0] enable changed
[ 18.979708] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]]
[CRTC:30:crtc-0] active changed
[ 18.979792] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]]
Updating routing for [CONNECTOR:28:Virtual-1]
[ 18.979877] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]]
[CONNECTOR:28:Virtual-1] using [ENCODER:31:None-31] on [CRTC:30:crtc-0]
[ 18.979960] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]]
[CRTC:30:crtc-0] needs all connectors, enable: y, active: y
[ 18.980139] [drm:drm_atomic_add_affected_connectors [drm]]
Adding all
current connectors for [CRTC:30:crtc-0] to 000000000bc224e7
[ 18.980184] ================================= plane_state->visible 0
crtc_state 00000000e5a28f6a
[ 18.980205] crtc_state->enable 1
*[ 19.022608] WARNING: CPU: 1 PID: 2105 at
drivers/gpu/drm/drm_simple_kms_helper.c:137
drm_simple_kms_plane_atomic_check+0xdc/0xf8 [drm_kms_helper]*
[...]
[ 19.113601] ================================= plane_state->visible 0
crtc_state 00000000e5a28f6a
[ 19.113623] crtc_state->enable 1
[ 19.113792] WARNING: CPU: 1 PID: 2105 at
drivers/gpu/drm/drm_simple_kms_helper.c:137
drm_simple_kms_plane_atomic_check+0xdc/0xf8 [drm_kms_helper]
[...]
And finally
[ 19.340249] ================================= plane_state->visible 0
crtc_state 0000000036a1b1f5
[ 19.340271] crtc_state->enable 0
So, it seems that crtc_state can still be NULL if
"!plane_state->visible"
making
NULL pointer dereference, so we need a check for that.
Yet, !plane_state->visible && crtc_state->enable makes WARN_ON to fire
always. So, probably we may want removing it.
Thanks, Daniel
Thank you,
Oleksandr
From dbcce708b237740158a2c16029c56a579324f269 Mon Sep 17 00:00:00 2001
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
Date: Tue, 13 Feb 2018 10:32:20 +0200
Subject: [PATCH] drm/simple_kms_helper: Fix NULL pointer
dereference with no
active CRTC
It is possible that drm_simple_kms_plane_atomic_check called
with no CRTC set, e.g. when user-space application sets CRTC_ID/FB_ID
to 0 before doing any actual drawing. This leads to NULL pointer
dereference because in this case new CRTC state is NULL and must be
checked before accessing.
Signed-off-by: Oleksandr Andrushchenko
<oleksandr_andrushchenko@xxxxxxxx>
---
drivers/gpu/drm/drm_simple_kms_helper.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
b/drivers/gpu/drm/drm_simple_kms_helper.c
index 9ca8a4a59b74..f54711ff9767 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -121,12 +121,6 @@ static int
drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
pipe = container_of(plane, struct drm_simple_display_pipe,
plane);
crtc_state = drm_atomic_get_new_crtc_state(plane_state->state,
&pipe->crtc);
- if (!crtc_state->enable)
- return 0; /* nothing to check when disabling or disabled */
-
- if (crtc_state->enable)
- drm_mode_get_hv_timing(&crtc_state->mode,
- &clip.x2, &clip.y2);
ret = drm_atomic_helper_check_plane_state(plane_state,
crtc_state,
&clip,
@@ -136,8 +130,13 @@ static int
drm_simple_kms_plane_atomic_check(struct drm_plane *plane,
if (ret)
return ret;
- if (!plane_state->visible)
- return -EINVAL;
+ if (!plane_state->visible) {
+ if (crtc_state)
+ WARN_ON(crtc_state->enable);
+ return 0;
+ }
+
+ drm_mode_get_hv_timing(&crtc_state->mode, &clip.x2, &clip.y2);
