From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from fllv0016.ext.ti.com ([198.47.19.142]:46944 "EHLO fllv0016.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726765AbeIZPvM (ORCPT ); Wed, 26 Sep 2018 11:51:12 -0400 Subject: Re: [PATCH] drm: fix use of freed memory in drm_mode_setcrtc To: Daniel Vetter CC: , Daniel Vetter , Dave Airlie , References: <20180917110054.4053-1-tomi.valkeinen@ti.com> <20180921085507.GW11082@phenom.ffwll.local> From: Tomi Valkeinen Message-ID: Date: Wed, 26 Sep 2018 12:39:03 +0300 MIME-Version: 1.0 In-Reply-To: <20180921085507.GW11082@phenom.ffwll.local> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: On 21/09/18 11:55, Daniel Vetter wrote: > 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 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 >> 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; > > Bit a bikeshed, but I'd reset the pointers right after we release/free > them, in the out: block. Avoids accidental leaking. But it's fine either > way. I did that first, but then changed it to this version as this seemed (to me) to make it more clear what's going on. But I don't feel strongly either way, so if anyone else chimes in before I push, I can change it =). > And I agree with Ville, I don't think we need a cc: stable here. Ok, I'll drop the stable, and push to drm-misc-fixes. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki