* [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC @ 2018-02-13 8:44 ` Oleksandr Andrushchenko 0 siblings, 0 replies; 18+ messages in thread From: Oleksandr Andrushchenko @ 2018-02-13 8:44 UTC (permalink / raw) To: dri-devel, linux-kernel Cc: daniel.vetter, gustavo, airlied, seanpaul, Oleksandr Andrushchenko 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, -- 2.7.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC @ 2018-02-13 8:44 ` Oleksandr Andrushchenko 0 siblings, 0 replies; 18+ messages in thread From: Oleksandr Andrushchenko @ 2018-02-13 8:44 UTC (permalink / raw) To: dri-devel, linux-kernel; +Cc: airlied, daniel.vetter, Oleksandr Andrushchenko 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, -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC 2018-02-13 8:44 ` Oleksandr Andrushchenko @ 2018-02-19 14:30 ` Daniel Vetter -1 siblings, 0 replies; 18+ messages in thread From: Daniel Vetter @ 2018-02-19 14:30 UTC (permalink / raw) To: Oleksandr Andrushchenko Cc: dri-devel, linux-kernel, airlied, daniel.vetter, Oleksandr Andrushchenko 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? 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. 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. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC @ 2018-02-19 14:30 ` Daniel Vetter 0 siblings, 0 replies; 18+ messages in thread From: Daniel Vetter @ 2018-02-19 14:30 UTC (permalink / raw) To: Oleksandr Andrushchenko Cc: airlied, daniel.vetter, linux-kernel, dri-devel, Oleksandr Andrushchenko 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? 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. 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. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC 2018-02-19 14:30 ` Daniel Vetter @ 2018-02-19 14:58 ` Oleksandr Andrushchenko -1 siblings, 0 replies; 18+ messages in thread From: Oleksandr Andrushchenko @ 2018-02-19 14:58 UTC (permalink / raw) To: Oleksandr Andrushchenko, dri-devel, linux-kernel, airlied, daniel.vetter 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: 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 && crtc_state->enable) + drm_mode_get_hv_timing(&crtc_state->mode, + &clip.x2, &clip.y2); + if (!plane_state->visible) return -EINVAL; > 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? Thank you, Oleksandr ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC @ 2018-02-19 14:58 ` Oleksandr Andrushchenko 0 siblings, 0 replies; 18+ messages in thread From: Oleksandr Andrushchenko @ 2018-02-19 14:58 UTC (permalink / raw) To: Oleksandr Andrushchenko, dri-devel, linux-kernel, airlied, daniel.vetter 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: 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 && crtc_state->enable) + drm_mode_get_hv_timing(&crtc_state->mode, + &clip.x2, &clip.y2); + if (!plane_state->visible) return -EINVAL; > 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? Thank you, Oleksandr _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC 2018-02-19 14:58 ` Oleksandr Andrushchenko @ 2018-02-20 11:17 ` Daniel Vetter -1 siblings, 0 replies; 18+ messages in thread From: Daniel Vetter @ 2018-02-20 11:17 UTC (permalink / raw) To: Oleksandr Andrushchenko Cc: Oleksandr Andrushchenko, dri-devel, linux-kernel, airlied, daniel.vetter 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. > + > + 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. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC @ 2018-02-20 11:17 ` Daniel Vetter 0 siblings, 0 replies; 18+ messages in thread From: Daniel Vetter @ 2018-02-20 11:17 UTC (permalink / raw) To: Oleksandr Andrushchenko Cc: Oleksandr Andrushchenko, airlied, linux-kernel, dri-devel, daniel.vetter 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. > + > + 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. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC 2018-02-20 11:17 ` Daniel Vetter @ 2018-02-20 12:36 ` Oleksandr Andrushchenko -1 siblings, 0 replies; 18+ messages in thread From: Oleksandr Andrushchenko @ 2018-02-20 12:36 UTC (permalink / raw) To: Oleksandr Andrushchenko, dri-devel, linux-kernel, airlied, daniel.vetter [-- Attachment #1: Type: text/plain, Size: 8470 bytes --] 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 [-- Attachment #2: 0001-drm-simple_kms_helper-Fix-NULL-pointer-dereference-w.patch --] [-- Type: text/x-patch, Size: 1909 bytes --] >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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC @ 2018-02-20 12:36 ` Oleksandr Andrushchenko 0 siblings, 0 replies; 18+ messages in thread From: Oleksandr Andrushchenko @ 2018-02-20 12:36 UTC (permalink / raw) To: Oleksandr Andrushchenko, dri-devel, linux-kernel, airlied, daniel.vetter [-- Attachment #1: Type: text/plain, Size: 8470 bytes --] 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 [-- Attachment #2: 0001-drm-simple_kms_helper-Fix-NULL-pointer-dereference-w.patch --] [-- Type: text/x-patch, Size: 1909 bytes --] >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 [-- Attachment #3: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC 2018-02-20 12:36 ` Oleksandr Andrushchenko @ 2018-02-20 12:49 ` Daniel Vetter -1 siblings, 0 replies; 18+ messages in thread From: Daniel Vetter @ 2018-02-20 12:49 UTC (permalink / raw) To: Oleksandr Andrushchenko Cc: Oleksandr Andrushchenko, dri-devel, linux-kernel, airlied, daniel.vetter 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); > > > + > > > + 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> Please resubmit as a stand-alone patch, patchwork can't pull patches out of attachments :-/ -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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC @ 2018-02-20 12:49 ` Daniel Vetter 0 siblings, 0 replies; 18+ messages in thread From: Daniel Vetter @ 2018-02-20 12:49 UTC (permalink / raw) To: Oleksandr Andrushchenko Cc: airlied, daniel.vetter, linux-kernel, dri-devel, Oleksandr Andrushchenko 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); > > > + > > > + 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> Please resubmit as a stand-alone patch, patchwork can't pull patches out of attachments :-/ -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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC 2018-02-20 12:49 ` Daniel Vetter @ 2018-02-20 12:53 ` Oleksandr Andrushchenko -1 siblings, 0 replies; 18+ messages in thread From: Oleksandr Andrushchenko @ 2018-02-20 12:53 UTC (permalink / raw) To: Oleksandr Andrushchenko, dri-devel, linux-kernel, airlied, daniel.vetter 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 > >>>> + >>>> + 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC @ 2018-02-20 12:53 ` Oleksandr Andrushchenko 0 siblings, 0 replies; 18+ messages in thread From: Oleksandr Andrushchenko @ 2018-02-20 12:53 UTC (permalink / raw) To: Oleksandr Andrushchenko, dri-devel, linux-kernel, airlied, daniel.vetter 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 > >>>> + >>>> + 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 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC 2018-02-20 12:53 ` Oleksandr Andrushchenko @ 2018-02-20 13:29 ` Oleksandr Andrushchenko -1 siblings, 0 replies; 18+ messages in thread From: Oleksandr Andrushchenko @ 2018-02-20 13:29 UTC (permalink / raw) To: Oleksandr Andrushchenko, dri-devel, linux-kernel, airlied, daniel.vetter 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 >> > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC @ 2018-02-20 13:29 ` Oleksandr Andrushchenko 0 siblings, 0 replies; 18+ messages in thread From: Oleksandr Andrushchenko @ 2018-02-20 13:29 UTC (permalink / raw) To: Oleksandr Andrushchenko, dri-devel, linux-kernel, airlied, daniel.vetter 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 >> > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC 2018-02-20 13:29 ` Oleksandr Andrushchenko (?) @ 2018-03-05 8:52 ` Daniel Vetter 2018-03-05 8:55 ` Oleksandr Andrushchenko -1 siblings, 1 reply; 18+ messages in thread From: Daniel Vetter @ 2018-03-05 8:52 UTC (permalink / raw) To: Oleksandr Andrushchenko Cc: Oleksandr Andrushchenko, dri-devel, linux-kernel, airlied, daniel.vetter On Tue, Feb 20, 2018 at 03:29:07PM +0200, Oleksandr Andrushchenko wrote: > 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? 1 week of vacation later I have to admit that this WARN_ON is completely bogus :-) Sorry for all the confusion, pls leave it out. -Daniel > > > > > > > > > + > > > > > > + 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 > > > > > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC 2018-03-05 8:52 ` Daniel Vetter @ 2018-03-05 8:55 ` Oleksandr Andrushchenko 0 siblings, 0 replies; 18+ messages in thread From: Oleksandr Andrushchenko @ 2018-03-05 8:55 UTC (permalink / raw) To: Oleksandr Andrushchenko, dri-devel, linux-kernel, airlied, daniel.vetter On 03/05/2018 10:52 AM, Daniel Vetter wrote: > On Tue, Feb 20, 2018 at 03:29:07PM +0200, Oleksandr Andrushchenko wrote: >> 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? > 1 week of vacation later I have to admit that this WARN_ON is completely > bogus :-) np ;) > Sorry for all the confusion, pls leave it out. > -Daniel > >>>>>>> + >>>>>>> + 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 >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-03-05 8:56 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-13 8:44 [PATCH] drm/simple_kms_helper: Fix NULL pointer dereference with no active CRTC Oleksandr Andrushchenko 2018-02-13 8:44 ` Oleksandr Andrushchenko 2018-02-19 14:30 ` Daniel Vetter 2018-02-19 14:30 ` Daniel Vetter 2018-02-19 14:58 ` Oleksandr Andrushchenko 2018-02-19 14:58 ` Oleksandr Andrushchenko 2018-02-20 11:17 ` Daniel Vetter 2018-02-20 11:17 ` Daniel Vetter 2018-02-20 12:36 ` Oleksandr Andrushchenko 2018-02-20 12:36 ` Oleksandr Andrushchenko 2018-02-20 12:49 ` Daniel Vetter 2018-02-20 12:49 ` Daniel Vetter 2018-02-20 12:53 ` Oleksandr Andrushchenko 2018-02-20 12:53 ` Oleksandr Andrushchenko 2018-02-20 13:29 ` Oleksandr Andrushchenko 2018-02-20 13:29 ` Oleksandr Andrushchenko 2018-03-05 8:52 ` Daniel Vetter 2018-03-05 8:55 ` Oleksandr Andrushchenko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.