Re: [PATCH] Fix wrong assumptions in cea_for_each_detailed_block

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Sun, Nov 13, 2011 at 02:04:54AM +0100, Christian Schmidt wrote:
> The current logic misunderstands the spec about CEA 18byte descriptors.
> First, the spec doesn't state "detailed timing descriptors" but "18 byte
> descriptors", so any data record could be stored, mixed timings and
> other data, just as in the standard EDID.
> Second, the lower four bit of byte 3 of the CEA record do not contain
> the number of descriptors, but "the total number of DTDs defining native
> formats in the whole EDID [...], starting with the first DTD in the DTD
> list (which starts in the base EDID block)." A device can of course
> support non-native formats.
> 
> As such the number can't be used to determine n, and the existing code
> will filter non-timing 18byte descriptors anyway.
> 
> Signed-off-by: Christian Schmidt <schmidt@digadd,de>

> diff -ur linux-3.2-rc1.orig/drivers/gpu/drm/drm_edid.c linux-3.2-rc1/drivers/gpu/drm/drm_edid.c
> --- linux-3.2-rc1.orig/drivers/gpu/drm/drm_edid.c	2011-11-13 01:42:29.771092473 +0100
> +++ linux-3.2-rc1/drivers/gpu/drm/drm_edid.c	2011-11-13 01:54:32.031062983 +0100
> @@ -511,22 +511,7 @@
>  	u8 rev = ext[0x01], d = ext[0x02];
>  	u8 *det_base = ext + d;
>  
> -	switch (rev) {
> -	case 0:
> -		/* can't happen */
> -		return;
> -	case 1:
> -		/* have to infer how many blocks we have, check pixel clock */
> -		for (i = 0; i < 6; i++)
> -			if (det_base[18*i] || det_base[18*i+1])
> -				n++;
> -		break;
> -	default:
> -		/* explicit count */
> -		n = min(ext[0x03] & 0x0f, 6);
> -		break;
> -	}
> -
> +	n = (127 - d) / 18;
>  	for (i = 0; i < n; i++)
>  		cb((struct detailed_timing *)(det_base + 18 * i), closure);
>  }

I just stumbled on this same thing when looking at some internal patch.

Looks good, except you should also check that 'd' is less than 127.
I do wonder how may other unchecked buffer accesses there are in the
EDID code...

-- 
Ville Syrjälä
Intel OTC