Re: [PATCH] Fix wrong assumptions in cea_for_each_detailed_block

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

On Thu, Mar 01, 2012 at 01:53:01PM +0200, Ville Syrjälä wrote:
> 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...

Ah, didn't realize this was in already. I was looking at an older tree.

I'll send a patch to do the bounds checking...

-- 
Ville Syrjälä
Intel OTC