On Fri, Jun 08, 2018 at 03:50:47PM +0300, Jani Nikula wrote:
> On Fri, 08 Jun 2018, Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
