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
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>
From:   Oleksandr Andrushchenko <andr2000@gmail.com>
Message-ID: <38f46c4f-3c0d-cf86-3d50-cf0f9313b205@gmail.com>
Date:   Tue, 20 Feb 2018 14:36:05 +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: <20180220111748.GJ22199@phenom.ffwll.local>
Content-Type: multipart/mixed;
 boundary="------------1FA7563911FF1FD6F04FF04E"
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

This is a multi-part message in MIME format.
--------------1FA7563911FF1FD6F04FF04E
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit

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
>> +
>> +       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

--------------1FA7563911FF1FD6F04FF04E
Content-Type: text/x-patch;
 name="0001-drm-simple_kms_helper-Fix-NULL-pointer-dereference-w.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="0001-drm-simple_kms_helper-Fix-NULL-pointer-dereference-w.pa";
 filename*1="tch"

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);
 
 	if (!pipe->funcs || !pipe->funcs->check)
 		return 0;
-- 
2.7.4


--------------1FA7563911FF1FD6F04FF04E--