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

[PATCH 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 since we're passing previous plane state,
and plane_state->state has been assigned to NULL in
drm_atomic_helper_swap_state().

Assign plane->state->state to new_plane_state->state before calling
vc4_plane_atomic_check() and reset it to NULL after
vc4_plane_atomic_check() as returned.

Fixes: 58a6a36fe8e0 ("drm/vc4: Use drm_atomic_helper_check_plane_state() to simplify the logic")
Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/gpu/drm/vc4/vc4_plane.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 9dc3fcbd290b..c33c7dc2119d 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -827,8 +827,13 @@ static void vc4_plane_atomic_async_update(struct drm_plane *plane,
 	plane->state->src_x = state->src_x;
 	plane->state->src_y = state->src_y;
 
-	/* Update the display list based on the new crtc_x/y. */
+	/* Update the display list based on the new crtc_x/y.
+	 * The plane->state->state update dance is needed to avoid NULL pointer
+	 * dereference in vc4_plane_setup_clipping_and_scaling().
+	 */
+	plane->state->state = state->state;
 	vc4_plane_atomic_check(plane, plane->state);
+	plane->state->state = NULL;
 
 	/* 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 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 now 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>
---
 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 2/2] drm/vc4: Set ->legacy_cursor_update to false when doing non-async updates

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 460 bytes --]

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

> drm_atomic_helper_setup_commit() auto-completes commit->flip_done when
> state->legacy_cursor_update is true, but we now 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.

Seems like a reasonable solution.

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

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

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

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 460 bytes --]

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

> drm_atomic_helper_setup_commit() auto-completes commit->flip_done when
> state->legacy_cursor_update is true, but we now 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.

Seems like a reasonable solution.

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

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

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

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1016 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 since we're passing previous plane state,
> and plane_state->state has been assigned to NULL in
> drm_atomic_helper_swap_state().
>
> Assign plane->state->state to new_plane_state->state before calling
> vc4_plane_atomic_check() and reset it to NULL after
> vc4_plane_atomic_check() as returned.
>
> Fixes: 58a6a36fe8e0 ("drm/vc4: Use drm_atomic_helper_check_plane_state() to simplify the logic")
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

Hmm.  Could we pass in the new state instead, and then pick the dlist
items out of the new state's dlist to write into both our dlist copy and
the hw dlist?

[-- 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 1/2] drm/vc4: Fix NULL pointer dereference in the async update path

[Boris Brezillon]

On Tue, 13 Nov 2018 13:24:07 -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 since we're passing previous plane state,
> > and plane_state->state has been assigned to NULL in
> > drm_atomic_helper_swap_state().
> >
> > Assign plane->state->state to new_plane_state->state before calling
> > vc4_plane_atomic_check() and reset it to NULL after
> > vc4_plane_atomic_check() as returned.
> >
> > Fixes: 58a6a36fe8e0 ("drm/vc4: Use drm_atomic_helper_check_plane_state() to simplify the logic")
> > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>  
> 
> Hmm.  Could we pass in the new state instead, and then pick the dlist
> items out of the new state's dlist to write into both our dlist copy and
> the hw dlist?

I considered doing that, but other fields in vc4_plane_state might be
updated too, and since we have to update things in place it was just
simpler to pass plane->state directly instead copying the updated
fields one by one (with the risk of missing one of them).

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