[Intel-gfx] [bug report] drm/i915/bios: add support for MIPI sequence block v3

[Dan Carpenter]

On Fri, Jun 08, 2018 at 03:50:47PM +0300, Jani Nikula wrote:
> On Fri, 08 Jun 2018, Dan Carpenter <dan.carpenter at oracle.com> wrote:
> > Hello Jani Nikula,
> >
> > The patch 2a33d93486f2: "drm/i915/bios: add support for MIPI sequence
> > block v3" from Jan 11, 2016, leads to the following static checker
> > warning:
> >
> > 	drivers/gpu/drm/i915/intel_bios.c:926 goto_next_sequence_v3()
> > 	warn: potentially one past the end of array 'data[index]'
> >
> > drivers/gpu/drm/i915/intel_bios.c
> >    897          /* Skip Sequence Byte. */
> >    898          index++;
> >    899  
> >    900          /*
> >    901           * Size of Sequence. Excludes the Sequence Byte and the size itself,
> >    902           * includes MIPI_SEQ_ELEM_END byte, excludes the final MIPI_SEQ_END
> >    903           * byte.
> >    904           */
> >    905          size_of_sequence = *((const uint32_t *)(data + index));
> >    906          index += 4;
> >    907  
> >    908          seq_end = index + size_of_sequence;
> >    909          if (seq_end > total) {
> >    910                  DRM_ERROR("Invalid sequence size\n");
> >    911                  return 0;
> >    912          }
> >    913  
> >    914          for (; index < total; index += len) {
> 
> The data being parsed here is a sort of TLV coded blob with len here
> referring to the payload length.
> 
> It's a sort of TLV coded blob with len here referring to the payload
> length. T being the 1-byte operation_byte, L being the 1-byte len.
> 
> 
> 
> 
> >    915                  u8 operation_byte = *(data + index);
> 
> index is now at T, or operation byte.
> 
> >    916                  index++;
> >                         ^^^^^^^
> 
> index is now at L, or length.
> 
> >    917  
> >    918                  if (operation_byte == MIPI_SEQ_ELEM_END) {
> 
> it could also be a marker for end of the whole thing, in which case the
> operation_byte is 0.
> 
> >    919                          if (index != seq_end) {
> >    920                                  DRM_ERROR("Invalid element structure\n");
> >    921                                  return 0;
> >    922                          }
> >    923                          return index;
> >    924                  }
> >    925  
> >    926                  len = *(data + index);
> >                         ^^^^^^^^^^^^^^^^^^^^^
> > This does look to uninitiated eyes as if it might be one past the end?
> >
> >    927                  index++;
> 
> index is now at the payload, which is len bytes.
> 
> Makes sense? N.b. I didn't specify the format...

Yeah.  That makes sense.  It's sort of a common idiom too...  I haven't
figured out a way to deal with it from a static analysis perspective...

Anyway, thanks for taking a look.

regards,
dan carpenter