On 02/20/2018 01:17 PM, Daniel Vetter wrote: > On Mon, Feb 19, 2018 at 04:58:43PM +0200, Oleksandr Andrushchenko wrote: >> On 02/19/2018 04:30 PM, Daniel Vetter wrote: >>> On Tue, Feb 13, 2018 at 10:44:16AM +0200, Oleksandr Andrushchenko wrote: >>>> From: Oleksandr Andrushchenko >>>> >>>> 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 >>>> --- >>>> drivers/gpu/drm/drm_simple_kms_helper.c | 6 ++++-- >>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c >>>> index 9ca8a4a59b74..a05eca9cec8b 100644 >>>> --- a/drivers/gpu/drm/drm_simple_kms_helper.c >>>> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c >>>> @@ -121,8 +121,10 @@ 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 || !crtc_state->enable) >>>> + /* nothing to check when disabling or disabled or no CRTC set */ >>>> + return 0; >>>> if (crtc_state->enable) >>>> drm_mode_get_hv_timing(&crtc_state->mode, >>> Hm, this is a bit annoying, since the can_position = false parameter to >>> drm_atomic_helper_check_plane_state is supposed to catch all this. Would >>> moving all the checks after the call to that helper, and gating them on >>> plane_state->visible also work? >> Yes, it does work if this is what you mean: > I wasn't sure, thanks for figuring this out! > >> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c >> b/drivers/gpu/drm/drm_simple_kms_helper.c >> index a05eca9cec8b..c48858bb2823 100644 >> --- a/drivers/gpu/drm/drm_simple_kms_helper.c >> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c >> @@ -122,14 +122,6 @@ static int drm_simple_kms_plane_atomic_check(struct >> drm_plane *plane, >>         crtc_state = drm_atomic_get_new_crtc_state(plane_state->state, >> &pipe->crtc); >> >> -       if (!crtc_state || !crtc_state->enable) >> -               /* nothing to check when disabling or disabled or no CRTC >> set */ >> -               return 0; >> - >> -       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, >> DRM_PLANE_HELPER_NO_SCALING, >> @@ -138,6 +130,13 @@ static int drm_simple_kms_plane_atomic_check(struct >> drm_plane *plane, >>         if (ret) >>                 return ret; >> >> +       if (!plane_state->visible || !crtc_state->enable) >> +               return 0; /* nothing to check when disabling or disabled */ > if (!plane_state->visible) { > WARN_ON(crtc_state->enabled); > return 0; > } > > The helper call above should guarantee this. Yes, but I still see cases when crtc_state is NULL, thus making crtc_state->enable to fail >> + >> +       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