Because vmwgfx has no clear delineation between crtcs, connectors, and encoders we were destroying and freeing the entire display unit when any of these components were destroyed. This breaks the cleanup code in drm_mode_config_cleanup which has a specific teardown order and results in a use after free and/or nullptr deref.
A display unit most closely maps to a CRTC which is destroyed last in drm_mode_config_cleanup so all major cleanup related to the DU should be done in the crtc_destroy hook. Signed-off-by: Ian Forbes <[email protected]> --- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 24 --------- drivers/gpu/drm/vmwgfx/vmwgfx_kms.h | 4 -- drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c | 41 ++++----------- drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c | 40 ++++----------- drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c | 75 ++++------------------------ drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c | 4 -- 6 files changed, 28 insertions(+), 160 deletions(-) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c index 1b407b61f683..53de95d25ee8 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c @@ -26,30 +26,6 @@ void vmw_du_init(struct vmw_display_unit *du) vmw_vkms_crtc_init(&du->crtc); } -void vmw_du_cleanup(struct vmw_display_unit *du) -{ - struct vmw_private *dev_priv = vmw_priv(du->primary.dev); - - vmw_vkms_crtc_cleanup(&du->crtc); - drm_plane_cleanup(&du->primary); - if (vmw_cmd_supported(dev_priv)) - drm_plane_cleanup(&du->cursor.base); - - drm_connector_unregister(&du->connector); - drm_crtc_cleanup(&du->crtc); - drm_encoder_cleanup(&du->encoder); - drm_connector_cleanup(&du->connector); -} - - -void vmw_du_primary_plane_destroy(struct drm_plane *plane) -{ - drm_plane_cleanup(plane); - - /* Planes are static in our case so we don't free it */ -} - - /** * vmw_du_plane_unpin_surf - unpins resource associated with a framebuffer surface * diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h index 2224d7d91d1b..ec3d23b449e4 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h @@ -348,7 +348,6 @@ struct vmw_display_unit { * Shared display unit functions - vmwgfx_kms.c */ void vmw_du_init(struct vmw_display_unit *du); -void vmw_du_cleanup(struct vmw_display_unit *du); int vmw_du_crtc_gamma_set(struct drm_crtc *crtc, u16 *r, u16 *g, u16 *b, uint32_t size, @@ -403,9 +402,6 @@ void vmw_guess_mode_timing(struct drm_display_mode *mode); void vmw_kms_update_implicit_fb(struct vmw_private *dev_priv); void vmw_kms_create_implicit_placement_property(struct vmw_private *dev_priv); -/* Universal Plane Helpers */ -void vmw_du_primary_plane_destroy(struct drm_plane *plane); - /* Atomic Helpers */ int vmw_du_primary_plane_atomic_check(struct drm_plane *plane, struct drm_atomic_commit *state); diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c index af3e32174563..e6e1c7235673 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c @@ -60,21 +60,18 @@ struct vmw_legacy_display_unit { struct list_head active; }; -static void vmw_ldu_destroy(struct vmw_legacy_display_unit *ldu) -{ - list_del_init(&ldu->active); - vmw_du_cleanup(&ldu->base); - kfree(ldu); -} - - /* * Legacy Display Unit CRTC functions */ static void vmw_ldu_crtc_destroy(struct drm_crtc *crtc) { - vmw_ldu_destroy(vmw_crtc_to_ldu(crtc)); + struct vmw_legacy_display_unit *ldu = vmw_crtc_to_ldu(crtc); + + list_del_init(&ldu->active); + vmw_vkms_crtc_cleanup(&ldu->base.crtc); + drm_crtc_cleanup(&ldu->base.crtc); + kfree(ldu); } static int vmw_ldu_commit_list(struct vmw_private *dev_priv) @@ -264,29 +261,19 @@ static const struct drm_crtc_funcs vmw_legacy_crtc_funcs = { * Legacy Display Unit encoder functions */ -static void vmw_ldu_encoder_destroy(struct drm_encoder *encoder) -{ - vmw_ldu_destroy(vmw_encoder_to_ldu(encoder)); -} - static const struct drm_encoder_funcs vmw_legacy_encoder_funcs = { - .destroy = vmw_ldu_encoder_destroy, + .destroy = drm_encoder_cleanup, }; /* * Legacy Display Unit connector functions */ -static void vmw_ldu_connector_destroy(struct drm_connector *connector) -{ - vmw_ldu_destroy(vmw_connector_to_ldu(connector)); -} - static const struct drm_connector_funcs vmw_legacy_connector_funcs = { .dpms = vmw_du_connector_dpms, .detect = vmw_du_connector_detect, .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = vmw_ldu_connector_destroy, + .destroy = drm_connector_cleanup, .reset = vmw_du_connector_reset, .atomic_duplicate_state = vmw_du_connector_duplicate_state, .atomic_destroy_state = vmw_du_connector_destroy_state, @@ -363,7 +350,7 @@ vmw_ldu_primary_plane_atomic_update(struct drm_plane *plane, static const struct drm_plane_funcs vmw_ldu_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, - .destroy = vmw_du_primary_plane_destroy, + .destroy = drm_plane_cleanup, .reset = vmw_du_plane_reset, .atomic_duplicate_state = vmw_du_plane_duplicate_state, .atomic_destroy_state = vmw_du_plane_destroy_state, @@ -492,18 +479,12 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit) encoder->possible_crtcs = (1 << unit); encoder->possible_clones = 0; - ret = drm_connector_register(connector); - if (ret) { - DRM_ERROR("Failed to register connector\n"); - goto err_free_encoder; - } - ret = drm_crtc_init_with_planes(dev, crtc, primary, vmw_cmd_supported(dev_priv) ? &cursor->base : NULL, &vmw_legacy_crtc_funcs, NULL); if (ret) { DRM_ERROR("Failed to initialize CRTC\n"); - goto err_free_unregister; + goto err_free_encoder; } drm_crtc_helper_add(crtc, &vmw_ldu_crtc_helper_funcs); @@ -526,8 +507,6 @@ static int vmw_ldu_init(struct vmw_private *dev_priv, unsigned unit) return 0; -err_free_unregister: - drm_connector_unregister(connector); err_free_encoder: drm_encoder_cleanup(encoder); err_free_connector: diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c index c83061cf7455..2c008892f51d 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c @@ -97,20 +97,16 @@ struct vmw_screen_object_unit { bool defined; }; -static void vmw_sou_destroy(struct vmw_screen_object_unit *sou) -{ - vmw_du_cleanup(&sou->base); - kfree(sou); -} - - /* * Screen Object Display Unit CRTC functions */ - static void vmw_sou_crtc_destroy(struct drm_crtc *crtc) { - vmw_sou_destroy(vmw_crtc_to_sou(crtc)); + struct vmw_screen_object_unit *sou = vmw_crtc_to_sou(crtc); + + vmw_vkms_crtc_cleanup(&sou->base.crtc); + drm_crtc_cleanup(&sou->base.crtc); + kfree(sou); } /* @@ -319,29 +315,19 @@ static const struct drm_crtc_funcs vmw_screen_object_crtc_funcs = { * Screen Object Display Unit encoder functions */ -static void vmw_sou_encoder_destroy(struct drm_encoder *encoder) -{ - vmw_sou_destroy(vmw_encoder_to_sou(encoder)); -} - static const struct drm_encoder_funcs vmw_screen_object_encoder_funcs = { - .destroy = vmw_sou_encoder_destroy, + .destroy = drm_encoder_cleanup, }; /* * Screen Object Display Unit connector functions */ -static void vmw_sou_connector_destroy(struct drm_connector *connector) -{ - vmw_sou_destroy(vmw_connector_to_sou(connector)); -} - static const struct drm_connector_funcs vmw_sou_connector_funcs = { .dpms = vmw_du_connector_dpms, .detect = vmw_du_connector_detect, .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = vmw_sou_connector_destroy, + .destroy = drm_connector_cleanup, .reset = vmw_du_connector_reset, .atomic_duplicate_state = vmw_du_connector_duplicate_state, .atomic_destroy_state = vmw_du_connector_destroy_state, @@ -755,7 +741,7 @@ vmw_sou_primary_plane_atomic_update(struct drm_plane *plane, static const struct drm_plane_funcs vmw_sou_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, - .destroy = vmw_du_primary_plane_destroy, + .destroy = drm_plane_cleanup, .reset = vmw_du_plane_reset, .atomic_duplicate_state = vmw_du_plane_duplicate_state, .atomic_destroy_state = vmw_du_plane_destroy_state, @@ -881,18 +867,12 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit) encoder->possible_crtcs = (1 << unit); encoder->possible_clones = 0; - ret = drm_connector_register(connector); - if (ret) { - DRM_ERROR("Failed to register connector\n"); - goto err_free_encoder; - } - ret = drm_crtc_init_with_planes(dev, crtc, primary, &cursor->base, &vmw_screen_object_crtc_funcs, NULL); if (ret) { DRM_ERROR("Failed to initialize CRTC\n"); - goto err_free_unregister; + goto err_free_encoder; } drm_crtc_helper_add(crtc, &vmw_sou_crtc_helper_funcs); @@ -910,8 +890,6 @@ static int vmw_sou_init(struct vmw_private *dev_priv, unsigned unit) return 0; -err_free_unregister: - drm_connector_unregister(connector); err_free_encoder: drm_encoder_cleanup(encoder); err_free_connector: diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c index 9deeee9b92da..eb3f64d2f54c 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c @@ -133,12 +133,6 @@ struct vmw_screen_target_display_unit { unsigned int cpp; }; - - -static void vmw_stdu_destroy(struct vmw_screen_target_display_unit *stdu); - - - /****************************************************************************** * Screen Target Display Unit CRTC Functions *****************************************************************************/ @@ -150,7 +144,11 @@ static void vmw_stdu_destroy(struct vmw_screen_target_display_unit *stdu); */ static void vmw_stdu_crtc_destroy(struct drm_crtc *crtc) { - vmw_stdu_destroy(vmw_crtc_to_stdu(crtc)); + struct vmw_screen_target_display_unit *stdu = vmw_crtc_to_stdu(crtc); + + vmw_vkms_crtc_cleanup(&stdu->base.crtc); + drm_crtc_cleanup(&stdu->base.crtc); + kfree(stdu); } /** @@ -806,23 +804,8 @@ static const struct drm_crtc_funcs vmw_stdu_crtc_funcs = { * Screen Target Display Unit Encoder Functions *****************************************************************************/ -/** - * vmw_stdu_encoder_destroy - cleans up the STDU - * - * @encoder: used the get the containing STDU - * - * vmwgfx cleans up crtc/encoder/connector all at the same time so technically - * this can be a no-op. Nevertheless, it doesn't hurt of have this in case - * the common KMS code changes and somehow vmw_stdu_crtc_destroy() doesn't - * get called. - */ -static void vmw_stdu_encoder_destroy(struct drm_encoder *encoder) -{ - vmw_stdu_destroy(vmw_encoder_to_stdu(encoder)); -} - static const struct drm_encoder_funcs vmw_stdu_encoder_funcs = { - .destroy = vmw_stdu_encoder_destroy, + .destroy = drm_encoder_cleanup, }; @@ -831,21 +814,6 @@ static const struct drm_encoder_funcs vmw_stdu_encoder_funcs = { * Screen Target Display Unit Connector Functions *****************************************************************************/ -/** - * vmw_stdu_connector_destroy - cleans up the STDU - * - * @connector: used to get the containing STDU - * - * vmwgfx cleans up crtc/encoder/connector all at the same time so technically - * this can be a no-op. Nevertheless, it doesn't hurt of have this in case - * the common KMS code changes and somehow vmw_stdu_crtc_destroy() doesn't - * get called. - */ -static void vmw_stdu_connector_destroy(struct drm_connector *connector) -{ - vmw_stdu_destroy(vmw_connector_to_stdu(connector)); -} - static enum drm_mode_status vmw_stdu_connector_mode_valid(struct drm_connector *connector, const struct drm_display_mode *mode) @@ -916,7 +884,7 @@ static const struct drm_connector_funcs vmw_stdu_connector_funcs = { .dpms = vmw_du_connector_dpms, .detect = vmw_du_connector_detect, .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = vmw_stdu_connector_destroy, + .destroy = drm_connector_cleanup, .reset = vmw_du_connector_reset, .atomic_duplicate_state = vmw_du_connector_duplicate_state, .atomic_destroy_state = vmw_du_connector_destroy_state, @@ -1482,7 +1450,7 @@ vmw_stdu_crtc_atomic_flush(struct drm_crtc *crtc, static const struct drm_plane_funcs vmw_stdu_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, - .destroy = vmw_du_primary_plane_destroy, + .destroy = drm_plane_cleanup, .reset = vmw_du_plane_reset, .atomic_duplicate_state = vmw_du_plane_duplicate_state, .atomic_destroy_state = vmw_du_plane_destroy_state, @@ -1616,18 +1584,12 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit) encoder->possible_crtcs = (1 << unit); encoder->possible_clones = 0; - ret = drm_connector_register(connector); - if (ret) { - DRM_ERROR("Failed to register connector\n"); - goto err_free_encoder; - } - ret = drm_crtc_init_with_planes(dev, crtc, primary, &cursor->base, &vmw_stdu_crtc_funcs, NULL); if (ret) { DRM_ERROR("Failed to initialize CRTC\n"); - goto err_free_unregister; + goto err_free_encoder; } drm_crtc_helper_add(crtc, &vmw_stdu_crtc_helper_funcs); @@ -1645,8 +1607,6 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit) return 0; -err_free_unregister: - drm_connector_unregister(connector); err_free_encoder: drm_encoder_cleanup(encoder); err_free_connector: @@ -1656,23 +1616,6 @@ static int vmw_stdu_init(struct vmw_private *dev_priv, unsigned unit) return ret; } - - -/** - * vmw_stdu_destroy - Cleans up a vmw_screen_target_display_unit - * - * @stdu: Screen Target Display Unit to be destroyed - * - * Clean up after vmw_stdu_init - */ -static void vmw_stdu_destroy(struct vmw_screen_target_display_unit *stdu) -{ - vmw_du_cleanup(&stdu->base); - kfree(stdu); -} - - - /****************************************************************************** * Screen Target Display KMS Functions * diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c index 7b8163b5e501..c223766ca900 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_vkms.c @@ -294,12 +294,8 @@ vmw_vkms_crtc_init(struct drm_crtc *crtc) void vmw_vkms_crtc_cleanup(struct drm_crtc *crtc) { - struct vmw_private *vmw = vmw_priv(crtc->dev); struct vmw_display_unit *du = vmw_crtc_to_du(crtc); - if (vmw->vkms_enabled) - drm_crtc_vblank_cancel_timer(crtc); - if (du->vkms.surface) vmw_surface_unreference(&du->vkms.surface); WARN_ON(work_pending(&du->vkms.crc_generator_work)); -- 2.54.0
