diff mbox series

[2/8] drm/edid: Don't accept any old garbage as a display descriptor

Message ID 20200124200231.10517-2-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [1/8] drm/edid: Check the number of detailed timing descriptors in the CEA ext block | expand

Commit Message

Ville Syrjälä Jan. 24, 2020, 8:02 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we assume any 18 byte descriptor to be a display descritor
if only the tag byte matches the expected value. But for detailed
timing descriptors that same byte is just the lower 8 bits of
hblank, and as such can match any display descriptor tag. To
properly validate that the 18 byte descriptor is in fact a
display descriptor we must also examine bytes 0-2 (just byte 1
should actually suffice but the spec does say that bytes 0 and
2 must also always be zero for display descriptors so we check
those too).

Unlike Allen's original proposed patch to just fix is_rb() we
roll this out across the board to fix everything.

Cc: Allen Chen <allen.chen@ite.com.tw>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_edid.c | 65 ++++++++++++++++++++++++--------------
 1 file changed, 41 insertions(+), 24 deletions(-)

Comments

Alex Deucher Jan. 27, 2020, 10:35 p.m. UTC | #1 
On Fri, Jan 24, 2020 at 3:02 PM Ville Syrjala
<ville.syrjala@linux.intel.com> wrote:
>
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently we assume any 18 byte descriptor to be a display descritor
> if only the tag byte matches the expected value. But for detailed
> timing descriptors that same byte is just the lower 8 bits of
> hblank, and as such can match any display descriptor tag. To
> properly validate that the 18 byte descriptor is in fact a
> display descriptor we must also examine bytes 0-2 (just byte 1
> should actually suffice but the spec does say that bytes 0 and
> 2 must also always be zero for display descriptors so we check
> those too).
>
> Unlike Allen's original proposed patch to just fix is_rb() we
> roll this out across the board to fix everything.
>
> Cc: Allen Chen <allen.chen@ite.com.tw>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Acked-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/drm_edid.c | 65 ++++++++++++++++++++++++--------------
>  1 file changed, 41 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 1b6e544cf5c7..96ae1fde4ce2 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2196,6 +2196,12 @@ struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_mode_find_dmt);
>
> +static bool is_display_descriptor(const u8 d[18], u8 tag)
> +{
> +       return d[0] == 0x00 && d[1] == 0x00 &&
> +               d[2] == 0x00 && d[3] == tag;
> +}
> +
>  typedef void detailed_cb(struct detailed_timing *timing, void *closure);
>
>  static void
> @@ -2257,9 +2263,12 @@ static void
>  is_rb(struct detailed_timing *t, void *data)
>  {
>         u8 *r = (u8 *)t;
> -       if (r[3] == EDID_DETAIL_MONITOR_RANGE)
> -               if (r[15] & 0x10)
> -                       *(bool *)data = true;
> +
> +       if (!is_display_descriptor(r, EDID_DETAIL_MONITOR_RANGE))
> +               return;
> +
> +       if (r[15] & 0x10)
> +               *(bool *)data = true;
>  }
>
>  /* EDID 1.4 defines this explicitly.  For EDID 1.3, we guess, badly. */
> @@ -2279,7 +2288,11 @@ static void
>  find_gtf2(struct detailed_timing *t, void *data)
>  {
>         u8 *r = (u8 *)t;
> -       if (r[3] == EDID_DETAIL_MONITOR_RANGE && r[10] == 0x02)
> +
> +       if (!is_display_descriptor(r, EDID_DETAIL_MONITOR_RANGE))
> +               return;
> +
> +       if (r[10] == 0x02)
>                 *(u8 **)data = r;
>  }
>
> @@ -2818,7 +2831,7 @@ do_inferred_modes(struct detailed_timing *timing, void *c)
>         struct detailed_non_pixel *data = &timing->data.other_data;
>         struct detailed_data_monitor_range *range = &data->data.range;
>
> -       if (data->type != EDID_DETAIL_MONITOR_RANGE)
> +       if (!is_display_descriptor((const u8 *)timing, EDID_DETAIL_MONITOR_RANGE))
>                 return;
>
>         closure->modes += drm_dmt_modes_for_range(closure->connector,
> @@ -2897,10 +2910,11 @@ static void
>  do_established_modes(struct detailed_timing *timing, void *c)
>  {
>         struct detailed_mode_closure *closure = c;
> -       struct detailed_non_pixel *data = &timing->data.other_data;
>
> -       if (data->type == EDID_DETAIL_EST_TIMINGS)
> -               closure->modes += drm_est3_modes(closure->connector, timing);
> +       if (!is_display_descriptor((const u8 *)timing, EDID_DETAIL_EST_TIMINGS))
> +               return;
> +
> +       closure->modes += drm_est3_modes(closure->connector, timing);
>  }
>
>  /**
> @@ -2949,19 +2963,19 @@ do_standard_modes(struct detailed_timing *timing, void *c)
>         struct detailed_non_pixel *data = &timing->data.other_data;
>         struct drm_connector *connector = closure->connector;
>         struct edid *edid = closure->edid;
> +       int i;
>
> -       if (data->type == EDID_DETAIL_STD_MODES) {
> -               int i;
> -               for (i = 0; i < 6; i++) {
> -                       struct std_timing *std;
> -                       struct drm_display_mode *newmode;
> +       if (!is_display_descriptor((const u8 *)timing, EDID_DETAIL_STD_MODES))
> +               return;
>
> -                       std = &data->data.timings[i];
> -                       newmode = drm_mode_std(connector, edid, std);
> -                       if (newmode) {
> -                               drm_mode_probed_add(connector, newmode);
> -                               closure->modes++;
> -                       }
> +       for (i = 0; i < 6; i++) {
> +               struct std_timing *std = &data->data.timings[i];
> +               struct drm_display_mode *newmode;
> +
> +               newmode = drm_mode_std(connector, edid, std);
> +               if (newmode) {
> +                       drm_mode_probed_add(connector, newmode);
> +                       closure->modes++;
>                 }
>         }
>  }
> @@ -3056,10 +3070,11 @@ static void
>  do_cvt_mode(struct detailed_timing *timing, void *c)
>  {
>         struct detailed_mode_closure *closure = c;
> -       struct detailed_non_pixel *data = &timing->data.other_data;
>
> -       if (data->type == EDID_DETAIL_CVT_3BYTE)
> -               closure->modes += drm_cvt_modes(closure->connector, timing);
> +       if (!is_display_descriptor((const u8 *)timing, EDID_DETAIL_CVT_3BYTE))
> +               return;
> +
> +       closure->modes += drm_cvt_modes(closure->connector, timing);
>  }
>
>  static int
> @@ -4285,8 +4300,10 @@ drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db)
>  static void
>  monitor_name(struct detailed_timing *t, void *data)
>  {
> -       if (t->data.other_data.type == EDID_DETAIL_MONITOR_NAME)
> -               *(u8 **)data = t->data.other_data.data.str.str;
> +       if (!is_display_descriptor((const u8 *)t, EDID_DETAIL_MONITOR_NAME))
> +               return;
> +
> +       *(u8 **)data = t->data.other_data.data.str.str;
>  }
>
>  static int get_monitor_name(struct edid *edid, char name[13])
> --
> 2.24.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Shankar, Uma Feb. 3, 2020, 7:44 p.m. UTC | #2 
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Alex
> Deucher
> Sent: Tuesday, January 28, 2020 4:06 AM
> To: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Allen Chen <allen.chen@ite.com.tw>; Intel Graphics Development <intel-
> gfx@lists.freedesktop.org>; Maling list - DRI developers <dri-
> devel@lists.freedesktop.org>
> Subject: Re: [Intel-gfx] [PATCH 2/8] drm/edid: Don't accept any old garbage as a
> display descriptor
> 
> On Fri, Jan 24, 2020 at 3:02 PM Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> >
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Currently we assume any 18 byte descriptor to be a display descritor
> > if only the tag byte matches the expected value. But for detailed
> > timing descriptors that same byte is just the lower 8 bits of hblank,
> > and as such can match any display descriptor tag. To properly validate
> > that the 18 byte descriptor is in fact a display descriptor we must
> > also examine bytes 0-2 (just byte 1 should actually suffice but the
> > spec does say that bytes 0 and
> > 2 must also always be zero for display descriptors so we check those
> > too).
> >
> > Unlike Allen's original proposed patch to just fix is_rb() we roll
> > this out across the board to fix everything.
> >
> > Cc: Allen Chen <allen.chen@ite.com.tw>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Acked-by: Alex Deucher <alexander.deucher@amd.com>

Looks Good to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> > ---
> >  drivers/gpu/drm/drm_edid.c | 65
> > ++++++++++++++++++++++++--------------
> >  1 file changed, 41 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 1b6e544cf5c7..96ae1fde4ce2 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -2196,6 +2196,12 @@ struct drm_display_mode
> > *drm_mode_find_dmt(struct drm_device *dev,  }
> > EXPORT_SYMBOL(drm_mode_find_dmt);
> >
> > +static bool is_display_descriptor(const u8 d[18], u8 tag) {
> > +       return d[0] == 0x00 && d[1] == 0x00 &&
> > +               d[2] == 0x00 && d[3] == tag; }
> > +
> >  typedef void detailed_cb(struct detailed_timing *timing, void
> > *closure);
> >
> >  static void
> > @@ -2257,9 +2263,12 @@ static void
> >  is_rb(struct detailed_timing *t, void *data)  {
> >         u8 *r = (u8 *)t;
> > -       if (r[3] == EDID_DETAIL_MONITOR_RANGE)
> > -               if (r[15] & 0x10)
> > -                       *(bool *)data = true;
> > +
> > +       if (!is_display_descriptor(r, EDID_DETAIL_MONITOR_RANGE))
> > +               return;
> > +
> > +       if (r[15] & 0x10)
> > +               *(bool *)data = true;
> >  }
> >
> >  /* EDID 1.4 defines this explicitly.  For EDID 1.3, we guess, badly.
> > */ @@ -2279,7 +2288,11 @@ static void  find_gtf2(struct
> > detailed_timing *t, void *data)  {
> >         u8 *r = (u8 *)t;
> > -       if (r[3] == EDID_DETAIL_MONITOR_RANGE && r[10] == 0x02)
> > +
> > +       if (!is_display_descriptor(r, EDID_DETAIL_MONITOR_RANGE))
> > +               return;
> > +
> > +       if (r[10] == 0x02)
> >                 *(u8 **)data = r;
> >  }
> >
> > @@ -2818,7 +2831,7 @@ do_inferred_modes(struct detailed_timing *timing, void
> *c)
> >         struct detailed_non_pixel *data = &timing->data.other_data;
> >         struct detailed_data_monitor_range *range = &data->data.range;
> >
> > -       if (data->type != EDID_DETAIL_MONITOR_RANGE)
> > +       if (!is_display_descriptor((const u8 *)timing,
> > + EDID_DETAIL_MONITOR_RANGE))
> >                 return;
> >
> >         closure->modes += drm_dmt_modes_for_range(closure->connector,
> > @@ -2897,10 +2910,11 @@ static void
> >  do_established_modes(struct detailed_timing *timing, void *c)  {
> >         struct detailed_mode_closure *closure = c;
> > -       struct detailed_non_pixel *data = &timing->data.other_data;
> >
> > -       if (data->type == EDID_DETAIL_EST_TIMINGS)
> > -               closure->modes += drm_est3_modes(closure->connector, timing);
> > +       if (!is_display_descriptor((const u8 *)timing, EDID_DETAIL_EST_TIMINGS))
> > +               return;
> > +
> > +       closure->modes += drm_est3_modes(closure->connector, timing);
> >  }
> >
> >  /**
> > @@ -2949,19 +2963,19 @@ do_standard_modes(struct detailed_timing *timing,
> void *c)
> >         struct detailed_non_pixel *data = &timing->data.other_data;
> >         struct drm_connector *connector = closure->connector;
> >         struct edid *edid = closure->edid;
> > +       int i;
> >
> > -       if (data->type == EDID_DETAIL_STD_MODES) {
> > -               int i;
> > -               for (i = 0; i < 6; i++) {
> > -                       struct std_timing *std;
> > -                       struct drm_display_mode *newmode;
> > +       if (!is_display_descriptor((const u8 *)timing, EDID_DETAIL_STD_MODES))
> > +               return;
> >
> > -                       std = &data->data.timings[i];
> > -                       newmode = drm_mode_std(connector, edid, std);
> > -                       if (newmode) {
> > -                               drm_mode_probed_add(connector, newmode);
> > -                               closure->modes++;
> > -                       }
> > +       for (i = 0; i < 6; i++) {
> > +               struct std_timing *std = &data->data.timings[i];
> > +               struct drm_display_mode *newmode;
> > +
> > +               newmode = drm_mode_std(connector, edid, std);
> > +               if (newmode) {
> > +                       drm_mode_probed_add(connector, newmode);
> > +                       closure->modes++;
> >                 }
> >         }
> >  }
> > @@ -3056,10 +3070,11 @@ static void
> >  do_cvt_mode(struct detailed_timing *timing, void *c)  {
> >         struct detailed_mode_closure *closure = c;
> > -       struct detailed_non_pixel *data = &timing->data.other_data;
> >
> > -       if (data->type == EDID_DETAIL_CVT_3BYTE)
> > -               closure->modes += drm_cvt_modes(closure->connector, timing);
> > +       if (!is_display_descriptor((const u8 *)timing, EDID_DETAIL_CVT_3BYTE))
> > +               return;
> > +
> > +       closure->modes += drm_cvt_modes(closure->connector, timing);
> >  }
> >
> >  static int
> > @@ -4285,8 +4300,10 @@ drm_parse_hdmi_vsdb_audio(struct drm_connector
> > *connector, const u8 *db)  static void  monitor_name(struct
> > detailed_timing *t, void *data)  {
> > -       if (t->data.other_data.type == EDID_DETAIL_MONITOR_NAME)
> > -               *(u8 **)data = t->data.other_data.data.str.str;
> > +       if (!is_display_descriptor((const u8 *)t, EDID_DETAIL_MONITOR_NAME))
> > +               return;
> > +
> > +       *(u8 **)data = t->data.other_data.data.str.str;
> >  }
> >
> >  static int get_monitor_name(struct edid *edid, char name[13])
> > --
> > 2.24.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 1b6e544cf5c7..96ae1fde4ce2 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2196,6 +2196,12 @@  struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_mode_find_dmt);
 
+static bool is_display_descriptor(const u8 d[18], u8 tag)
+{
+	return d[0] == 0x00 && d[1] == 0x00 &&
+		d[2] == 0x00 && d[3] == tag;
+}
+
 typedef void detailed_cb(struct detailed_timing *timing, void *closure);
 
 static void
@@ -2257,9 +2263,12 @@  static void
 is_rb(struct detailed_timing *t, void *data)
 {
 	u8 *r = (u8 *)t;
-	if (r[3] == EDID_DETAIL_MONITOR_RANGE)
-		if (r[15] & 0x10)
-			*(bool *)data = true;
+
+	if (!is_display_descriptor(r, EDID_DETAIL_MONITOR_RANGE))
+		return;
+
+	if (r[15] & 0x10)
+		*(bool *)data = true;
 }
 
 /* EDID 1.4 defines this explicitly.  For EDID 1.3, we guess, badly. */
@@ -2279,7 +2288,11 @@  static void
 find_gtf2(struct detailed_timing *t, void *data)
 {
 	u8 *r = (u8 *)t;
-	if (r[3] == EDID_DETAIL_MONITOR_RANGE && r[10] == 0x02)
+
+	if (!is_display_descriptor(r, EDID_DETAIL_MONITOR_RANGE))
+		return;
+
+	if (r[10] == 0x02)
 		*(u8 **)data = r;
 }
 
@@ -2818,7 +2831,7 @@  do_inferred_modes(struct detailed_timing *timing, void *c)
 	struct detailed_non_pixel *data = &timing->data.other_data;
 	struct detailed_data_monitor_range *range = &data->data.range;
 
-	if (data->type != EDID_DETAIL_MONITOR_RANGE)
+	if (!is_display_descriptor((const u8 *)timing, EDID_DETAIL_MONITOR_RANGE))
 		return;
 
 	closure->modes += drm_dmt_modes_for_range(closure->connector,
@@ -2897,10 +2910,11 @@  static void
 do_established_modes(struct detailed_timing *timing, void *c)
 {
 	struct detailed_mode_closure *closure = c;
-	struct detailed_non_pixel *data = &timing->data.other_data;
 
-	if (data->type == EDID_DETAIL_EST_TIMINGS)
-		closure->modes += drm_est3_modes(closure->connector, timing);
+	if (!is_display_descriptor((const u8 *)timing, EDID_DETAIL_EST_TIMINGS))
+		return;
+
+	closure->modes += drm_est3_modes(closure->connector, timing);
 }
 
 /**
@@ -2949,19 +2963,19 @@  do_standard_modes(struct detailed_timing *timing, void *c)
 	struct detailed_non_pixel *data = &timing->data.other_data;
 	struct drm_connector *connector = closure->connector;
 	struct edid *edid = closure->edid;
+	int i;
 
-	if (data->type == EDID_DETAIL_STD_MODES) {
-		int i;
-		for (i = 0; i < 6; i++) {
-			struct std_timing *std;
-			struct drm_display_mode *newmode;
+	if (!is_display_descriptor((const u8 *)timing, EDID_DETAIL_STD_MODES))
+		return;
 
-			std = &data->data.timings[i];
-			newmode = drm_mode_std(connector, edid, std);
-			if (newmode) {
-				drm_mode_probed_add(connector, newmode);
-				closure->modes++;
-			}
+	for (i = 0; i < 6; i++) {
+		struct std_timing *std = &data->data.timings[i];
+		struct drm_display_mode *newmode;
+
+		newmode = drm_mode_std(connector, edid, std);
+		if (newmode) {
+			drm_mode_probed_add(connector, newmode);
+			closure->modes++;
 		}
 	}
 }
@@ -3056,10 +3070,11 @@  static void
 do_cvt_mode(struct detailed_timing *timing, void *c)
 {
 	struct detailed_mode_closure *closure = c;
-	struct detailed_non_pixel *data = &timing->data.other_data;
 
-	if (data->type == EDID_DETAIL_CVT_3BYTE)
-		closure->modes += drm_cvt_modes(closure->connector, timing);
+	if (!is_display_descriptor((const u8 *)timing, EDID_DETAIL_CVT_3BYTE))
+		return;
+
+	closure->modes += drm_cvt_modes(closure->connector, timing);
 }
 
 static int
@@ -4285,8 +4300,10 @@  drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db)
 static void
 monitor_name(struct detailed_timing *t, void *data)
 {
-	if (t->data.other_data.type == EDID_DETAIL_MONITOR_NAME)
-		*(u8 **)data = t->data.other_data.data.str.str;
+	if (!is_display_descriptor((const u8 *)t, EDID_DETAIL_MONITOR_NAME))
+		return;
+
+	*(u8 **)data = t->data.other_data.data.str.str;
 }
 
 static int get_monitor_name(struct edid *edid, char name[13])