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
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html