Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with
 no active CRTC
From:   Oleksandr Andrushchenko <andr2000@gmail.com>
To:     Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>,
        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>
 <d676eef9-55b2-a7fd-be9b-4d53d59610a9@epam.com>
 <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
Precedence: bulk

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 <oleksandr_andrushchenko@epam.com>
>>>>>>>
>>>>>>> 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@epam.com>
>>>>>>> ---
>>>>>>>     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 <oleksandr_andrushchenko@epam.com>
>>> 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@epam.com>
>>> ---
>>>   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 <daniel.vetter@ffwll.ch>
> 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
>>
>