diff mbox

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

Message ID 1462375734-8213-1-git-send-email-cpaul@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

cpaul@redhat.com May 4, 2016, 3:28 p.m. UTC
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>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Daniel Vetter May 4, 2016, 4:17 p.m. UTC | #1
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
diff mbox

Patch

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);
 	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;
 	}