Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC

From: Oleksandr Andrushchenko
Date: Tue Feb 20 2018 - 08:29:17 EST

Next message: Corey Minyard: "Re: [PATCH ipmi/kcs_bmc v2] ipmi: kcs_bmc: make the code be more clean"
Previous message: Yafang Shao: "[PATCH net-next] tcp: remove the hardcode in the definition of TCPF Macro"
In reply to: Oleksandr Andrushchenko: "Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxx>

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 | 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@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);

lgtm. With or without the bikeshed to pull the crtc_state check into the
WARN_ON.

Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>

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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Next message: Corey Minyard: "Re: [PATCH ipmi/kcs_bmc v2] ipmi: kcs_bmc: make the code be more clean"
Previous message: Yafang Shao: "[PATCH net-next] tcp: remove the hardcode in the definition of TCPF Macro"
In reply to: Oleksandr Andrushchenko: "Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]