Re: [bug report] [media] vcodec: mediatek: Add Mediatek V4L2 Video Decoder Driver

[Dan Carpenter <dan.carpenter@oracle.com>]

On Wed, Nov 09, 2016 at 02:45:21PM +0100, Hans Verkuil wrote:
> On 11/09/16 14:28, Dan Carpenter wrote:
> >Hello Tiffany Lin,
> >
> >The patch 590577a4e525: "[media] vcodec: mediatek: Add Mediatek V4L2
> >Video Decoder Driver" from Sep 2, 2016, leads to the following static
> >checker warning:
> >
> >	drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c:536 vidioc_vdec_qbuf()
> >	error: buffer overflow 'vq->bufs' 32 <= u32max
> >
> >drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> >   520  static int vidioc_vdec_qbuf(struct file *file, void *priv,
> >   521                              struct v4l2_buffer *buf)
> >   522  {
> >   523          struct mtk_vcodec_ctx *ctx = fh_to_ctx(priv);
> >   524          struct vb2_queue *vq;
> >   525          struct vb2_buffer *vb;
> >   526          struct mtk_video_dec_buf *mtkbuf;
> >   527          struct vb2_v4l2_buffer  *vb2_v4l2;
> >   528
> >   529          if (ctx->state == MTK_STATE_ABORT) {
> >   530                  mtk_v4l2_err("[%d] Call on QBUF after unrecoverable error",
> >   531                                  ctx->id);
> >   532                  return -EIO;
> >   533          }
> >   534
> >   535          vq = v4l2_m2m_get_vq(ctx->m2m_ctx, buf->type);
> >   536          vb = vq->bufs[buf->index];
> >
> >Smatch thinks that "buf->index" comes straight from the user without
> >being checked and that this is a buffer overflow.  It seems simple
> >enough to analyse the call tree.
> >
> >__video_do_ioctl()
> >->  v4l_qbuf()
> >  -> vidioc_vdec_qbuf()
> >
> >It seems like Smatch is correct.  I looked at a different implementation
> >of this and that one wasn't checked either so maybe there is something
> >I am not seeing.
> >
> >This has obvious security implications.  Can someone take a look at
> >this?
> 
> This is indeed wrong.
> 
> The v4l2_m2m_qbuf() call at the end of this function calls in turn
> vb2_qbuf which
> will check the index.

There could be an issue before you reach v4l2_m2m_qbuf() because we
set "mtkbuf->lastframe = true;" so we could be corrupting random
memory.

I re-reviewed the other function I looked at earlier and that one was OK
though.

regards,
dan carpenter