[PATCH] Fix wrong assumptions in cea_for_each_detailed_block

[PATCH] Fix wrong assumptions in cea_for_each_detailed_block

[Christian Schmidt]

[-- Attachment #1: Type: text/plain, Size: 742 bytes --]

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>

[-- Attachment #2: fix_cea_for_each_detailed_block.patch --]
[-- Type: text/x-patch, Size: 774 bytes --]

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);
 }

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

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

Re: [PATCH] Fix wrong assumptions in cea_for_each_detailed_block

[Ville Syrjälä]

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

Re: [PATCH] Fix wrong assumptions in cea_for_each_detailed_block

[Ville Syrjälä]

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