diff mbox series

[1/8] drm/edid: Check the number of detailed timing descriptors in the CEA ext block

Message ID 20200124200231.10517-1-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 Syrjala Jan. 24, 2020, 8:02 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

CEA-861 says :
"d = offset for the byte following the reserved data block.
 If no data is provided in the reserved data block, then d=4.
 If no DTDs are provided, then d=0."

So let's not look for DTDs when d==0. In fact let's just make that
<4 since those values would just mean that he DTDs overlap the block
header. And let's also check that d isn't so big as to declare
the descriptors to live past the block end, although the code
does already survive that case as we'd just end up with a negative
number of descriptors and the loop would not do anything.

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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Alex Deucher Jan. 27, 2020, 10:34 p.m. UTC | #1 
On Fri, Jan 24, 2020 at 3:03 PM Ville Syrjala
<ville.syrjala@linux.intel.com> wrote:
>
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> CEA-861 says :
> "d = offset for the byte following the reserved data block.
>  If no data is provided in the reserved data block, then d=4.
>  If no DTDs are provided, then d=0."
>
> So let's not look for DTDs when d==0. In fact let's just make that
> <4 since those values would just mean that he DTDs overlap the block
> header. And let's also check that d isn't so big as to declare
> the descriptors to live past the block end, although the code
> does already survive that case as we'd just end up with a negative
> number of descriptors and the loop would not do anything.
>
> 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 | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 99769d6c9f84..1b6e544cf5c7 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2201,10 +2201,13 @@ typedef void detailed_cb(struct detailed_timing *timing, void *closure);
>  static void
>  cea_for_each_detailed_block(u8 *ext, detailed_cb *cb, void *closure)
>  {
> -       int i, n = 0;
> +       int i, n;
>         u8 d = ext[0x02];
>         u8 *det_base = ext + d;
>
> +       if (d < 4 || d > 127)
> +               return;
> +
>         n = (127 - d) / 18;
>         for (i = 0; i < n; i++)
>                 cb((struct detailed_timing *)(det_base + 18 * i), closure);
> --
> 2.24.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter Jan. 28, 2020, 3:17 p.m. UTC | #2 
On Fri, Jan 24, 2020 at 10:02:24PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> CEA-861 says :
> "d = offset for the byte following the reserved data block.
>  If no data is provided in the reserved data block, then d=4.
>  If no DTDs are provided, then d=0."
> 
> So let's not look for DTDs when d==0. In fact let's just make that
> <4 since those values would just mean that he DTDs overlap the block
> header. And let's also check that d isn't so big as to declare
> the descriptors to live past the block end, although the code
> does already survive that case as we'd just end up with a negative
> number of descriptors and the loop would not do anything.
> 
> Cc: Allen Chen <allen.chen@ite.com.tw>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Hm I think edid parsing is like the perfect use-case for some in-kernel
unit tests ... In case anyone feels motivated?
-Daniel

> ---
>  drivers/gpu/drm/drm_edid.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 99769d6c9f84..1b6e544cf5c7 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2201,10 +2201,13 @@ typedef void detailed_cb(struct detailed_timing *timing, void *closure);
>  static void
>  cea_for_each_detailed_block(u8 *ext, detailed_cb *cb, void *closure)
>  {
> -	int i, n = 0;
> +	int i, n;
>  	u8 d = ext[0x02];
>  	u8 *det_base = ext + d;
>  
> +	if (d < 4 || d > 127)
> +		return;
> +
>  	n = (127 - d) / 18;
>  	for (i = 0; i < n; i++)
>  		cb((struct detailed_timing *)(det_base + 18 * i), closure);
> -- 
> 2.24.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Ville Syrjala Jan. 28, 2020, 4:15 p.m. UTC | #3 
On Tue, Jan 28, 2020 at 04:17:58PM +0100, Daniel Vetter wrote:
> On Fri, Jan 24, 2020 at 10:02:24PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > CEA-861 says :
> > "d = offset for the byte following the reserved data block.
> >  If no data is provided in the reserved data block, then d=4.
> >  If no DTDs are provided, then d=0."
> > 
> > So let's not look for DTDs when d==0. In fact let's just make that
> > <4 since those values would just mean that he DTDs overlap the block
> > header. And let's also check that d isn't so big as to declare
> > the descriptors to live past the block end, although the code
> > does already survive that case as we'd just end up with a negative
> > number of descriptors and the loop would not do anything.
> > 
> > Cc: Allen Chen <allen.chen@ite.com.tw>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Hm I think edid parsing is like the perfect use-case for some in-kernel
> unit tests ... In case anyone feels motivated?

Another idea I've been putting off is simply shoving the parser into
userspace. Kinda looks like with fork_usermode_blob() we could embed
the executable into the kernel/module and thus avoid the problem of
actually shipping the binary somehow.
Daniel Vetter Jan. 28, 2020, 4:18 p.m. UTC | #4 
On Tue, Jan 28, 2020 at 5:15 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Tue, Jan 28, 2020 at 04:17:58PM +0100, Daniel Vetter wrote:
> > On Fri, Jan 24, 2020 at 10:02:24PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > CEA-861 says :
> > > "d = offset for the byte following the reserved data block.
> > >  If no data is provided in the reserved data block, then d=4.
> > >  If no DTDs are provided, then d=0."
> > >
> > > So let's not look for DTDs when d==0. In fact let's just make that
> > > <4 since those values would just mean that he DTDs overlap the block
> > > header. And let's also check that d isn't so big as to declare
> > > the descriptors to live past the block end, although the code
> > > does already survive that case as we'd just end up with a negative
> > > number of descriptors and the loop would not do anything.
> > >
> > > Cc: Allen Chen <allen.chen@ite.com.tw>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Hm I think edid parsing is like the perfect use-case for some in-kernel
> > unit tests ... In case anyone feels motivated?
>
> Another idea I've been putting off is simply shoving the parser into
> userspace. Kinda looks like with fork_usermode_blob() we could embed
> the executable into the kernel/module and thus avoid the problem of
> actually shipping the binary somehow.

"How to run unit tests without losing hair" is essentially what kunit
tries to solve. I think we should cut over to that (it's merged now,
should be good enough for at least the edid parser, mocking stuff
isn't there there), and then make sure CI runs that stuff for us. Then
we could convert over at least the unit test like selftests eventually
too.
-Daniel
Ville Syrjala Jan. 28, 2020, 4:28 p.m. UTC | #5 
On Tue, Jan 28, 2020 at 05:18:34PM +0100, Daniel Vetter wrote:
> On Tue, Jan 28, 2020 at 5:15 PM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Tue, Jan 28, 2020 at 04:17:58PM +0100, Daniel Vetter wrote:
> > > On Fri, Jan 24, 2020 at 10:02:24PM +0200, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > CEA-861 says :
> > > > "d = offset for the byte following the reserved data block.
> > > >  If no data is provided in the reserved data block, then d=4.
> > > >  If no DTDs are provided, then d=0."
> > > >
> > > > So let's not look for DTDs when d==0. In fact let's just make that
> > > > <4 since those values would just mean that he DTDs overlap the block
> > > > header. And let's also check that d isn't so big as to declare
> > > > the descriptors to live past the block end, although the code
> > > > does already survive that case as we'd just end up with a negative
> > > > number of descriptors and the loop would not do anything.
> > > >
> > > > Cc: Allen Chen <allen.chen@ite.com.tw>
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Hm I think edid parsing is like the perfect use-case for some in-kernel
> > > unit tests ... In case anyone feels motivated?
> >
> > Another idea I've been putting off is simply shoving the parser into
> > userspace. Kinda looks like with fork_usermode_blob() we could embed
> > the executable into the kernel/module and thus avoid the problem of
> > actually shipping the binary somehow.
> 
> "How to run unit tests without losing hair" is essentially what kunit
> tries to solve. I think we should cut over to that (it's merged now,
> should be good enough for at least the edid parser, mocking stuff
> isn't there there), and then make sure CI runs that stuff for us. Then
> we could convert over at least the unit test like selftests eventually
> too.

I meant run it in userspace *always*, not just for testing.
Shankar, Uma Feb. 3, 2020, 7:15 p.m. UTC | #6 
> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Alex
> Deucher
> Sent: Tuesday, January 28, 2020 4:04 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: [PATCH 1/8] drm/edid: Check the number of detailed timing descriptors
> in the CEA ext block
> 
> On Fri, Jan 24, 2020 at 3:03 PM Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> >
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > CEA-861 says :
> > "d = offset for the byte following the reserved data block.
> >  If no data is provided in the reserved data block, then d=4.
> >  If no DTDs are provided, then d=0."
> >
> > So let's not look for DTDs when d==0. In fact let's just make that
> > <4 since those values would just mean that he DTDs overlap the block
> > header. And let's also check that d isn't so big as to declare the
> > descriptors to live past the block end, although the code does already
> > survive that case as we'd just end up with a negative number of
> > descriptors and the loop would not do anything.
> >
> > 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 as well.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> > ---
> >  drivers/gpu/drm/drm_edid.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 99769d6c9f84..1b6e544cf5c7 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -2201,10 +2201,13 @@ typedef void detailed_cb(struct
> > detailed_timing *timing, void *closure);  static void
> >  cea_for_each_detailed_block(u8 *ext, detailed_cb *cb, void *closure)
> > {
> > -       int i, n = 0;
> > +       int i, n;
> >         u8 d = ext[0x02];
> >         u8 *det_base = ext + d;
> >
> > +       if (d < 4 || d > 127)
> > +               return;
> > +
> >         n = (127 - d) / 18;
> >         for (i = 0; i < n; i++)
> >                 cb((struct detailed_timing *)(det_base + 18 * i),
> > closure);
> > --
> > 2.24.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 99769d6c9f84..1b6e544cf5c7 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2201,10 +2201,13 @@  typedef void detailed_cb(struct detailed_timing *timing, void *closure);
 static void
 cea_for_each_detailed_block(u8 *ext, detailed_cb *cb, void *closure)
 {
-	int i, n = 0;
+	int i, n;
 	u8 d = ext[0x02];
 	u8 *det_base = ext + d;
 
+	if (d < 4 || d > 127)
+		return;
+
 	n = (127 - d) / 18;
 	for (i = 0; i < n; i++)
 		cb((struct detailed_timing *)(det_base + 18 * i), closure);