lkml.org 
[lkml]   [2016]   [May]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/3] drm/i915/fbdev: Fix num_connector references in intel_fb_initial_config()
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 <cpaul@redhat.com>

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

\
 
 \ /
  Last update: 2016-05-04 18:41    [W:0.092 / U:0.368 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site