diff mbox

[Karmic,SRU] drm/i915: Avoid NULL dereference with component_only tv_modes

Message ID 4B1E7092.8010307@canonical.com
State Accepted
Delegated to: Stefan Bader
Headers show

Commit Message

Steve Conklin Dec. 8, 2009, 3:28 p.m. UTC 
SRU Justification: This patch fixes a regression that has
caused problems for users of another distribution. We
probably have users experiencing the same problem.

The patch is sane and minimal. The patch is in drm-next,
but not yet in Linus's tree or stable.

Bug #494045

----------------

From d271817baecbccb47da0d9f28c285a0dae8a06b7 Mon Sep 17 00:00:00 2001
From: Chris Wilson <chris@chris-wilson.co.uk>
Date: Fri, 27 Nov 2009 13:06:56 +0000
Subject: [PATCH] drm/i915: Avoid NULL dereference with component_only tv_modes

In commit d2d9f2324, the guard for a valid video mode was removed. This
caused the regression:

  kernel crash during kms graphic boot on Intel GM4500 platform
  https://bugzilla.redhat.com/show_bug.cgi?id=540218

This patches changes the logic slightly not to rely on a coupled
variable, but to just check whether the video_modes is valid before
dereferencing.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Zhenyu Wang <zhenyu.z.wang@intel.com>
[ickle: Actually reference the correct bug report]
Acked-by: Zhenyu Wang <zhenyuw@linux.intel.com>
Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/i915/intel_tv.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

Comments

Leann Ogasawara Dec. 8, 2009, 5:20 p.m. UTC | #1 
On Tue, 2009-12-08 at 09:28 -0600, Steve Conklin wrote:
> SRU Justification: This patch fixes a regression that has
> caused problems for users of another distribution. We
> probably have users experiencing the same problem.

Just for a data point, I had bdmurray do a quick query of all
OopsText.txt files attached to Launchpad bug reports to see if there
were any existing bugs which would be resolved by this patch.  The
search came back empty.

> The patch is sane and minimal. The patch is in drm-next,
> but not yet in Linus's tree or stable.

Aside from the unnecessary cosmetic changes, this patch looks sane as
it's checking that video_levels is not null before dereferencing.  There
should be a very low risk of regression.

My only concern is that this is deviating slightly from our normal SRU
policy that the patch must be in Linus' tree first and that we
preferably want to have the patch merged into Karmic via upstream
stable.  Stefan, your thoughts?

As far as the content of the patch itself, I'll give it my ack.

Acked-by: Leann Ogasawara <leann.ogasawara@canonical.com>

Thanks,
Leann

> Bug #494045
> 
> ----------------
> 
> From d271817baecbccb47da0d9f28c285a0dae8a06b7 Mon Sep 17 00:00:00 2001
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Fri, 27 Nov 2009 13:06:56 +0000
> Subject: [PATCH] drm/i915: Avoid NULL dereference with component_only tv_modes
> 
> In commit d2d9f2324, the guard for a valid video mode was removed. This
> caused the regression:
> 
>   kernel crash during kms graphic boot on Intel GM4500 platform
>   https://bugzilla.redhat.com/show_bug.cgi?id=540218
> 
> This patches changes the logic slightly not to rely on a coupled
> variable, but to just check whether the video_modes is valid before
> dereferencing.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Zhenyu Wang <zhenyu.z.wang@intel.com>
> [ickle: Actually reference the correct bug report]
> Acked-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  drivers/gpu/drm/i915/intel_tv.c |   11 ++++-------
>  1 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index a0e4bc4..d7465ca 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1213,20 +1213,17 @@ intel_tv_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
>  		tv_ctl |= TV_TRILEVEL_SYNC;
>  	if (tv_mode->pal_burst)
>  		tv_ctl |= TV_PAL_BURST;
> +
>  	scctl1 = 0;
> -	/* dda1 implies valid video levels */
> -	if (tv_mode->dda1_inc) {
> +	if (tv_mode->dda1_inc)
>  		scctl1 |= TV_SC_DDA1_EN;
> -	}
> -
>  	if (tv_mode->dda2_inc)
>  		scctl1 |= TV_SC_DDA2_EN;
> -
>  	if (tv_mode->dda3_inc)
>  		scctl1 |= TV_SC_DDA3_EN;
> -
>  	scctl1 |= tv_mode->sc_reset;
> -	scctl1 |= video_levels->burst << TV_BURST_LEVEL_SHIFT;
> +	if (video_levels)
> +		scctl1 |= video_levels->burst << TV_BURST_LEVEL_SHIFT;
>  	scctl1 |= tv_mode->dda1_inc << TV_SCDDA1_INC_SHIFT;
> 
>  	scctl2 = tv_mode->dda2_size << TV_SCDDA2_SIZE_SHIFT |
> -- 
> 1.6.3.3
Stefan Bader Dec. 8, 2009, 5:31 p.m. UTC | #2 
Steve Conklin wrote:
> SRU Justification: This patch fixes a regression that has
> caused problems for users of another distribution. We
> probably have users experiencing the same problem.
> 
> The patch is sane and minimal. The patch is in drm-next,
> but not yet in Linus's tree or stable.
> 
> Bug #494045
> 
> ----------------
> 
> From d271817baecbccb47da0d9f28c285a0dae8a06b7 Mon Sep 17 00:00:00 2001
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Fri, 27 Nov 2009 13:06:56 +0000
> Subject: [PATCH] drm/i915: Avoid NULL dereference with component_only tv_modes
> 
> In commit d2d9f2324, the guard for a valid video mode was removed. This
> caused the regression:
> 
>   kernel crash during kms graphic boot on Intel GM4500 platform
>   https://bugzilla.redhat.com/show_bug.cgi?id=540218
> 
> This patches changes the logic slightly not to rely on a coupled
> variable, but to just check whether the video_modes is valid before
> dereferencing.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Zhenyu Wang <zhenyu.z.wang@intel.com>
> [ickle: Actually reference the correct bug report]
> Acked-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  drivers/gpu/drm/i915/intel_tv.c |   11 ++++-------
>  1 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index a0e4bc4..d7465ca 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1213,20 +1213,17 @@ intel_tv_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
>  		tv_ctl |= TV_TRILEVEL_SYNC;
>  	if (tv_mode->pal_burst)
>  		tv_ctl |= TV_PAL_BURST;
> +
>  	scctl1 = 0;
> -	/* dda1 implies valid video levels */
> -	if (tv_mode->dda1_inc) {
> +	if (tv_mode->dda1_inc)
>  		scctl1 |= TV_SC_DDA1_EN;
> -	}
> -
>  	if (tv_mode->dda2_inc)
>  		scctl1 |= TV_SC_DDA2_EN;
> -
>  	if (tv_mode->dda3_inc)
>  		scctl1 |= TV_SC_DDA3_EN;
> -
>  	scctl1 |= tv_mode->sc_reset;
> -	scctl1 |= video_levels->burst << TV_BURST_LEVEL_SHIFT;
> +	if (video_levels)
> +		scctl1 |= video_levels->burst << TV_BURST_LEVEL_SHIFT;
>  	scctl1 |= tv_mode->dda1_inc << TV_SCDDA1_INC_SHIFT;
> 
>  	scctl2 = tv_mode->dda2_size << TV_SCDDA2_SIZE_SHIFT |

My preference would also be to get this over to upstream+stable
(with or without the cosmetically change) and then back to us.
Seems there is at least one more Karmic update after 2.6.31.7
from what I read from Greg's comments.

-Stefan
Stefan Bader Dec. 16, 2009, 11:22 p.m. UTC | #3 
Steve Conklin wrote:
> SRU Justification: This patch fixes a regression that has
> caused problems for users of another distribution. We
> probably have users experiencing the same problem.
> 
> The patch is sane and minimal. The patch is in drm-next,
> but not yet in Linus's tree or stable.
> 
> Bug #494045
> 

Ok, as this is upstream now and a regression/oops I took the
patch and forwarded it to stable.
I also applied it to the Karmic tree as a potential NULL pointer
violation is of high enough severity.

-Stefan


> ----------------
> 
> From d271817baecbccb47da0d9f28c285a0dae8a06b7 Mon Sep 17 00:00:00 2001
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Fri, 27 Nov 2009 13:06:56 +0000
> Subject: [PATCH] drm/i915: Avoid NULL dereference with component_only tv_modes
> 
> In commit d2d9f2324, the guard for a valid video mode was removed. This
> caused the regression:
> 
>   kernel crash during kms graphic boot on Intel GM4500 platform
>   https://bugzilla.redhat.com/show_bug.cgi?id=540218
> 
> This patches changes the logic slightly not to rely on a coupled
> variable, but to just check whether the video_modes is valid before
> dereferencing.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Zhenyu Wang <zhenyu.z.wang@intel.com>
> [ickle: Actually reference the correct bug report]
> Acked-by: Zhenyu Wang <zhenyuw@linux.intel.com>
> Signed-off-by: Eric Anholt <eric@anholt.net>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  drivers/gpu/drm/i915/intel_tv.c |   11 ++++-------
>  1 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index a0e4bc4..d7465ca 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1213,20 +1213,17 @@ intel_tv_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
>  		tv_ctl |= TV_TRILEVEL_SYNC;
>  	if (tv_mode->pal_burst)
>  		tv_ctl |= TV_PAL_BURST;
> +
>  	scctl1 = 0;
> -	/* dda1 implies valid video levels */
> -	if (tv_mode->dda1_inc) {
> +	if (tv_mode->dda1_inc)
>  		scctl1 |= TV_SC_DDA1_EN;
> -	}
> -
>  	if (tv_mode->dda2_inc)
>  		scctl1 |= TV_SC_DDA2_EN;
> -
>  	if (tv_mode->dda3_inc)
>  		scctl1 |= TV_SC_DDA3_EN;
> -
>  	scctl1 |= tv_mode->sc_reset;
> -	scctl1 |= video_levels->burst << TV_BURST_LEVEL_SHIFT;
> +	if (video_levels)
> +		scctl1 |= video_levels->burst << TV_BURST_LEVEL_SHIFT;
>  	scctl1 |= tv_mode->dda1_inc << TV_SCDDA1_INC_SHIFT;
> 
>  	scctl2 = tv_mode->dda2_size << TV_SCDDA2_SIZE_SHIFT |
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index a0e4bc4..d7465ca 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1213,20 +1213,17 @@  intel_tv_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 		tv_ctl |= TV_TRILEVEL_SYNC;
 	if (tv_mode->pal_burst)
 		tv_ctl |= TV_PAL_BURST;
+
 	scctl1 = 0;
-	/* dda1 implies valid video levels */
-	if (tv_mode->dda1_inc) {
+	if (tv_mode->dda1_inc)
 		scctl1 |= TV_SC_DDA1_EN;
-	}
-
 	if (tv_mode->dda2_inc)
 		scctl1 |= TV_SC_DDA2_EN;
-
 	if (tv_mode->dda3_inc)
 		scctl1 |= TV_SC_DDA3_EN;
-
 	scctl1 |= tv_mode->sc_reset;
-	scctl1 |= video_levels->burst << TV_BURST_LEVEL_SHIFT;
+	if (video_levels)
+		scctl1 |= video_levels->burst << TV_BURST_LEVEL_SHIFT;
 	scctl1 |= tv_mode->dda1_inc << TV_SCDDA1_INC_SHIFT;

 	scctl2 = tv_mode->dda2_size << TV_SCDDA2_SIZE_SHIFT |