After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 762856 - It is possible hit Buffer Overflow - Array Index Out of Bounds in fill_iq_matrix_8x8 for H264
It is possible hit Buffer Overflow - Array Index Out of Bounds in fill_iq_mat...
Status: RESOLVED INVALID
Product: GStreamer
Classification: Platform
Component: gstreamer-vaapi
git master
Other Linux
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-02-29 09:03 UTC by Lim Siew Hoon
Modified: 2016-02-29 10:26 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Lim Siew Hoon 2016-02-29 09:03:38 UTC
I still got 1 KW issue that I'm not able to confirm is it possible an issue.  I'm look on avdec_h264 side how they handle it and also check thru libva and Intel VA driver side.
It look like really an issue will pretty sure will hit the buffer overflow - Array Index out of bounds n=6 and chroma_format_idc = 3, when the for loop i= 2...5. 
Please advise will it be an issue?


From libva
/** H.264 Inverse Quantization Matrix Buffer */
typedef struct _VAIQMatrixBufferH264	
{		
    /** \brief 4x4 scaling list, in raster scan order. */	
    unsigned char ScalingList4x4[6][16];	
    /** \brief 8x8 scaling list, in raster scan order. */	
    unsigned char ScalingList8x8[2][64];	
} VAIQMatrixBufferH264;

From gsth264parser.h
struct _GstH264PPS
{
  guint8 transform_8x8_mode_flag;		
  guint8 scaling_lists_4x4[6][16];	
  guint8 scaling_lists_8x8[6][64];	
...
};


static void
fill_iq_matrix_8x8 (VAIQMatrixBufferH264 * iq_matrix, const GstH264PPS * pps,
    const GstH264SPS * sps)
{
  guint i, n;

  /* If chroma_format_idc != 3, there are up to 2 8x8 scaling lists */
  if (!pps->transform_8x8_mode_flag)
    return;

  g_assert (G_N_ELEMENTS (iq_matrix->ScalingList8x8) >= 2);
  g_assert (G_N_ELEMENTS (iq_matrix->ScalingList8x8[0]) == 64);

  n = (sps->chroma_format_idc != 3) ? 2 : 6;
  for (i = 0; i < n; i++) {
    gst_h264_quant_matrix_8x8_get_raster_from_zigzag (iq_matrix->ScalingList8x8
        [i], pps->scaling_lists_8x8[i]);
  }
}
Comment 1 sreerenj 2016-02-29 09:44:06 UTC
We never hit this code,   See the code before invoking fill_iq_matrix_8x8()

/* XXX: we can only support 4:2:0 or 4:2:2 since ScalingLists8x8[]
     is not large enough to hold lists for 4:4:4 */
  if (sps->chroma_format_idc == 3)
    return GST_VAAPI_DECODER_STATUS_ERROR_UNSUPPORTED_CHROMA_FORMAT;

Parse is supposed to parse whatever possible, which can be up to 6 scaling lists for 8x8, but libva only support two.
Comment 2 Sebastian Dröge (slomo) 2016-02-29 10:14:31 UTC
So all good here and just a comment has to be added to the code?
Comment 3 Lim Siew Hoon 2016-02-29 10:20:22 UTC
(In reply to sreerenj from comment #1)
> We never hit this code,   See the code before invoking fill_iq_matrix_8x8()
> 
> /* XXX: we can only support 4:2:0 or 4:2:2 since ScalingLists8x8[]
>      is not large enough to hold lists for 4:4:4 */
>   if (sps->chroma_format_idc == 3)
>     return GST_VAAPI_DECODER_STATUS_ERROR_UNSUPPORTED_CHROMA_FORMAT;
> 
> Parse is supposed to parse whatever possible, which can be up to 6 scaling
> lists for 8x8, but libva only support two.

Yes, you are right. The checking is done in ensure_quant_matrix function before the fill_iq_matrix_8x8 function get call. I missed out that one. Thank you for feedback. You may closed the issue now. Thank you.