From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752038AbeBTN3M (ORCPT ); Tue, 20 Feb 2018 08:29:12 -0500 Received: from mail-lf0-f66.google.com ([209.85.215.66]:34324 "EHLO mail-lf0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751737AbeBTN3K (ORCPT ); Tue, 20 Feb 2018 08:29:10 -0500 X-Google-Smtp-Source: AH8x2274D+6PE8OAyj53m2w+ROvnfBEcv7oW0dGYphp3stXcIQX3SHZHCzG2to6XuFNM6fU3GMzj/w== Subject: Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC From: Oleksandr Andrushchenko To: Oleksandr Andrushchenko , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, airlied@linux.ie, daniel.vetter@intel.com References: <1518511456-28257-1-git-send-email-andr2000@gmail.com> <20180219143002.GC22199@phenom.ffwll.local> <20180220111748.GJ22199@phenom.ffwll.local> <38f46c4f-3c0d-cf86-3d50-cf0f9313b205@gmail.com> <20180220124919.GS22199@phenom.ffwll.local> <94327c23-af1c-a348-5dd2-dfb963b71c96@gmail.com> Message-ID: <12b4be99-9003-d566-bda4-2982d5562307@gmail.com> Date: Tue, 20 Feb 2018 15:29:07 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <94327c23-af1c-a348-5dd2-dfb963b71c96@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 02/20/2018 02:53 PM, Oleksandr Andrushchenko wrote: > On 02/20/2018 02:49 PM, Daniel Vetter wrote: >> On Tue, Feb 20, 2018 at 02:36:05PM +0200, Oleksandr Andrushchenko wrote: >>> 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 >> Right, when the plane is completely off there's no CRTC state. Correct >> check should be >> >>     WARN_ON(crtc_state && crtc_state->enabled); > ok, will update with this additional check huh, this indeed solves the NULL pointer dereference, but floods a lot with every page flip I have, e.g. !plane_state->visible == true and crtc_state is not NULL and crtc_state->enable == true, thus firing WARN_ON. Is this something wrong with my use-case/driver or it is still legal to have such a configuration and leave it without WARN_ON and just return 0? >> >>>>> + >>>>> +       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 >>> 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 >>> >>> --- >>>   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); >> lgtm. With or without the bikeshed to pull the crtc_state check into the >> WARN_ON. >> >> Reviewed-by: Daniel Vetter > Thank you >> >> Please resubmit as a stand-alone patch, patchwork can't pull patches out >> of attachments :-/ > oh, that was for demonstration purpose only, so we > are on the same page and see the patch we are discussing ;) >> -Daniel >> >>>         if (!pipe->funcs || !pipe->funcs->check) >>>           return 0; >>> -- >>> 2.7.4 >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> >