From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f66.google.com ([74.125.82.66]:35534 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751388AbcEDQRj (ORCPT ); Wed, 4 May 2016 12:17:39 -0400 Received: by mail-wm0-f66.google.com with SMTP id e201so11417036wme.2 for ; Wed, 04 May 2016 09:17:38 -0700 (PDT) Date: Wed, 4 May 2016 18:17:36 +0200 From: Daniel Vetter To: Lyude Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, open list , stable@vger.kernel.org, Daniel Vetter Subject: Re: [PATCH 1/3] drm/i915/fbdev: Fix num_connector references in intel_fb_initial_config() Message-ID: <20160504161736.GE1286@phenom.ffwll.local> References: <1462375734-8213-1-git-send-email-cpaul@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1462375734-8213-1-git-send-email-cpaul@redhat.com> Sender: stable-owner@vger.kernel.org List-ID: On Wed, May 04, 2016 at 11:28:51AM -0400, Lyude wrote: > During boot time, MST devices usually send a ton of hotplug events > irregardless of whether or not any physical hotplugs actually occurred. > Hotplugs mean connectors being created/destroyed, and the number of DRM > connectors changing under us. This isn't a problem if we use > fb_helper->connector_count since we only set it once in the code, > however if we use num_connector from struct drm_mode_config we risk it's > value changing under us. On top of that, there's even a chance that > dev->mode_config.num_connector != fb_helper->connector_count. If the > number of connectors happens to increase under us, we'll end up using > the wrong array size for memcpy and start writing beyond the actual > length of the array, occasionally resulting in kernel panics. > > Cc: stable@vger.kernel.org > Signed-off-by: Lyude So at first I thought this is impossible, because we hold the dev->mode_config.mutex in all relevant places. But we drop it in-between, so this can indeed race. > --- > drivers/gpu/drm/i915/intel_fbdev.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > index 97a91e6..c607217 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -366,12 +366,12 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper, > uint64_t conn_configured = 0, mask; > int pass = 0; > > - save_enabled = kcalloc(dev->mode_config.num_connector, sizeof(bool), > + save_enabled = kcalloc(fb_helper->connector_count, sizeof(bool), > GFP_KERNEL); > if (!save_enabled) > return false; > > - memcpy(save_enabled, enabled, dev->mode_config.num_connector); > + memcpy(save_enabled, enabled, fb_helper->connector_count); If num_connector < connector_count this can still go boom. I think we need to create a local variable int num_conn = min(..., ...); and then use that all throughout. Plus a big comment that fbdev setup drops locks and hence might become inconsistent. -Daniel > mask = (1 << fb_helper->connector_count) - 1; > retry: > for (i = 0; i < fb_helper->connector_count; i++) { > @@ -510,7 +510,7 @@ retry: > if (fallback) { > bail: > DRM_DEBUG_KMS("Not using firmware configuration\n"); > - memcpy(enabled, save_enabled, dev->mode_config.num_connector); > + memcpy(enabled, save_enabled, fb_helper->connector_count); > kfree(save_enabled); > return false; > } > -- > 2.5.5 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch