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

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

[Lyude]

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(-)

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

[PATCH 2/3] drm/fb_helper: Fix references to dev->mode_config.num_connector

[Lyude]

During boot, MST hotplugs are generally expected (even if no physical
hotplugging occurs) and result in DRM's connector topology changing.
This means that using num_connector from the current mode configuration
can lead to the number of connectors changing under us. This can lead to
some nasty scenarios in fbcon:

- We allocate an array to the size of dev->mode_config.num_connectors.
- MST hotplug occurs, dev->mode_config.num_connectors gets incremented.
- We try to loop through each element in the array using the new value
  of dev->mode_config.num_connectors, and end up going out of bounds
  since dev->mode_config.num_connectors is now larger then the array we
  allocated.

fb_helper->connector_count however, will always remain consistent while
we do a modeset in fb_helper.

Cc: stable@vger.kernel.org
Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 855108e..15204c0 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1914,7 +1914,7 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
 	if (modes[n] == NULL)
 		return best_score;
 
-	crtcs = kzalloc(dev->mode_config.num_connector *
+	crtcs = kzalloc(fb_helper->connector_count *
 			sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL);
 	if (!crtcs)
 		return best_score;
@@ -1960,7 +1960,7 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
 		if (score > best_score) {
 			best_score = score;
 			memcpy(best_crtcs, crtcs,
-			       dev->mode_config.num_connector *
+			       fb_helper->connector_count *
 			       sizeof(struct drm_fb_helper_crtc *));
 		}
 	}
-- 
2.5.5

Re: [PATCH 2/3] drm/fb_helper: Fix references to dev->mode_config.num_connector

[Daniel Vetter]

On Wed, May 04, 2016 at 11:28:52AM -0400, Lyude wrote:
> During boot, MST hotplugs are generally expected (even if no physical
> hotplugging occurs) and result in DRM's connector topology changing.
> This means that using num_connector from the current mode configuration
> can lead to the number of connectors changing under us. This can lead to
> some nasty scenarios in fbcon:
> 
> - We allocate an array to the size of dev->mode_config.num_connectors.
> - MST hotplug occurs, dev->mode_config.num_connectors gets incremented.
> - We try to loop through each element in the array using the new value
>   of dev->mode_config.num_connectors, and end up going out of bounds
>   since dev->mode_config.num_connectors is now larger then the array we
>   allocated.
> 
> fb_helper->connector_count however, will always remain consistent while
> we do a modeset in fb_helper.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Lyude <cpaul@redhat.com>

Ok, this one looks like it's indeed in the same critical section (within
drm_setup_outputs) as where we allocate the fb helper connector array. So
I think this one here is not needed to fix a bug?

Still makes sense as a patch, but not with cc: stable I think.
-Daniel

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 855108e..15204c0 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1914,7 +1914,7 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>  	if (modes[n] == NULL)
>  		return best_score;
>  
> -	crtcs = kzalloc(dev->mode_config.num_connector *
> +	crtcs = kzalloc(fb_helper->connector_count *
>  			sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL);
>  	if (!crtcs)
>  		return best_score;
> @@ -1960,7 +1960,7 @@ static int drm_pick_crtcs(struct drm_fb_helper *fb_helper,
>  		if (score > best_score) {
>  			best_score = score;
>  			memcpy(best_crtcs, crtcs,
> -			       dev->mode_config.num_connector *
> +			       fb_helper->connector_count *
>  			       sizeof(struct drm_fb_helper_crtc *));
>  		}
>  	}
> -- 
> 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

Re: [PATCH 2/3] drm/fb_helper: Fix references to dev->mode_config.num_connector

[Lyude Paul]

I would Cc it to stable just in case tbh. I picked up on this one since KASAN
was picking up on some of the memcpy() calls around here going out of bounds. I
can include some of the backtraces from that if needed.

On Wed, 2016-05-04 at 19:11 +0200, Daniel Vetter wrote:
> On Wed, May 04, 2016 at 11:28:52AM -0400, Lyude wrote:
> > 
> > During boot, MST hotplugs are generally expected (even if no physical
> > hotplugging occurs) and result in DRM's connector topology changing.
> > This means that using num_connector from the current mode configuration
> > can lead to the number of connectors changing under us. This can lead to
> > some nasty scenarios in fbcon:
> > 
> > - We allocate an array to the size of dev->mode_config.num_connectors.
> > - MST hotplug occurs, dev->mode_config.num_connectors gets incremented.
> > - We try to loop through each element in the array using the new value
> >   of dev->mode_config.num_connectors, and end up going out of bounds
> >   since dev->mode_config.num_connectors is now larger then the array we
> >   allocated.
> > 
> > fb_helper->connector_count however, will always remain consistent while
> > we do a modeset in fb_helper.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Lyude <cpaul@redhat.com>
> Ok, this one looks like it's indeed in the same critical section (within
> drm_setup_outputs) as where we allocate the fb helper connector array. So
> I think this one here is not needed to fix a bug?
> 
> Still makes sense as a patch, but not with cc: stable I think.
> -Daniel
> 
> > 
> > ---
> >  drivers/gpu/drm/drm_fb_helper.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c
> > b/drivers/gpu/drm/drm_fb_helper.c
> > index 855108e..15204c0 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -1914,7 +1914,7 @@ static int drm_pick_crtcs(struct drm_fb_helper
> > *fb_helper,
> >  	if (modes[n] == NULL)
> >  		return best_score;
> >  
> > -	crtcs = kzalloc(dev->mode_config.num_connector *
> > +	crtcs = kzalloc(fb_helper->connector_count *
> >  			sizeof(struct drm_fb_helper_crtc *), GFP_KERNEL);
> >  	if (!crtcs)
> >  		return best_score;
> > @@ -1960,7 +1960,7 @@ static int drm_pick_crtcs(struct drm_fb_helper
> > *fb_helper,
> >  		if (score > best_score) {
> >  			best_score = score;
> >  			memcpy(best_crtcs, crtcs,
> > -			       dev->mode_config.num_connector *
> > +			       fb_helper->connector_count *
> >  			       sizeof(struct drm_fb_helper_crtc *));
> >  		}
> >  	}
> > -- 
> > 2.5.5
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- 
Cheers,
	Lyude

[PATCH 3/3] drm/fb_helper: Fix a few typos

[Lyude]

s/modest/modeset/
s/aftert/after/

Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 15204c0..7778a0e 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -2104,8 +2104,8 @@ out:
  * cmdline option.
  *
  * The other option is to just disable fbdev emulation since very likely the
- * first modest from userspace will crash in the same way, and is even easier to
- * debug. This can be done by setting the drm_kms_helper.fbdev_emulation=0
+ * first modeset from userspace will crash in the same way, and is even easier
+ * to debug. This can be done by setting the drm_kms_helper.fbdev_emulation=0
  * kernel cmdline option.
  *
  * RETURNS:
@@ -2150,7 +2150,7 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
  * hotplug interrupt).
  *
  * Note that drivers may call this even before calling
- * drm_fb_helper_initial_config but only aftert drm_fb_helper_init. This allows
+ * drm_fb_helper_initial_config but only after drm_fb_helper_init. This allows
  * for a race-free fbcon setup and will make sure that the fbdev emulation will
  * not miss any hotplug events.
  *
-- 
2.5.5

Re: [Intel-gfx] [PATCH 3/3] drm/fb_helper: Fix a few typos

[Daniel Vetter]

On Wed, May 04, 2016 at 11:28:53AM -0400, Lyude wrote:
> s/modest/modeset/
> s/aftert/after/
> 
> Signed-off-by: Lyude <cpaul@redhat.com>

Applied to drm-misc, thanks.
-Daniel

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 15204c0..7778a0e 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2104,8 +2104,8 @@ out:
>   * cmdline option.
>   *
>   * The other option is to just disable fbdev emulation since very likely the
> - * first modest from userspace will crash in the same way, and is even easier to
> - * debug. This can be done by setting the drm_kms_helper.fbdev_emulation=0
> + * first modeset from userspace will crash in the same way, and is even easier
> + * to debug. This can be done by setting the drm_kms_helper.fbdev_emulation=0
>   * kernel cmdline option.
>   *
>   * RETURNS:
> @@ -2150,7 +2150,7 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config);
>   * hotplug interrupt).
>   *
>   * Note that drivers may call this even before calling
> - * drm_fb_helper_initial_config but only aftert drm_fb_helper_init. This allows
> + * drm_fb_helper_initial_config but only after drm_fb_helper_init. This allows
>   * for a race-free fbcon setup and will make sure that the fbdev emulation will
>   * not miss any hotplug events.
>   *
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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@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