* [bug report] [media] vcodec: mediatek: Add Mediatek V4L2 Video Decoder Driver
@ 2016-11-09 13:28 Dan Carpenter
2016-11-09 13:45 ` Hans Verkuil
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2016-11-09 13:28 UTC (permalink / raw)
To: tiffany.lin; +Cc: linux-media, linux-mediatek
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?
537 vb2_v4l2 = container_of(vb, struct vb2_v4l2_buffer, vb2_buf);
538 mtkbuf = container_of(vb2_v4l2, struct mtk_video_dec_buf, vb);
539
540 if ((buf->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) &&
541 (buf->m.planes[0].bytesused == 0)) {
542 mtkbuf->lastframe = true;
543 mtk_v4l2_debug(1, "[%d] (%d) id=%d lastframe=%d (%d,%d, %d) vb=%p",
544 ctx->id, buf->type, buf->index,
545 mtkbuf->lastframe, buf->bytesused,
546 buf->m.planes[0].bytesused, buf->length,
547 vb);
548 }
549
550 return v4l2_m2m_qbuf(file, ctx->m2m_ctx, buf);
551 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] [media] vcodec: mediatek: Add Mediatek V4L2 Video Decoder Driver
2016-11-09 13:28 [bug report] [media] vcodec: mediatek: Add Mediatek V4L2 Video Decoder Driver Dan Carpenter
@ 2016-11-09 13:45 ` Hans Verkuil
2016-11-10 4:31 ` Tiffany Lin
2016-11-10 13:30 ` Dan Carpenter
0 siblings, 2 replies; 4+ messages in thread
From: Hans Verkuil @ 2016-11-09 13:45 UTC (permalink / raw)
To: Dan Carpenter, tiffany.lin; +Cc: linux-media, linux-mediatek
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. But if you override vidioc_qbuf (or
vidioc_prepare), then
you need to check the index value.
I double-checked all cases where vidioc_qbuf was set to a
driver-specific function
and this is the only driver that doesn't check the index field. In all
other cases
it is either checked, or it is not used before calling into the vb1/vb2
framework
which checks this.
So luckily this only concerns this driver.
Regards,
Hans
>
> 537 vb2_v4l2 = container_of(vb, struct vb2_v4l2_buffer, vb2_buf);
> 538 mtkbuf = container_of(vb2_v4l2, struct mtk_video_dec_buf, vb);
> 539
> 540 if ((buf->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) &&
> 541 (buf->m.planes[0].bytesused == 0)) {
> 542 mtkbuf->lastframe = true;
> 543 mtk_v4l2_debug(1, "[%d] (%d) id=%d lastframe=%d (%d,%d, %d) vb=%p",
> 544 ctx->id, buf->type, buf->index,
> 545 mtkbuf->lastframe, buf->bytesused,
> 546 buf->m.planes[0].bytesused, buf->length,
> 547 vb);
> 548 }
> 549
> 550 return v4l2_m2m_qbuf(file, ctx->m2m_ctx, buf);
> 551 }
>
> regards,
> dan carpenter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] [media] vcodec: mediatek: Add Mediatek V4L2 Video Decoder Driver
2016-11-09 13:45 ` Hans Verkuil
@ 2016-11-10 4:31 ` Tiffany Lin
2016-11-10 13:30 ` Dan Carpenter
1 sibling, 0 replies; 4+ messages in thread
From: Tiffany Lin @ 2016-11-10 4:31 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Dan Carpenter, linux-media, linux-mediatek
Hi Hans, Dan,
On Wed, 2016-11-09 at 14:45 +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. But if you override vidioc_qbuf (or
> vidioc_prepare), then
> you need to check the index value.
>
> I double-checked all cases where vidioc_qbuf was set to a
> driver-specific function
> and this is the only driver that doesn't check the index field. In all
> other cases
> it is either checked, or it is not used before calling into the vb1/vb2
> framework
> which checks this.
>
> So luckily this only concerns this driver.
Thanks for point out this issue.
As Hans' mentioned, our driver access index field in v4l2_buffer before
framework check buffer index.
We will prepare patch for this issue.
best regards,
Tiffany
>
> Regards,
>
> Hans
>
> >
> > 537 vb2_v4l2 = container_of(vb, struct vb2_v4l2_buffer, vb2_buf);
> > 538 mtkbuf = container_of(vb2_v4l2, struct mtk_video_dec_buf, vb);
> > 539
> > 540 if ((buf->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) &&
> > 541 (buf->m.planes[0].bytesused == 0)) {
> > 542 mtkbuf->lastframe = true;
> > 543 mtk_v4l2_debug(1, "[%d] (%d) id=%d lastframe=%d (%d,%d, %d) vb=%p",
> > 544 ctx->id, buf->type, buf->index,
> > 545 mtkbuf->lastframe, buf->bytesused,
> > 546 buf->m.planes[0].bytesused, buf->length,
> > 547 vb);
> > 548 }
> > 549
> > 550 return v4l2_m2m_qbuf(file, ctx->m2m_ctx, buf);
> > 551 }
> >
> > regards,
> > dan carpenter
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-media" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [bug report] [media] vcodec: mediatek: Add Mediatek V4L2 Video Decoder Driver
2016-11-09 13:45 ` Hans Verkuil
2016-11-10 4:31 ` Tiffany Lin
@ 2016-11-10 13:30 ` Dan Carpenter
1 sibling, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2016-11-10 13:30 UTC (permalink / raw)
To: Hans Verkuil; +Cc: tiffany.lin, linux-media, linux-mediatek
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-11-10 13:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-09 13:28 [bug report] [media] vcodec: mediatek: Add Mediatek V4L2 Video Decoder Driver Dan Carpenter
2016-11-09 13:45 ` Hans Verkuil
2016-11-10 4:31 ` Tiffany Lin
2016-11-10 13:30 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).