Hi Am 29.07.22 um 18:40 schrieb Chrisanthus, Anitha:
Agree with Thomas, drm_atomic_commit() will not call kmb_atomic_update() with a NULL plane. This is not an actual bug.
Indeed, it's the atomic_update function. I didn't notice at first.
Thanks, Anitha-----Original Message----- From: Thomas Zimmermann <tzimmerm...@suse.de> Sent: Friday, July 29, 2022 7:26 AM To: Zeng Jingxiang <zengj...@gmail.com>; Chrisanthus, Anitha <anitha.chrisant...@intel.com>; edmund.j....@intel.com; airl...@linux.ie; dan...@ffwll.ch; laurent.pinch...@ideasonboard.com; max...@cerno.tech; ville.syrj...@linux.intel.com Cc: Zeng Jingxiang <linusz...@tencent.com>; linux-ker...@vger.kernel.org; dri-devel@lists.freedesktop.org Subject: Re: [PATCH] drm/kmb: fix dereference before NULL check in kmb_plane_atomic_update() Hi Am 29.07.22 um 16:23 schrieb Thomas Zimmermann:Hi Am 29.07.22 um 05:07 schrieb Zeng Jingxiang:From: Zeng Jingxiang <linusz...@tencent.com> The "plane" pointer was access before checking if it was NULL. The drm_atomic_get_old_plane_state() function will dereference the pointer "plane". 345 struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane); 346 struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane); A NULL check for "plane" indicates that it may be NULL 363 if (!plane || !new_plane_state || !old_plane_state)Is this an actual bug that happens? All planes should always have a state. Therefore the tests for !new_plane_state and !old_plane_state can be removed, I'd say.Just to clarify: moving the test for !plane before using one of the get_plane_state functions is a correct bugfix. Best regards ThomasBest regards ThomasFixes: 977697e20b3d ("drm/atomic: Pass the full state to planes atomic disable and update") Fixes: 37418bf14c13 ("drm: Use state helper instead of the plane state pointer") Signed-off-by: Zeng Jingxiang <linusz...@tencent.com> --- drivers/gpu/drm/kmb/kmb_plane.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/kmb/kmb_plane.c b/drivers/gpu/drm/kmb/kmb_plane.c index 2735b8eb3537..d2bc998b65ce 100644 --- a/drivers/gpu/drm/kmb/kmb_plane.c +++ b/drivers/gpu/drm/kmb/kmb_plane.c @@ -342,10 +342,7 @@ static void kmb_plane_set_alpha(struct kmb_drm_private *kmb, static void kmb_plane_atomic_update(struct drm_plane *plane, struct drm_atomic_state *state) { - struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, - plane); - struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, - plane); + struct drm_plane_state *old_plane_state, *new_plane_state; struct drm_framebuffer *fb; struct kmb_drm_private *kmb; unsigned int width; @@ -360,7 +357,13 @@ static void kmb_plane_atomic_update(struct drm_plane *plane, static dma_addr_t addr[MAX_SUB_PLANES]; struct disp_cfg *init_disp_cfg; - if (!plane || !new_plane_state || !old_plane_state) + if (!plane) + return; + + old_plane_state = drm_atomic_get_old_plane_state(state, plane); + new_plane_state = drm_atomic_get_new_plane_state(state, plane); + + if (!new_plane_state || !old_plane_state)
To add to my previous reply, none of these variables can legally be NULL here. If plane if NULL, DRM helpers would run atomic_disable instead.
If you want to fix anything, just remove the tests entirely and save a few CPU cycles.
Best regards Thomas
return; fb = new_plane_state->fb;-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
