Re: [PATCH 1/3] drm/i915/fbdev: Fix num_connector references in intel_fb_initial_config()

[Daniel Vetter]

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@xxxxxxxxxxxxxxx
> Signed-off-by: Lyude <cpaul@xxxxxxxxxx>

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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch