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

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 08 Jun 2018, Dan Carpenter <dan.carpenter@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...

BR,
Jani.

>    928  
>    929                  /*
>    930                   * FIXME: Would be nice to check elements like for v1/v2 in
>    931                   * goto_next_sequence() above.
>    932                   */
>    933                  switch (operation_byte) {
>    934                  case MIPI_SEQ_ELEM_SEND_PKT:
>    935                  case MIPI_SEQ_ELEM_DELAY:
>    936                  case MIPI_SEQ_ELEM_GPIO:
>    937                  case MIPI_SEQ_ELEM_I2C:
>    938                  case MIPI_SEQ_ELEM_SPI:
>    939                  case MIPI_SEQ_ELEM_PMIC:
>    940                          break;
>    941                  default:
>
> regards,
> dan carpenter

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx