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:57:41 +0200 Message-ID: <20120301115741.GU3592@intel.com> References: <4EBF17B6.9060108@digadd.de> <20120301115301.GT3592@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id CB5D29F598 for ; Thu, 1 Mar 2012 03:50:38 -0800 (PST) Content-Disposition: inline In-Reply-To: <20120301115301.GT3592@intel.com> 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 Thu, Mar 01, 2012 at 01:53:01PM +0200, Ville Syrj=E4l=E4 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 > = > > diff -ur linux-3.2-rc1.orig/drivers/gpu/drm/drm_edid.c linux-3.2-rc1/dr= ivers/gpu/drm/drm_edid.c > > --- linux-3.2-rc1.orig/drivers/gpu/drm/drm_edid.c 2011-11-13 01:42:29.7= 71092473 +0100 > > +++ linux-3.2-rc1/drivers/gpu/drm/drm_edid.c 2011-11-13 01:54:32.031062= 983 +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... 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=E4l=E4 Intel OTC