Unchecked memory allocations in Intel driver

Unchecked memory allocations in Intel driver

[Alan Cox]

Found this going over intel_bios.c and cross comparing it with the
Intel/IMG driver intel_bios.c

        temp_mode = kzalloc(sizeof(*temp_mode), GFP_KERNEL);
        temp_downclock = panel_fixed_mode->clock;
        /*
         * enumerate the LVDS panel timing info entry in VBT to check
        whether
         * the LVDS downclock is found.
         */
        for (i = 0; i < 16; i++) {
                entry = (struct bdb_lvds_lfp_data_entry *)
                        ((uint8_t *)lvds_lfp_data->data + (lfp_data_size
        * i)); dvo_timing = (struct lvds_dvo_timing *)
                        ((unsigned char *)entry + dvo_timing_offset);
 
                fill_detail_timing_data(temp_mode, dvo_timing);


The one for IMG devices (GMA500 etc) has an additional unchecked kmalloc
which the i915 driver has fixed.

Re: Unchecked memory allocations in Intel driver

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 421 bytes --]

On Mon, 4 Jul 2011 13:27:51 +0100, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> Found this going over intel_bios.c and cross comparing it with the
> Intel/IMG driver intel_bios.c
> 
>         temp_mode = kzalloc(sizeof(*temp_mode), GFP_KERNEL);

This object is about 216 bytes long; would it be reasonable to allocate
it on the stack? Or is that pushing stack allocations too far?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Unchecked memory allocations in Intel driver

[Alan Cox]

On Mon, 04 Jul 2011 11:18:13 -0700
Keith Packard <keithp@keithp.com> wrote:

> On Mon, 4 Jul 2011 13:27:51 +0100, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > Found this going over intel_bios.c and cross comparing it with the
> > Intel/IMG driver intel_bios.c
> > 
> >         temp_mode = kzalloc(sizeof(*temp_mode), GFP_KERNEL);
> 
> This object is about 216 bytes long; would it be reasonable to allocate
> it on the stack? Or is that pushing stack allocations too far?

You'd have to review the rest of the call stack. Its pretty borderline.

Alan

[PATCH] drm/i915/bios: Avoid temporary allocation whilst searching for downclock

[Chris Wilson]

Alan Cox reported a missing check on the kmalloc return value for the
allocation of a temporary mode used for searching for the LVDS downlock
frequency. This allocation is roughly 200 bytes, a little too large to
friviously place on the stack. However, we can simply use the few bytes
we need stored within the original DVO timing data, skip the translation
and do the compare directly between the timing data rather than on a
mode, thus avoiding the need for any temporary allocations.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_bios.c |  142 +++++++++++++++++++++---------------
 1 files changed, 83 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index fb5b4d4..06a9f72 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -74,7 +74,7 @@ get_blocksize(void *p)
 
 static void
 fill_detail_timing_data(struct drm_display_mode *panel_fixed_mode,
-			struct lvds_dvo_timing *dvo_timing)
+			const struct lvds_dvo_timing *dvo_timing)
 {
 	panel_fixed_mode->hdisplay = (dvo_timing->hactive_hi << 8) |
 		dvo_timing->hactive_lo;
@@ -115,20 +115,75 @@ fill_detail_timing_data(struct drm_display_mode *panel_fixed_mode,
 	drm_mode_set_name(panel_fixed_mode);
 }
 
+static bool
+lvds_dvo_timing_equal_size(const struct lvds_dvo_timing *a,
+			   const struct lvds_dvo_timing *b)
+{
+	if (a->hactive_hi != b->hactive_hi ||
+	    a->hactive_lo != b->hactive_lo)
+		return false;
+
+	if (a->hsync_off_hi != b->hsync_off_hi ||
+	    a->hsync_off_lo != b->hsync_off_lo)
+		return false;
+
+	if (a->hsync_pulse_width != b->hsync_pulse_width)
+		return false;
+
+	if (a->hblank_hi != b->hblank_hi ||
+	    a->hblank_lo != b->hblank_lo)
+		return false;
+
+	if (a->vactive_hi != b->vactive_hi ||
+	    a->vactive_lo != b->vactive_lo)
+		return false;
+
+	if (a->vsync_off != b->vsync_off)
+		return false;
+
+	if (a->vsync_pulse_width != b->vsync_pulse_width)
+		return false;
+
+	if (a->vblank_hi != b->vblank_hi ||
+	    a->vblank_lo != b->vblank_lo)
+		return false;
+
+	return true;
+}
+
+static const struct lvds_dvo_timing *
+get_lvds_dvo_timing(const struct bdb_lvds_lfp_data *lvds_lfp_data,
+		    const struct bdb_lvds_lfp_data_ptrs *lvds_lfp_data_ptrs,
+		    int index)
+{
+	/*
+	 * the size of fp_timing varies on the different platform.
+	 * So calculate the DVO timing relative offset in LVDS data
+	 * entry to get the DVO timing entry
+	 */
+
+	int lfp_data_size =
+		lvds_lfp_data_ptrs->ptr[1].dvo_timing_offset -
+		lvds_lfp_data_ptrs->ptr[0].dvo_timing_offset;
+	int dvo_timing_offset =
+		lvds_lfp_data_ptrs->ptr[0].dvo_timing_offset -
+		lvds_lfp_data_ptrs->ptr[0].fp_timing_offset;
+	char *entry = (char *)lvds_lfp_data->data + lfp_data_size * index;
+
+	return (struct lvds_dvo_timing *)(entry + dvo_timing_offset);
+}
+
 /* Try to find integrated panel data */
 static void
 parse_lfp_panel_data(struct drm_i915_private *dev_priv,
 			    struct bdb_header *bdb)
 {
-	struct bdb_lvds_options *lvds_options;
-	struct bdb_lvds_lfp_data *lvds_lfp_data;
-	struct bdb_lvds_lfp_data_ptrs *lvds_lfp_data_ptrs;
-	struct bdb_lvds_lfp_data_entry *entry;
-	struct lvds_dvo_timing *dvo_timing;
+	const struct bdb_lvds_options *lvds_options;
+	const struct bdb_lvds_lfp_data *lvds_lfp_data;
+	const struct bdb_lvds_lfp_data_ptrs *lvds_lfp_data_ptrs;
+	const struct lvds_dvo_timing *panel_dvo_timing;
 	struct drm_display_mode *panel_fixed_mode;
-	int lfp_data_size, dvo_timing_offset;
-	int i, temp_downclock;
-	struct drm_display_mode *temp_mode;
+	int i, downclock;
 
 	lvds_options = find_section(bdb, BDB_LVDS_OPTIONS);
 	if (!lvds_options)
@@ -150,75 +205,44 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
 
 	dev_priv->lvds_vbt = 1;
 
-	lfp_data_size = lvds_lfp_data_ptrs->ptr[1].dvo_timing_offset -
-		lvds_lfp_data_ptrs->ptr[0].dvo_timing_offset;
-	entry = (struct bdb_lvds_lfp_data_entry *)
-		((uint8_t *)lvds_lfp_data->data + (lfp_data_size *
-						   lvds_options->panel_type));
-	dvo_timing_offset = lvds_lfp_data_ptrs->ptr[0].dvo_timing_offset -
-		lvds_lfp_data_ptrs->ptr[0].fp_timing_offset;
-
-	/*
-	 * the size of fp_timing varies on the different platform.
-	 * So calculate the DVO timing relative offset in LVDS data
-	 * entry to get the DVO timing entry
-	 */
-	dvo_timing = (struct lvds_dvo_timing *)
-			((unsigned char *)entry + dvo_timing_offset);
+	panel_dvo_timing = get_lvds_dvo_timing(lvds_lfp_data,
+					       lvds_lfp_data_ptrs,
+					       lvds_options->panel_type);
 
 	panel_fixed_mode = kzalloc(sizeof(*panel_fixed_mode), GFP_KERNEL);
 	if (!panel_fixed_mode)
 		return;
 
-	fill_detail_timing_data(panel_fixed_mode, dvo_timing);
+	fill_detail_timing_data(panel_fixed_mode, panel_dvo_timing);
 
 	dev_priv->lfp_lvds_vbt_mode = panel_fixed_mode;
 
 	DRM_DEBUG_KMS("Found panel mode in BIOS VBT tables:\n");
 	drm_mode_debug_printmodeline(panel_fixed_mode);
 
-	temp_mode = kzalloc(sizeof(*temp_mode), GFP_KERNEL);
-	temp_downclock = panel_fixed_mode->clock;
 	/*
-	 * enumerate the LVDS panel timing info entry in VBT to check whether
-	 * the LVDS downclock is found.
+	 * Iterate over the LVDS panel timing info to find the lowest clock
+	 * for the native resolution.
 	 */
+	downclock = panel_dvo_timing->clock;
 	for (i = 0; i < 16; i++) {
-		entry = (struct bdb_lvds_lfp_data_entry *)
-			((uint8_t *)lvds_lfp_data->data + (lfp_data_size * i));
-		dvo_timing = (struct lvds_dvo_timing *)
-			((unsigned char *)entry + dvo_timing_offset);
-
-		fill_detail_timing_data(temp_mode, dvo_timing);
-
-		if (temp_mode->hdisplay == panel_fixed_mode->hdisplay &&
-		temp_mode->hsync_start == panel_fixed_mode->hsync_start &&
-		temp_mode->hsync_end == panel_fixed_mode->hsync_end &&
-		temp_mode->htotal == panel_fixed_mode->htotal &&
-		temp_mode->vdisplay == panel_fixed_mode->vdisplay &&
-		temp_mode->vsync_start == panel_fixed_mode->vsync_start &&
-		temp_mode->vsync_end == panel_fixed_mode->vsync_end &&
-		temp_mode->vtotal == panel_fixed_mode->vtotal &&
-		temp_mode->clock < temp_downclock) {
-			/*
-			 * downclock is already found. But we expect
-			 * to find the lower downclock.
-			 */
-			temp_downclock = temp_mode->clock;
-		}
-		/* clear it to zero */
-		memset(temp_mode, 0, sizeof(*temp_mode));
+		const struct lvds_dvo_timing *dvo_timing;
+
+		dvo_timing = get_lvds_dvo_timing(lvds_lfp_data,
+						 lvds_lfp_data_ptrs,
+						 i);
+		if (lvds_dvo_timing_equal_size(dvo_timing, panel_dvo_timing) &&
+		    dvo_timing->clock < downclock)
+			downclock = dvo_timing->clock;
 	}
-	kfree(temp_mode);
-	if (temp_downclock < panel_fixed_mode->clock &&
-	    i915_lvds_downclock) {
+
+	if (downclock < panel_dvo_timing->clock && i915_lvds_downclock) {
 		dev_priv->lvds_downclock_avail = 1;
-		dev_priv->lvds_downclock = temp_downclock;
+		dev_priv->lvds_downclock = downclock * 10;
 		DRM_DEBUG_KMS("LVDS downclock is found in VBT. ",
 				"Normal Clock %dKHz, downclock %dKHz\n",
-				temp_downclock, panel_fixed_mode->clock);
+				panel_fixed_mode->clock, 10*downclock);
 	}
-	return;
 }
 
 /* Try to find sdvo panel data */
-- 
1.7.5.4

Re: [PATCH] drm/i915/bios: Avoid temporary allocation whilst searching for downclock

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 736 bytes --]

On Tue,  5 Jul 2011 09:55:34 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Alan Cox reported a missing check on the kmalloc return value for the
> allocation of a temporary mode used for searching for the LVDS downlock
> frequency. This allocation is roughly 200 bytes, a little too large to
> friviously place on the stack. However, we can simply use the few bytes
> we need stored within the original DVO timing data, skip the translation
> and do the compare directly between the timing data rather than on a
> mode, thus avoiding the need for any temporary allocations.

Can you clean this one up for -next? I'm merging bug fixes to -next to
send off for post-3.0 integration.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel