drm_edid: potential range checking issue?

drm_edid: potential range checking issue?

[Dan Carpenter]

Hi list,  :)

Just going through some Smatch warnings.

drivers/gpu/drm/drm_edid.c +1032 add_detailed_modes() 'data->data.timings' 5 <= 5
  1027                  /* Six modes per detailed section */
  1028                  for (i = 0; i < 6; i++) {
  1029                          struct std_timing *std;
  1030                          struct drm_display_mode *newmode;
  1031
  1032                          std = &data->data.timings[i];
                                      ^^^^^^^^^^^^^^^^^^^^^^

In include/drm/drm_edid.h this array has 5 elements not 6.

struct detailed_non_pixel {
        u8 pad1;
        u8 type; /* ff=serial, fe=string, fd=monitor range, fc=monitor name
                    fb=color point data, fa=standard timing data,
                    f9=undefined, f8=mfg. reserved */
        u8 pad2;
        union {         
                struct detailed_data_string str;
                struct detailed_data_monitor_range range;
                struct detailed_data_wpindex color;
                struct std_timing timings[5];
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                struct cvt_timing cvt[4];
        } data; 
} __attribute__((packed));

regards,
dan carpenter

------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
--

Re: drm_edid: potential range checking issue?

[Dan Carpenter]

Ping.  This off-by-one overflow is still there in -next.

I'm not sure how to fix it because both the declaraion and the
use in add_detailed_modes() look deliberate.

regards,
dan carpenter

On Sun, Mar 28, 2010 at 02:25:58PM +0300, Dan Carpenter wrote:
> Hi list,  :)
> 
> Just going through some Smatch warnings.
> 
> drivers/gpu/drm/drm_edid.c +1032 add_detailed_modes() 'data->data.timings' 5 <= 5
>   1027                  /* Six modes per detailed section */
>   1028                  for (i = 0; i < 6; i++) {
>   1029                          struct std_timing *std;
>   1030                          struct drm_display_mode *newmode;
>   1031
>   1032                          std = &data->data.timings[i];
>                                       ^^^^^^^^^^^^^^^^^^^^^^
> 
> In include/drm/drm_edid.h this array has 5 elements not 6.
> 
> struct detailed_non_pixel {
>         u8 pad1;
>         u8 type; /* ff=serial, fe=string, fd=monitor range, fc=monitor name
>                     fb=color point data, fa=standard timing data,
>                     f9=undefined, f8=mfg. reserved */
>         u8 pad2;
>         union {         
>                 struct detailed_data_string str;
>                 struct detailed_data_monitor_range range;
>                 struct detailed_data_wpindex color;
>                 struct std_timing timings[5];
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>                 struct cvt_timing cvt[4];
>         } data; 
> } __attribute__((packed));
> 
> regards,
> dan carpenter

------------------------------------------------------------------------------

--

Re: drm_edid: potential range checking issue?

[Adam Jackson]

[-- Attachment #1.1: Type: text/plain, Size: 1163 bytes --]

On Fri, 2010-05-07 at 10:35 +0200, Dan Carpenter wrote:

> > In include/drm/drm_edid.h this array has 5 elements not 6.
> > 
> > struct detailed_non_pixel {
> >         u8 pad1;
> >         u8 type; /* ff=serial, fe=string, fd=monitor range, fc=monitor name
> >                     fb=color point data, fa=standard timing data,
> >                     f9=undefined, f8=mfg. reserved */
> >         u8 pad2;
> >         union {         
> >                 struct detailed_data_string str;
> >                 struct detailed_data_monitor_range range;
> >                 struct detailed_data_wpindex color;
> >                 struct std_timing timings[5];
> >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This decl is wrong, should be 6.  From the 1.4 spec:

"Six additional Standard Timings may be listed as a display descriptor
(tag #FAh)."

The 1.3 spec is a little less explicit about it, but does show 6
standard timing codes in the 0xFA detailed subblock, terminated by 0x0A
in the 18th byte.  I don't have the docs for 1.2 or earlier, but we're
paranoid enough about not adding broken timings that we should be fine.

- ajax

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 80 bytes --]

------------------------------------------------------------------------------


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

--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel

[patch] drm_edid: There should be 6 Standard Timings

[Dan Carpenter]

Smatch complained that we initialize 6 elements in add_detailed_modes() 
but the timings[] array is declared with 5 elements.  Adam Jackson 
verified that 6 is the correct number of timings.

On Mon, May 10, 2010 at 12:08:24PM -0400, Adam Jackson wrote:
> > >                 struct std_timing timings[5];
> > >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> This decl is wrong, should be 6.  From the 1.4 spec:
> 
> "Six additional Standard Timings may be listed as a display descriptor
> (tag #FAh)."
> 
> The 1.3 spec is a little less explicit about it, but does show 6
> standard timing codes in the 0xFA detailed subblock, terminated by 0x0A
> in the 18th byte.  I don't have the docs for 1.2 or earlier, but we're
> paranoid enough about not adding broken timings that we should be fine.

This patch is basically a clean up, because timings[] is declared inside
a union and increasing the number of elements here doesn't change the
overall size of the union.

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
This patch applies to drm-next.

diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index d33c3e0..39e2cc5 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -120,7 +120,7 @@ struct detailed_non_pixel {
 		struct detailed_data_string str;
 		struct detailed_data_monitor_range range;
 		struct detailed_data_wpindex color;
-		struct std_timing timings[5];
+		struct std_timing timings[6];
 		struct cvt_timing cvt[4];
 	} data;
 } __attribute__((packed));

------------------------------------------------------------------------------

--

Re: [patch] drm_edid: There should be 6 Standard Timings

[Adam Jackson]

[-- Attachment #1.1: Type: text/plain, Size: 1202 bytes --]

On Fri, 2010-05-14 at 13:06 +0200, Dan Carpenter wrote:
> Smatch complained that we initialize 6 elements in add_detailed_modes() 
> but the timings[] array is declared with 5 elements.  Adam Jackson 
> verified that 6 is the correct number of timings.
> 
> On Mon, May 10, 2010 at 12:08:24PM -0400, Adam Jackson wrote:
> > > >                 struct std_timing timings[5];
> > > >                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > This decl is wrong, should be 6.  From the 1.4 spec:
> > 
> > "Six additional Standard Timings may be listed as a display descriptor
> > (tag #FAh)."
> > 
> > The 1.3 spec is a little less explicit about it, but does show 6
> > standard timing codes in the 0xFA detailed subblock, terminated by 0x0A
> > in the 18th byte.  I don't have the docs for 1.2 or earlier, but we're
> > paranoid enough about not adding broken timings that we should be fine.
> 
> This patch is basically a clean up, because timings[] is declared inside
> a union and increasing the number of elements here doesn't change the
> overall size of the union.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>

Reviewed-by: Adam Jackson <ajax@redhat.com>

- ajax

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 80 bytes --]

------------------------------------------------------------------------------


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

--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel