From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] Fix wrong assumptions in cea_for_each_detailed_block Date: Thu, 1 Mar 2012 13:53:01 +0200 Message-ID: <20120301115301.GT3592@intel.com> References: <4EBF17B6.9060108@digadd.de> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 4C46F9F598 for ; Thu, 1 Mar 2012 03:45:58 -0800 (PST) Content-Disposition: inline In-Reply-To: <4EBF17B6.9060108@digadd.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Christian Schmidt Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 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 > diff -ur linux-3.2-rc1.orig/drivers/gpu/drm/drm_edid.c linux-3.2-rc1/driv= ers/gpu/drm/drm_edid.c > --- linux-3.2-rc1.orig/drivers/gpu/drm/drm_edid.c 2011-11-13 01:42:29.771= 092473 +0100 > +++ linux-3.2-rc1/drivers/gpu/drm/drm_edid.c 2011-11-13 01:54:32.03106298= 3 +0100 > @@ -511,22 +511,7 @@ > u8 rev =3D ext[0x01], d =3D ext[0x02]; > u8 *det_base =3D 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 =3D 0; i < 6; i++) > - if (det_base[18*i] || det_base[18*i+1]) > - n++; > - break; > - default: > - /* explicit count */ > - n =3D min(ext[0x03] & 0x0f, 6); > - break; > - } > - > + n =3D (127 - d) / 18; > for (i =3D 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=E4l=E4 Intel OTC