[PATCH v3 1/2] drm/vc4: Fix NULL pointer dereference in the async update path

[PATCH v3 1/2] drm/vc4: Fix NULL pointer dereference in the async update path

[Boris Brezillon]

vc4_plane_atomic_async_update() calls vc4_plane_atomic_check()
which in turn calls vc4_plane_setup_clipping_and_scaling(), and since
commit 58a6a36fe8e0 ("drm/vc4: Use
drm_atomic_helper_check_plane_state() to simplify the logic"), this
function accesses plane_state->state which will be NULL when called
from the async update path because we're passing the current plane
state, and plane_state->state has been assigned to NULL in
drm_atomic_helper_swap_state().

Pass the new state instead of the current one (the new state has
->state set to a non-NULL value).

Fixes: 58a6a36fe8e0 ("drm/vc4: Use drm_atomic_helper_check_plane_state() to simplify the logic")
Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v3:
- Add a Fixes tag

Changes in v2:
- Pass the new state instead of plane->state (suggested by Eric)
---
 drivers/gpu/drm/vc4/vc4_plane.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 1728fb7d00ba..c3ded0ba0441 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -854,7 +854,7 @@ void vc4_plane_async_set_fb(struct drm_plane *plane, struct drm_framebuffer *fb)
 static void vc4_plane_atomic_async_update(struct drm_plane *plane,
 					  struct drm_plane_state *state)
 {
-	struct vc4_plane_state *vc4_state = to_vc4_plane_state(plane->state);
+	struct vc4_plane_state *vc4_state, *new_vc4_state;
 
 	if (plane->state->fb != state->fb) {
 		vc4_plane_async_set_fb(plane, state->fb);
@@ -875,7 +875,18 @@ static void vc4_plane_atomic_async_update(struct drm_plane *plane,
 	plane->state->src_y = state->src_y;
 
 	/* Update the display list based on the new crtc_x/y. */
-	vc4_plane_atomic_check(plane, plane->state);
+	vc4_plane_atomic_check(plane, state);
+
+	new_vc4_state = to_vc4_plane_state(state);
+	vc4_state = to_vc4_plane_state(plane->state);
+
+	/* Update the current vc4_state pos0, pos2 and ptr0 dlist entries. */
+	vc4_state->dlist[vc4_state->pos0_offset] =
+		new_vc4_state->dlist[vc4_state->pos0_offset];
+	vc4_state->dlist[vc4_state->pos2_offset] =
+		new_vc4_state->dlist[vc4_state->pos2_offset];
+	vc4_state->dlist[vc4_state->ptr0_offset] =
+		new_vc4_state->dlist[vc4_state->ptr0_offset];
 
 	/* Note that we can't just call vc4_plane_write_dlist()
 	 * because that would smash the context data that the HVS is
-- 
2.17.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 2/2] drm/vc4: Set ->legacy_cursor_update to false when doing non-async updates

[Boris Brezillon]

drm_atomic_helper_setup_commit() auto-completes commit->flip_done when
state->legacy_cursor_update is true, but we know for sure that we want
a sync update when we call drm_atomic_helper_setup_commit() from
vc4_atomic_commit().

Explicitly set state->legacy_cursor_update to false to prevent this
auto-completion.

Fixes: 184d3cf4f738 ("drm/vc4: Use wait_for_flip_done() instead of wait_for_vblanks()")
Cc: <stable@vger.kernel.org>
Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
---
Changes in v3:
- Add missing Fixes and Cc-stable tags
- Fix a typo in the commit message

Changes in v2:
- Add Eric's R-b
---
 drivers/gpu/drm/vc4/vc4_kms.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 127468785f74..1f94b9affe4b 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -214,6 +214,12 @@ static int vc4_atomic_commit(struct drm_device *dev,
 		return 0;
 	}
 
+	/* We know for sure we don't want an async update here. Set
+	 * state->legacy_cursor_update to false to prevent
+	 * drm_atomic_helper_setup_commit() from auto-completing
+	 * commit->flip_done.
+	 */
+	state->legacy_cursor_update = false;
 	ret = drm_atomic_helper_setup_commit(state, nonblock);
 	if (ret)
 		return ret;
-- 
2.17.1

Re: [PATCH v3 1/2] drm/vc4: Fix NULL pointer dereference in the async update path

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 869 bytes --]

Boris Brezillon <boris.brezillon@bootlin.com> writes:

> vc4_plane_atomic_async_update() calls vc4_plane_atomic_check()
> which in turn calls vc4_plane_setup_clipping_and_scaling(), and since
> commit 58a6a36fe8e0 ("drm/vc4: Use
> drm_atomic_helper_check_plane_state() to simplify the logic"), this
> function accesses plane_state->state which will be NULL when called
> from the async update path because we're passing the current plane
> state, and plane_state->state has been assigned to NULL in
> drm_atomic_helper_swap_state().
>
> Pass the new state instead of the current one (the new state has
> ->state set to a non-NULL value).
>
> Fixes: 58a6a36fe8e0 ("drm/vc4: Use drm_atomic_helper_check_plane_state() to simplify the logic")
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

I'm glad this worked!

Reviewed-by: Eric Anholt <eric@anholt.net>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/2] drm/vc4: Fix NULL pointer dereference in the async update path

[Boris Brezillon]

On Thu, 15 Nov 2018 08:25:48 -0800
Eric Anholt <eric@anholt.net> wrote:

> Boris Brezillon <boris.brezillon@bootlin.com> writes:
> 
> > vc4_plane_atomic_async_update() calls vc4_plane_atomic_check()
> > which in turn calls vc4_plane_setup_clipping_and_scaling(), and since
> > commit 58a6a36fe8e0 ("drm/vc4: Use
> > drm_atomic_helper_check_plane_state() to simplify the logic"), this
> > function accesses plane_state->state which will be NULL when called
> > from the async update path because we're passing the current plane
> > state, and plane_state->state has been assigned to NULL in
> > drm_atomic_helper_swap_state().
> >
> > Pass the new state instead of the current one (the new state has  
> > ->state set to a non-NULL value).  
> >
> > Fixes: 58a6a36fe8e0 ("drm/vc4: Use drm_atomic_helper_check_plane_state() to simplify the logic")
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>  
> 
> I'm glad this worked!
> 
> Reviewed-by: Eric Anholt <eric@anholt.net>

Series applied to drm-misc-fixes.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel