From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-f66.google.com ([209.85.208.66]:35708 "EHLO mail-ed1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726479AbeIUOnD (ORCPT ); Fri, 21 Sep 2018 10:43:03 -0400 Received: by mail-ed1-f66.google.com with SMTP id y20-v6so10119570edq.2 for ; Fri, 21 Sep 2018 01:55:11 -0700 (PDT) Date: Fri, 21 Sep 2018 10:55:07 +0200 From: Daniel Vetter To: Tomi Valkeinen Cc: dri-devel@lists.freedesktop.org, Daniel Vetter , Dave Airlie , stable@vger.kernel.org Subject: Re: [PATCH] drm: fix use of freed memory in drm_mode_setcrtc Message-ID: <20180921085507.GW11082@phenom.ffwll.local> References: <20180917110054.4053-1-tomi.valkeinen@ti.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180917110054.4053-1-tomi.valkeinen@ti.com> Sender: stable-owner@vger.kernel.org List-ID: 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. And I agree with Ville, I don't think we need a cc: stable here. Reviewed-by: Daniel Vetter > + > 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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch