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 |
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
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
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.
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
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.
> -----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 --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);