| Message ID | 1452784327-27258-1-git-send-email-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 14, 2016 at 05:12:07PM +0200, Jani Nikula wrote: > Two errors in a single line. The size was read from the wrong offset, > and the end index didn't take the five bytes for sequence byte and size > of sequence into account. Fix it all, and break up the calculations a > bit to make it clearer. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Reported-by: Mika Kahola <mika.kahola@intel.com> > Fixes: 2a33d93486f2 ("drm/i915/bios: add support for MIPI sequence block v3") > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c > index 12e2f8b8bf9c..bf62a19c8f69 100644 > --- a/drivers/gpu/drm/i915/intel_bios.c > +++ b/drivers/gpu/drm/i915/intel_bios.c > @@ -842,6 +842,7 @@ static int goto_next_sequence_v3(const u8 *data, int index, int total) > { > int seq_end; > u16 len; > + u32 size_of_sequence; > > /* > * Could skip sequence based on Size of Sequence alone, but also do some > @@ -852,14 +853,24 @@ static int goto_next_sequence_v3(const u8 *data, int index, int total) > return 0; > } > > - seq_end = index + *((const u32 *)(data + 1)); > + /* Skip Sequence Byte. */ > + index++; > + > + /* > + * Size of Sequence. Excludes the Sequence Byte and the size itself, > + * includes MIPI_SEQ_ELEM_END byte, excludes the final MIPI_SEQ_END > + * byte. > + */ > + size_of_sequence = *((const uint32_t *)(data + index)); Hmm. So it was reading from 'data+1' and now it's basically 'data+index+1'. So it was correct for the first sequence, and busted for later ones I suppose. > + index += 4; > + > + seq_end = index + size_of_sequence; And now we count the size of the sequence starting from the operation byte, before we counted it from the sequence byte. "Fortunately" the spec doesn't even tell us which is correct. If it works, it works. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> BTW I was thinking that we could maybe add some kind of "read the thing at index, and and increment the index past it" helpers. Eg. int get_u8(const void *data, int index, int size, u8 *ret); int get_u32(const void *data, int index, int size, u32 *ret); they could also do the index vs. size check and just return an error if we try to go too far. > if (seq_end > total) { > DRM_ERROR("Invalid sequence size\n"); > return 0; > } > > - /* Skip Sequence Byte and Size of Sequence. */ > - for (index = index + 5; index < total; index += len) { > + for (; index < total; index += len) { > u8 operation_byte = *(data + index); > index++; > > -- > 2.1.4
On Thu, 2016-01-14 at 17:12 +0200, Jani Nikula wrote: > Two errors in a single line. The size was read from the wrong offset, > and the end index didn't take the five bytes for sequence byte and size > of sequence into account. Fix it all, and break up the calculations a > bit to make it clearer. > Tested-by: Mika Kahola <mika.kahola@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Reported-by: Mika Kahola <mika.kahola@intel.com> > Fixes: 2a33d93486f2 ("drm/i915/bios: add support for MIPI sequence block v3") > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c > index 12e2f8b8bf9c..bf62a19c8f69 100644 > --- a/drivers/gpu/drm/i915/intel_bios.c > +++ b/drivers/gpu/drm/i915/intel_bios.c > @@ -842,6 +842,7 @@ static int goto_next_sequence_v3(const u8 *data, int index, int total) > { > int seq_end; > u16 len; > + u32 size_of_sequence; > > /* > * Could skip sequence based on Size of Sequence alone, but also do some > @@ -852,14 +853,24 @@ static int goto_next_sequence_v3(const u8 *data, int index, int total) > return 0; > } > > - seq_end = index + *((const u32 *)(data + 1)); > + /* Skip Sequence Byte. */ > + index++; > + > + /* > + * Size of Sequence. Excludes the Sequence Byte and the size itself, > + * includes MIPI_SEQ_ELEM_END byte, excludes the final MIPI_SEQ_END > + * byte. > + */ > + size_of_sequence = *((const uint32_t *)(data + index)); > + index += 4; > + > + seq_end = index + size_of_sequence; > if (seq_end > total) { > DRM_ERROR("Invalid sequence size\n"); > return 0; > } > > - /* Skip Sequence Byte and Size of Sequence. */ > - for (index = index + 5; index < total; index += len) { > + for (; index < total; index += len) { > u8 operation_byte = *(data + index); > index++; >
On Thu, 14 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Jan 14, 2016 at 05:12:07PM +0200, Jani Nikula wrote: >> Two errors in a single line. The size was read from the wrong offset, >> and the end index didn't take the five bytes for sequence byte and size >> of sequence into account. Fix it all, and break up the calculations a >> bit to make it clearer. >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Reported-by: Mika Kahola <mika.kahola@intel.com> >> Fixes: 2a33d93486f2 ("drm/i915/bios: add support for MIPI sequence block v3") >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++--- >> 1 file changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c >> index 12e2f8b8bf9c..bf62a19c8f69 100644 >> --- a/drivers/gpu/drm/i915/intel_bios.c >> +++ b/drivers/gpu/drm/i915/intel_bios.c >> @@ -842,6 +842,7 @@ static int goto_next_sequence_v3(const u8 *data, int index, int total) >> { >> int seq_end; >> u16 len; >> + u32 size_of_sequence; >> >> /* >> * Could skip sequence based on Size of Sequence alone, but also do some >> @@ -852,14 +853,24 @@ static int goto_next_sequence_v3(const u8 *data, int index, int total) >> return 0; >> } >> >> - seq_end = index + *((const u32 *)(data + 1)); >> + /* Skip Sequence Byte. */ >> + index++; >> + >> + /* >> + * Size of Sequence. Excludes the Sequence Byte and the size itself, >> + * includes MIPI_SEQ_ELEM_END byte, excludes the final MIPI_SEQ_END >> + * byte. >> + */ >> + size_of_sequence = *((const uint32_t *)(data + index)); > > Hmm. So it was reading from 'data+1' and now it's basically 'data+index+1'. > So it was correct for the first sequence, and busted for later ones I > suppose. > >> + index += 4; >> + >> + seq_end = index + size_of_sequence; > > And now we count the size of the sequence starting from the operation > byte, before we counted it from the sequence byte. "Fortunately" the spec > doesn't even tell us which is correct. If it works, it works. > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Pushed to drm-intel-next-queued, thanks for the review and testing. BR, Jani. > > BTW I was thinking that we could maybe add some kind of > "read the thing at index, and and increment the index past it" helpers. > > Eg. > int get_u8(const void *data, int index, int size, u8 *ret); > int get_u32(const void *data, int index, int size, u32 *ret); > > they could also do the index vs. size check and just return an error if > we try to go too far. > >> if (seq_end > total) { >> DRM_ERROR("Invalid sequence size\n"); >> return 0; >> } >> >> - /* Skip Sequence Byte and Size of Sequence. */ >> - for (index = index + 5; index < total; index += len) { >> + for (; index < total; index += len) { >> u8 operation_byte = *(data + index); >> index++; >> >> -- >> 2.1.4
On Fri, Jan 15, 2016 at 11:51:31AM +0200, Jani Nikula wrote: > On Thu, 14 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Thu, Jan 14, 2016 at 05:12:07PM +0200, Jani Nikula wrote: > >> Two errors in a single line. The size was read from the wrong offset, > >> and the end index didn't take the five bytes for sequence byte and size > >> of sequence into account. Fix it all, and break up the calculations a > >> bit to make it clearer. > >> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Reported-by: Mika Kahola <mika.kahola@intel.com> > >> Fixes: 2a33d93486f2 ("drm/i915/bios: add support for MIPI sequence block v3") > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++--- > >> 1 file changed, 14 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c > >> index 12e2f8b8bf9c..bf62a19c8f69 100644 > >> --- a/drivers/gpu/drm/i915/intel_bios.c > >> +++ b/drivers/gpu/drm/i915/intel_bios.c > >> @@ -842,6 +842,7 @@ static int goto_next_sequence_v3(const u8 *data, int index, int total) > >> { > >> int seq_end; > >> u16 len; > >> + u32 size_of_sequence; > >> > >> /* > >> * Could skip sequence based on Size of Sequence alone, but also do some > >> @@ -852,14 +853,24 @@ static int goto_next_sequence_v3(const u8 *data, int index, int total) > >> return 0; > >> } > >> > >> - seq_end = index + *((const u32 *)(data + 1)); > >> + /* Skip Sequence Byte. */ > >> + index++; > >> + > >> + /* > >> + * Size of Sequence. Excludes the Sequence Byte and the size itself, > >> + * includes MIPI_SEQ_ELEM_END byte, excludes the final MIPI_SEQ_END > >> + * byte. > >> + */ > >> + size_of_sequence = *((const uint32_t *)(data + index)); > > > > Hmm. So it was reading from 'data+1' and now it's basically 'data+index+1'. > > So it was correct for the first sequence, and busted for later ones I > > suppose. > > > >> + index += 4; > >> + > >> + seq_end = index + size_of_sequence; > > > > And now we count the size of the sequence starting from the operation > > byte, before we counted it from the sequence byte. "Fortunately" the spec > > doesn't even tell us which is correct. If it works, it works. > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Pushed to drm-intel-next-queued, thanks for the review and testing. You failed bat CI. Please make that the failure really is pre-existing and if so dig out the bugzilla for it. If that's not the case please revert. I'll paste you the link to the internal wiki in private. -Daniel
On Tue, 19 Jan 2016, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Jan 15, 2016 at 11:51:31AM +0200, Jani Nikula wrote: >> On Thu, 14 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> > On Thu, Jan 14, 2016 at 05:12:07PM +0200, Jani Nikula wrote: >> >> Two errors in a single line. The size was read from the wrong offset, >> >> and the end index didn't take the five bytes for sequence byte and size >> >> of sequence into account. Fix it all, and break up the calculations a >> >> bit to make it clearer. >> >> >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> Reported-by: Mika Kahola <mika.kahola@intel.com> >> >> Fixes: 2a33d93486f2 ("drm/i915/bios: add support for MIPI sequence block v3") >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> >> --- >> >> drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++--- >> >> 1 file changed, 14 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c >> >> index 12e2f8b8bf9c..bf62a19c8f69 100644 >> >> --- a/drivers/gpu/drm/i915/intel_bios.c >> >> +++ b/drivers/gpu/drm/i915/intel_bios.c >> >> @@ -842,6 +842,7 @@ static int goto_next_sequence_v3(const u8 *data, int index, int total) >> >> { >> >> int seq_end; >> >> u16 len; >> >> + u32 size_of_sequence; >> >> >> >> /* >> >> * Could skip sequence based on Size of Sequence alone, but also do some >> >> @@ -852,14 +853,24 @@ static int goto_next_sequence_v3(const u8 *data, int index, int total) >> >> return 0; >> >> } >> >> >> >> - seq_end = index + *((const u32 *)(data + 1)); >> >> + /* Skip Sequence Byte. */ >> >> + index++; >> >> + >> >> + /* >> >> + * Size of Sequence. Excludes the Sequence Byte and the size itself, >> >> + * includes MIPI_SEQ_ELEM_END byte, excludes the final MIPI_SEQ_END >> >> + * byte. >> >> + */ >> >> + size_of_sequence = *((const uint32_t *)(data + index)); >> > >> > Hmm. So it was reading from 'data+1' and now it's basically 'data+index+1'. >> > So it was correct for the first sequence, and busted for later ones I >> > suppose. >> > >> >> + index += 4; >> >> + >> >> + seq_end = index + size_of_sequence; >> > >> > And now we count the size of the sequence starting from the operation >> > byte, before we counted it from the sequence byte. "Fortunately" the spec >> > doesn't even tell us which is correct. If it works, it works. >> > >> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> Pushed to drm-intel-next-queued, thanks for the review and testing. > > You failed bat CI. Please make that the failure really is pre-existing and > if so dig out the bugzilla for it. If that's not the case please revert. Hey, can't apply new rules after the fact. Seriously. I looked at the results before pushing and observed they were bogus wrt this patch. The changed code shouldn't be run on any of the CI machines, and even if it were run (e.g. due to bogus BIOS), none of the CI machines have DSI displays where the change would matter (a CI fail of a bigger scale). I was about as worried for the test results change as I would have been for a pure comment update. BR, Jani.
On Tue, Jan 19, 2016 at 07:55:58PM +0200, Jani Nikula wrote: > On Tue, 19 Jan 2016, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Fri, Jan 15, 2016 at 11:51:31AM +0200, Jani Nikula wrote: > >> On Thu, 14 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > >> > On Thu, Jan 14, 2016 at 05:12:07PM +0200, Jani Nikula wrote: > >> >> Two errors in a single line. The size was read from the wrong offset, > >> >> and the end index didn't take the five bytes for sequence byte and size > >> >> of sequence into account. Fix it all, and break up the calculations a > >> >> bit to make it clearer. > >> >> > >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> >> Reported-by: Mika Kahola <mika.kahola@intel.com> > >> >> Fixes: 2a33d93486f2 ("drm/i915/bios: add support for MIPI sequence block v3") > >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> >> --- > >> >> drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++--- > >> >> 1 file changed, 14 insertions(+), 3 deletions(-) > >> >> > >> >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c > >> >> index 12e2f8b8bf9c..bf62a19c8f69 100644 > >> >> --- a/drivers/gpu/drm/i915/intel_bios.c > >> >> +++ b/drivers/gpu/drm/i915/intel_bios.c > >> >> @@ -842,6 +842,7 @@ static int goto_next_sequence_v3(const u8 *data, int index, int total) > >> >> { > >> >> int seq_end; > >> >> u16 len; > >> >> + u32 size_of_sequence; > >> >> > >> >> /* > >> >> * Could skip sequence based on Size of Sequence alone, but also do some > >> >> @@ -852,14 +853,24 @@ static int goto_next_sequence_v3(const u8 *data, int index, int total) > >> >> return 0; > >> >> } > >> >> > >> >> - seq_end = index + *((const u32 *)(data + 1)); > >> >> + /* Skip Sequence Byte. */ > >> >> + index++; > >> >> + > >> >> + /* > >> >> + * Size of Sequence. Excludes the Sequence Byte and the size itself, > >> >> + * includes MIPI_SEQ_ELEM_END byte, excludes the final MIPI_SEQ_END > >> >> + * byte. > >> >> + */ > >> >> + size_of_sequence = *((const uint32_t *)(data + index)); > >> > > >> > Hmm. So it was reading from 'data+1' and now it's basically 'data+index+1'. > >> > So it was correct for the first sequence, and busted for later ones I > >> > suppose. > >> > > >> >> + index += 4; > >> >> + > >> >> + seq_end = index + size_of_sequence; > >> > > >> > And now we count the size of the sequence starting from the operation > >> > byte, before we counted it from the sequence byte. "Fortunately" the spec > >> > doesn't even tell us which is correct. If it works, it works. > >> > > >> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> > >> Pushed to drm-intel-next-queued, thanks for the review and testing. > > > > You failed bat CI. Please make that the failure really is pre-existing and > > if so dig out the bugzilla for it. If that's not the case please revert. > > Hey, can't apply new rules after the fact. Seriously. > > I looked at the results before pushing and observed they were bogus wrt > this patch. The changed code shouldn't be run on any of the CI machines, > and even if it were run (e.g. due to bogus BIOS), none of the CI > machines have DSI displays where the change would matter (a CI fail of a > bigger scale). > > I was about as worried for the test results change as I would have been > for a pure comment update. It's not just about your patch, but to make sure we do have all the random noise tracked somewhere. Because there's going to be another person who'll run into this, and for him/her this will again be random noise no one seems to care about. This specific bug is tracked already in https://bugs.freedesktop.org/show_bug.cgi?id=93699 And yes I did check the backtraces to make sure it's indeed a match. If you want to object to this please raise it in Jesse's meeting this week. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index 12e2f8b8bf9c..bf62a19c8f69 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -842,6 +842,7 @@ static int goto_next_sequence_v3(const u8 *data, int index, int total) { int seq_end; u16 len; + u32 size_of_sequence; /* * Could skip sequence based on Size of Sequence alone, but also do some @@ -852,14 +853,24 @@ static int goto_next_sequence_v3(const u8 *data, int index, int total) return 0; } - seq_end = index + *((const u32 *)(data + 1)); + /* Skip Sequence Byte. */ + index++; + + /* + * Size of Sequence. Excludes the Sequence Byte and the size itself, + * includes MIPI_SEQ_ELEM_END byte, excludes the final MIPI_SEQ_END + * byte. + */ + size_of_sequence = *((const uint32_t *)(data + index)); + index += 4; + + seq_end = index + size_of_sequence; if (seq_end > total) { DRM_ERROR("Invalid sequence size\n"); return 0; } - /* Skip Sequence Byte and Size of Sequence. */ - for (index = index + 5; index < total; index += len) { + for (; index < total; index += len) { u8 operation_byte = *(data + index); index++;
Two errors in a single line. The size was read from the wrong offset, and the end index didn't take the five bytes for sequence byte and size of sequence into account. Fix it all, and break up the calculations a bit to make it clearer. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Reported-by: Mika Kahola <mika.kahola@intel.com> Fixes: 2a33d93486f2 ("drm/i915/bios: add support for MIPI sequence block v3") Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)