* [bug report] drm/i915/bios: add support for MIPI sequence block v3
@ 2018-06-08 12:36 Dan Carpenter
2018-06-08 12:50 ` Jani Nikula
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2018-06-08 12:36 UTC (permalink / raw)
To: jani.nikula; +Cc: intel-gfx
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) {
915 u8 operation_byte = *(data + index);
916 index++;
^^^^^^^
917
918 if (operation_byte == MIPI_SEQ_ELEM_END) {
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++;
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] drm/i915/bios: add support for MIPI sequence block v3
2018-06-08 12:36 [bug report] drm/i915/bios: add support for MIPI sequence block v3 Dan Carpenter
@ 2018-06-08 12:50 ` Jani Nikula
2018-06-08 13:30 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Jani Nikula @ 2018-06-08 12:50 UTC (permalink / raw)
To: Dan Carpenter; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] drm/i915/bios: add support for MIPI sequence block v3
2018-06-08 12:50 ` Jani Nikula
@ 2018-06-08 13:30 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2018-06-08 13:30 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
On Fri, Jun 08, 2018 at 03:50:47PM +0300, Jani Nikula wrote:
> 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...
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-06-08 13:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-08 12:36 [bug report] drm/i915/bios: add support for MIPI sequence block v3 Dan Carpenter
2018-06-08 12:50 ` Jani Nikula
2018-06-08 13:30 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.