Re: [PATCH] drm: fix use of freed memory in drm_mode_setcrtc

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: dri-devel@lists.freedesktop.org,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Dave Airlie <airlied@gmail.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH] drm: fix use of freed memory in drm_mode_setcrtc
Date: Mon, 17 Sep 2018 17:41:50 +0300	[thread overview]
Message-ID: <20180917144150.GN5565@intel.com> (raw)
In-Reply-To: <20180917110054.4053-1-tomi.valkeinen@ti.com>

On Mon, Sep 17, 2018 at 02:00:54PM +0300, Tomi Valkeinen wrote:
> drm_mode_setcrtc() retries modesetting in case one of the functions it
> calls returns -EDEADLK. connector_set, mode and fb are freed before
> retrying, but they are not set to NULL. This can cause
> drm_mode_setcrtc() to use those variables.
> 
> For example: On the first try __drm_mode_set_config_internal() returns
> -EDEADLK. connector_set, mode and fb are freed. Next retry starts, and
> drm_modeset_lock_all_ctx() returns -EDEADLK, and we jump to 'out'. The
> code will happily try to release all three again.

This thing uses lock_all() so I guess the EDEADLK must be coming from
some private locks in the driver?

Anyways, patch looks good so
Reviewed-by: Ville Syrjï¿½lï¿½ <ville.syrjala@linux.intel.com>

> 
> This leads to crashes of different kinds, depending on the sequence the
> EDEADLKs happen.
> 
> Fix this by setting the three variables to NULL at the start of the
> retry loop.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/drm_crtc.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 2f6c877299e4..2ad14593fb23 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -570,9 +570,9 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  	struct drm_mode_crtc *crtc_req = data;
>  	struct drm_crtc *crtc;
>  	struct drm_plane *plane;
> -	struct drm_connector **connector_set = NULL, *connector;
> -	struct drm_framebuffer *fb = NULL;
> -	struct drm_display_mode *mode = NULL;
> +	struct drm_connector **connector_set, *connector;
> +	struct drm_framebuffer *fb;
> +	struct drm_display_mode *mode;
>  	struct drm_mode_set set;
>  	uint32_t __user *set_connectors_ptr;
>  	struct drm_modeset_acquire_ctx ctx;
> @@ -601,6 +601,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  	mutex_lock(&crtc->dev->mode_config.mutex);
>  	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
>  retry:
> +	connector_set = NULL;
> +	fb = NULL;
> +	mode = NULL;
> +
>  	ret = drm_modeset_lock_all_ctx(crtc->dev, &ctx);
>  	if (ret)
>  		goto out;
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjï¿½lï¿½
Intel