* [bug report] [media] s5p-mfc: use MFC_BUF_FLAG_EOS to identify last buffers in decoder capture queue
@ 2019-04-04 15:42 ` Dan Carpenter
2019-04-05 10:02 ` Andrzej Hajda
0 siblings, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2019-04-04 15:42 UTC (permalink / raw)
To: a.hajda; +Cc: linux-media
Hello Andrzej Hajda,
The patch 4d0b0ed63660: "[media] s5p-mfc: use MFC_BUF_FLAG_EOS to
identify last buffers in decoder capture queue" from Oct 7, 2015,
leads to the following static checker warning:
drivers/media/platform/s5p-mfc/s5p_mfc_dec.c:664 vidioc_dqbuf()
warn: uncapped user index 'ctx->dst_bufs[buf->index]'
drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
642 static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
643 {
644 const struct v4l2_event ev = {
645 .type = V4L2_EVENT_EOS
646 };
647 struct s5p_mfc_ctx *ctx = fh_to_ctx(priv);
648 int ret;
649
650 if (ctx->state == MFCINST_ERROR) {
651 mfc_err_limited("Call on DQBUF after unrecoverable error\n");
652 return -EIO;
653 }
654
655 switch (buf->type) {
656 case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
657 return vb2_dqbuf(&ctx->vq_src, buf, file->f_flags & O_NONBLOCK);
658 case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
659 ret = vb2_dqbuf(&ctx->vq_dst, buf, file->f_flags & O_NONBLOCK);
660 if (ret)
661 return ret;
662
663 if (ctx->state == MFCINST_FINISHED &&
--> 664 (ctx->dst_bufs[buf->index].flags & MFC_BUF_FLAG_EOS))
^^^^^^^^^^
Smatch is saying that this isn't capped. The truth is that v4l2 code is
a bit complicated for Smatch, but in this case I can't see where
"buf->index" does get capped. I would have expected it to be capped in
check_array_args() where we check "buf->length" but it's not.
I've been going through these warnings really carefully in the past
couple weeks trying to fix false positives so let me know what I'm
missing and I will update the check. Even if I have to manually muck
in the DB.
665 v4l2_event_queue_fh(&ctx->fh, &ev);
666 return 0;
667 default:
668 return -EINVAL;
669 }
670 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [bug report] [media] s5p-mfc: use MFC_BUF_FLAG_EOS to identify last buffers in decoder capture queue
2019-04-04 15:42 ` [bug report] [media] s5p-mfc: use MFC_BUF_FLAG_EOS to identify last buffers in decoder capture queue Dan Carpenter
@ 2019-04-05 10:02 ` Andrzej Hajda
2019-04-05 11:33 ` Dan Carpenter
0 siblings, 1 reply; 10+ messages in thread
From: Andrzej Hajda @ 2019-04-05 10:02 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-media
Hi Dan,
On 04.04.2019 17:42, Dan Carpenter wrote:
> Hello Andrzej Hajda,
>
> The patch 4d0b0ed63660: "[media] s5p-mfc: use MFC_BUF_FLAG_EOS to
> identify last buffers in decoder capture queue" from Oct 7, 2015,
> leads to the following static checker warning:
>
> drivers/media/platform/s5p-mfc/s5p_mfc_dec.c:664 vidioc_dqbuf()
> warn: uncapped user index 'ctx->dst_bufs[buf->index]'
Almost identical e-mail you have sent about year ago, and me and Hans
have explained you that it is false positive.
Has something changed?
Regards
Andrzej
>
> drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> 642 static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
> 643 {
> 644 const struct v4l2_event ev = {
> 645 .type = V4L2_EVENT_EOS
> 646 };
> 647 struct s5p_mfc_ctx *ctx = fh_to_ctx(priv);
> 648 int ret;
> 649
> 650 if (ctx->state == MFCINST_ERROR) {
> 651 mfc_err_limited("Call on DQBUF after unrecoverable error\n");
> 652 return -EIO;
> 653 }
> 654
> 655 switch (buf->type) {
> 656 case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> 657 return vb2_dqbuf(&ctx->vq_src, buf, file->f_flags & O_NONBLOCK);
> 658 case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> 659 ret = vb2_dqbuf(&ctx->vq_dst, buf, file->f_flags & O_NONBLOCK);
> 660 if (ret)
> 661 return ret;
> 662
> 663 if (ctx->state == MFCINST_FINISHED &&
> --> 664 (ctx->dst_bufs[buf->index].flags & MFC_BUF_FLAG_EOS))
> ^^^^^^^^^^
> Smatch is saying that this isn't capped. The truth is that v4l2 code is
> a bit complicated for Smatch, but in this case I can't see where
> "buf->index" does get capped. I would have expected it to be capped in
> check_array_args() where we check "buf->length" but it's not.
>
> I've been going through these warnings really carefully in the past
> couple weeks trying to fix false positives so let me know what I'm
> missing and I will update the check. Even if I have to manually muck
> in the DB.
>
> 665 v4l2_event_queue_fh(&ctx->fh, &ev);
> 666 return 0;
> 667 default:
> 668 return -EINVAL;
> 669 }
> 670 }
>
> regards,
> dan carpenter
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [bug report] [media] s5p-mfc: use MFC_BUF_FLAG_EOS to identify last buffers in decoder capture queue
2019-04-05 10:02 ` Andrzej Hajda
@ 2019-04-05 11:33 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2019-04-05 11:33 UTC (permalink / raw)
To: Andrzej Hajda; +Cc: linux-media
On Fri, Apr 05, 2019 at 12:02:49PM +0200, Andrzej Hajda wrote:
> Hi Dan,
>
> On 04.04.2019 17:42, Dan Carpenter wrote:
> > Hello Andrzej Hajda,
> >
> > The patch 4d0b0ed63660: "[media] s5p-mfc: use MFC_BUF_FLAG_EOS to
> > identify last buffers in decoder capture queue" from Oct 7, 2015,
> > leads to the following static checker warning:
> >
> > drivers/media/platform/s5p-mfc/s5p_mfc_dec.c:664 vidioc_dqbuf()
> > warn: uncapped user index 'ctx->dst_bufs[buf->index]'
>
>
> Almost identical e-mail you have sent about year ago, and me and Hans
> have explained you that it is false positive.
>
> Has something changed?
Wow! It's remarkable how similar the emails are. Thank you for
your patience on this. I am testing a patch which should silence this
false positive in Smatch.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [bug report] [media] s5p-mfc: use MFC_BUF_FLAG_EOS to identify last buffers in decoder capture queue
2018-01-25 12:31 ` Hans Verkuil
@ 2018-01-25 13:10 ` Dan Carpenter
0 siblings, 0 replies; 10+ messages in thread
From: Dan Carpenter @ 2018-01-25 13:10 UTC (permalink / raw)
To: Hans Verkuil
Cc: Andrzej Hajda, linux-media, Mauro Carvalho Chehab, Hans Verkuil
On Thu, Jan 25, 2018 at 01:31:36PM +0100, Hans Verkuil wrote:
> > Ah... Hm. Is it the call to vb2_core_dqbuf() which limits buf->index?
> > I don't see a path from vb2_core_dqbuf() to vb2_qbuf() but I may have
> > missed it.
>
> The __fill_v4l2_buffer() function in videobuf2-v4l2.c is called by vb2_core_dqbuf().
> And that __fill_v4l2_buffer() overwrited the index field: b->index = vb->index;
>
> So after the vb2_dqbuf call the buf->index field is correct and bounded.
>
Ah.. I get it. Thanks.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [bug report] [media] s5p-mfc: use MFC_BUF_FLAG_EOS to identify last buffers in decoder capture queue
2018-01-25 12:25 ` Dan Carpenter
2018-01-25 12:31 ` Hans Verkuil
@ 2018-01-25 12:46 ` Andrzej Hajda
1 sibling, 0 replies; 10+ messages in thread
From: Andrzej Hajda @ 2018-01-25 12:46 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-media, Mauro Carvalho Chehab, Hans Verkuil
On 25.01.2018 13:25, Dan Carpenter wrote:
> On Thu, Jan 25, 2018 at 10:58:45AM +0100, Andrzej Hajda wrote:
>> On 23.01.2018 09:32, Dan Carpenter wrote:
>>> Hello Andrzej Hajda,
>>>
>>> The patch 4d0b0ed63660: "[media] s5p-mfc: use MFC_BUF_FLAG_EOS to
>>> identify last buffers in decoder capture queue" from Oct 7, 2015,
>>> leads to the following static checker warning:
>>>
>>> drivers/media/platform/s5p-mfc/s5p_mfc_dec.c:658 vidioc_dqbuf()
>>> error: buffer overflow 'ctx->dst_bufs' 32 user_rl = '0-u32max'
>>>
>>> drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>>> 635 /* Dequeue a buffer */
>>> 636 static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
>>> 637 {
>>> 638 const struct v4l2_event ev = {
>>> 639 .type = V4L2_EVENT_EOS
>>> 640 };
>>> 641 struct s5p_mfc_ctx *ctx = fh_to_ctx(priv);
>>> 642 int ret;
>>> 643
>>> 644 if (ctx->state == MFCINST_ERROR) {
>>> 645 mfc_err_limited("Call on DQBUF after unrecoverable error\n");
>>> 646 return -EIO;
>>> 647 }
>>> 648
>>> 649 switch (buf->type) {
>>> 650 case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>>> 651 return vb2_dqbuf(&ctx->vq_src, buf, file->f_flags & O_NONBLOCK);
>>> 652 case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>>> 653 ret = vb2_dqbuf(&ctx->vq_dst, buf, file->f_flags & O_NONBLOCK);
>>> 654 if (ret)
>>> 655 return ret;
>>> 656
>>> 657 if (ctx->state == MFCINST_FINISHED &&
>>> 658 (ctx->dst_bufs[buf->index].flags & MFC_BUF_FLAG_EOS))
>>> ^^^^^^^^^^
>>> Smatch is complaining that "buf->index" is not capped. So far as I can
>>> see this is true. I would have expected it to be checked in
>>> check_array_args() or video_usercopy() but I couldn't find the check.
>> I did not work in V4L2 area for long time, so I could be wrong, but I
>> hope the code is correct, below my explanation.
>> User provides only type, memory and reserved fields in buf, other fields
>> are filled by vb2_dqbuf (line 653) core function, ie index field is
>> copied from buffer which was queued by qbuf.
>> And vidioc_qbuf calls vb2_qbuf, which calls vb2_queue_or_prepare_buf,
>> which checks index bounds [1].
>>
>> So I suppose this code is correct.
>> Btw, I have also looked at other drivers and it looks omap driver
>> handles it incorrectly, ie it uses index field provided by user -
>> possible memory leak. CC Hans and Mauro, since there is no driver
>> maintainer of OMAP.
>>
>> Btw2, is it possible to check in smatch which fields of passed struct
>> given callback can read or fill ? For example here API restrict dqbuf
>> callback to read only three fields of buf, and fill the others.
>>
>> [1]:
>> http://elixir.free-electrons.com/linux/latest/source/drivers/media/v4l2-core/videobuf2-v4l2.c#L165
>> [2]:
>> http://elixir.free-electrons.com/linux/latest/source/drivers/media/platform/omap/omap_vout.c#L1520
>>
>> Regards
>> Andrzej
> Smatch does track the feilds... Smatch sees that buf->index is capped
> in vidioc_qbuf() but it still complains that buf->index gets set by the
> user in the ioctl and not checked before we use it vb2_dqbuf(). The
> call tree looks like this:
>
> --> video_usercopy()
> Copies _IOC_SIZE(cmd) bytes to parg. The _IOC_SIZE() is
> sizeof(struct v4l2_buffer) so all the feilds are reset. Smatch
> doesn't track how many bytes the users controls, it just marks
> everything in *parg as tainted but it doesn't matter in this case
> since all the feilds are set.
> video_usercopy() calls err = func(file, cmd, parg);
>
> --> __video_do_ioctl()
> calls info->u.func(ops, file, fh, arg);
>
> --> v4l_dqbuf()
> calls ops->vidioc_dqbuf(file, fh, p);
>
> --> vidioc_dqbuf()
In our case s5p_mfc_dec.c:vidioc_dqbuf
> uses unchecked buf->index
No, it uses only buf->type (it is correct usage), then calls
vb2_dqbuf(which should fill index and other fields with proper values),
and then uses .index.
> Ah... Hm. Is it the call to vb2_core_dqbuf() which limits buf->index?
No, buf->index is bound checked at buffer queuing:
s5p_mfc_dec.c:vidioc_qbuf
vb2_qbuf
vb2_queue_or_prepare_buf
> I don't see a path from vb2_core_dqbuf() to vb2_qbuf() but I may have
> missed it.
It is reverse:
s5p_mfc_dec.c:vidioc_dqbuf
vb2_dqbuf
vb2_core_dqbuf
Regards
Andrzej
>
> regards,
> dan carpenter
>
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [bug report] [media] s5p-mfc: use MFC_BUF_FLAG_EOS to identify last buffers in decoder capture queue
2018-01-25 12:25 ` Dan Carpenter
@ 2018-01-25 12:31 ` Hans Verkuil
2018-01-25 13:10 ` Dan Carpenter
2018-01-25 12:46 ` Andrzej Hajda
1 sibling, 1 reply; 10+ messages in thread
From: Hans Verkuil @ 2018-01-25 12:31 UTC (permalink / raw)
To: Dan Carpenter, Andrzej Hajda
Cc: linux-media, Mauro Carvalho Chehab, Hans Verkuil
On 01/25/2018 01:25 PM, Dan Carpenter wrote:
> On Thu, Jan 25, 2018 at 10:58:45AM +0100, Andrzej Hajda wrote:
>> On 23.01.2018 09:32, Dan Carpenter wrote:
>>> Hello Andrzej Hajda,
>>>
>>> The patch 4d0b0ed63660: "[media] s5p-mfc: use MFC_BUF_FLAG_EOS to
>>> identify last buffers in decoder capture queue" from Oct 7, 2015,
>>> leads to the following static checker warning:
>>>
>>> drivers/media/platform/s5p-mfc/s5p_mfc_dec.c:658 vidioc_dqbuf()
>>> error: buffer overflow 'ctx->dst_bufs' 32 user_rl = '0-u32max'
>>>
>>> drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>>> 635 /* Dequeue a buffer */
>>> 636 static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
>>> 637 {
>>> 638 const struct v4l2_event ev = {
>>> 639 .type = V4L2_EVENT_EOS
>>> 640 };
>>> 641 struct s5p_mfc_ctx *ctx = fh_to_ctx(priv);
>>> 642 int ret;
>>> 643
>>> 644 if (ctx->state == MFCINST_ERROR) {
>>> 645 mfc_err_limited("Call on DQBUF after unrecoverable error\n");
>>> 646 return -EIO;
>>> 647 }
>>> 648
>>> 649 switch (buf->type) {
>>> 650 case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>>> 651 return vb2_dqbuf(&ctx->vq_src, buf, file->f_flags & O_NONBLOCK);
>>> 652 case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>>> 653 ret = vb2_dqbuf(&ctx->vq_dst, buf, file->f_flags & O_NONBLOCK);
>>> 654 if (ret)
>>> 655 return ret;
>>> 656
>>> 657 if (ctx->state == MFCINST_FINISHED &&
>>> 658 (ctx->dst_bufs[buf->index].flags & MFC_BUF_FLAG_EOS))
>>> ^^^^^^^^^^
>>> Smatch is complaining that "buf->index" is not capped. So far as I can
>>> see this is true. I would have expected it to be checked in
>>> check_array_args() or video_usercopy() but I couldn't find the check.
>>
>> I did not work in V4L2 area for long time, so I could be wrong, but I
>> hope the code is correct, below my explanation.
>> User provides only type, memory and reserved fields in buf, other fields
>> are filled by vb2_dqbuf (line 653) core function, ie index field is
>> copied from buffer which was queued by qbuf.
>> And vidioc_qbuf calls vb2_qbuf, which calls vb2_queue_or_prepare_buf,
>> which checks index bounds [1].
>>
>> So I suppose this code is correct.
>> Btw, I have also looked at other drivers and it looks omap driver
>> handles it incorrectly, ie it uses index field provided by user -
>> possible memory leak. CC Hans and Mauro, since there is no driver
>> maintainer of OMAP.
>>
>> Btw2, is it possible to check in smatch which fields of passed struct
>> given callback can read or fill ? For example here API restrict dqbuf
>> callback to read only three fields of buf, and fill the others.
>>
>> [1]:
>> http://elixir.free-electrons.com/linux/latest/source/drivers/media/v4l2-core/videobuf2-v4l2.c#L165
>> [2]:
>> http://elixir.free-electrons.com/linux/latest/source/drivers/media/platform/omap/omap_vout.c#L1520
>>
>> Regards
>> Andrzej
>
> Smatch does track the feilds... Smatch sees that buf->index is capped
> in vidioc_qbuf() but it still complains that buf->index gets set by the
> user in the ioctl and not checked before we use it vb2_dqbuf(). The
> call tree looks like this:
>
> --> video_usercopy()
> Copies _IOC_SIZE(cmd) bytes to parg. The _IOC_SIZE() is
> sizeof(struct v4l2_buffer) so all the feilds are reset. Smatch
> doesn't track how many bytes the users controls, it just marks
> everything in *parg as tainted but it doesn't matter in this case
> since all the feilds are set.
> video_usercopy() calls err = func(file, cmd, parg);
>
> --> __video_do_ioctl()
> calls info->u.func(ops, file, fh, arg);
>
> --> v4l_dqbuf()
> calls ops->vidioc_dqbuf(file, fh, p);
>
> --> vidioc_dqbuf()
> uses unchecked buf->index
>
> Ah... Hm. Is it the call to vb2_core_dqbuf() which limits buf->index?
> I don't see a path from vb2_core_dqbuf() to vb2_qbuf() but I may have
> missed it.
The __fill_v4l2_buffer() function in videobuf2-v4l2.c is called by vb2_core_dqbuf().
And that __fill_v4l2_buffer() overwrited the index field: b->index = vb->index;
So after the vb2_dqbuf call the buf->index field is correct and bounded.
Regards,
Hans
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [bug report] [media] s5p-mfc: use MFC_BUF_FLAG_EOS to identify last buffers in decoder capture queue
2018-01-25 9:58 ` Andrzej Hajda
2018-01-25 10:14 ` Hans Verkuil
@ 2018-01-25 12:25 ` Dan Carpenter
2018-01-25 12:31 ` Hans Verkuil
2018-01-25 12:46 ` Andrzej Hajda
1 sibling, 2 replies; 10+ messages in thread
From: Dan Carpenter @ 2018-01-25 12:25 UTC (permalink / raw)
To: Andrzej Hajda; +Cc: linux-media, Mauro Carvalho Chehab, Hans Verkuil
On Thu, Jan 25, 2018 at 10:58:45AM +0100, Andrzej Hajda wrote:
> On 23.01.2018 09:32, Dan Carpenter wrote:
> > Hello Andrzej Hajda,
> >
> > The patch 4d0b0ed63660: "[media] s5p-mfc: use MFC_BUF_FLAG_EOS to
> > identify last buffers in decoder capture queue" from Oct 7, 2015,
> > leads to the following static checker warning:
> >
> > drivers/media/platform/s5p-mfc/s5p_mfc_dec.c:658 vidioc_dqbuf()
> > error: buffer overflow 'ctx->dst_bufs' 32 user_rl = '0-u32max'
> >
> > drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> > 635 /* Dequeue a buffer */
> > 636 static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
> > 637 {
> > 638 const struct v4l2_event ev = {
> > 639 .type = V4L2_EVENT_EOS
> > 640 };
> > 641 struct s5p_mfc_ctx *ctx = fh_to_ctx(priv);
> > 642 int ret;
> > 643
> > 644 if (ctx->state == MFCINST_ERROR) {
> > 645 mfc_err_limited("Call on DQBUF after unrecoverable error\n");
> > 646 return -EIO;
> > 647 }
> > 648
> > 649 switch (buf->type) {
> > 650 case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> > 651 return vb2_dqbuf(&ctx->vq_src, buf, file->f_flags & O_NONBLOCK);
> > 652 case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> > 653 ret = vb2_dqbuf(&ctx->vq_dst, buf, file->f_flags & O_NONBLOCK);
> > 654 if (ret)
> > 655 return ret;
> > 656
> > 657 if (ctx->state == MFCINST_FINISHED &&
> > 658 (ctx->dst_bufs[buf->index].flags & MFC_BUF_FLAG_EOS))
> > ^^^^^^^^^^
> > Smatch is complaining that "buf->index" is not capped. So far as I can
> > see this is true. I would have expected it to be checked in
> > check_array_args() or video_usercopy() but I couldn't find the check.
>
> I did not work in V4L2 area for long time, so I could be wrong, but I
> hope the code is correct, below my explanation.
> User provides only type, memory and reserved fields in buf, other fields
> are filled by vb2_dqbuf (line 653) core function, ie index field is
> copied from buffer which was queued by qbuf.
> And vidioc_qbuf calls vb2_qbuf, which calls vb2_queue_or_prepare_buf,
> which checks index bounds [1].
>
> So I suppose this code is correct.
> Btw, I have also looked at other drivers and it looks omap driver
> handles it incorrectly, ie it uses index field provided by user -
> possible memory leak. CC Hans and Mauro, since there is no driver
> maintainer of OMAP.
>
> Btw2, is it possible to check in smatch which fields of passed struct
> given callback can read or fill ? For example here API restrict dqbuf
> callback to read only three fields of buf, and fill the others.
>
> [1]:
> http://elixir.free-electrons.com/linux/latest/source/drivers/media/v4l2-core/videobuf2-v4l2.c#L165
> [2]:
> http://elixir.free-electrons.com/linux/latest/source/drivers/media/platform/omap/omap_vout.c#L1520
>
> Regards
> Andrzej
Smatch does track the feilds... Smatch sees that buf->index is capped
in vidioc_qbuf() but it still complains that buf->index gets set by the
user in the ioctl and not checked before we use it vb2_dqbuf(). The
call tree looks like this:
--> video_usercopy()
Copies _IOC_SIZE(cmd) bytes to parg. The _IOC_SIZE() is
sizeof(struct v4l2_buffer) so all the feilds are reset. Smatch
doesn't track how many bytes the users controls, it just marks
everything in *parg as tainted but it doesn't matter in this case
since all the feilds are set.
video_usercopy() calls err = func(file, cmd, parg);
--> __video_do_ioctl()
calls info->u.func(ops, file, fh, arg);
--> v4l_dqbuf()
calls ops->vidioc_dqbuf(file, fh, p);
--> vidioc_dqbuf()
uses unchecked buf->index
Ah... Hm. Is it the call to vb2_core_dqbuf() which limits buf->index?
I don't see a path from vb2_core_dqbuf() to vb2_qbuf() but I may have
missed it.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [bug report] [media] s5p-mfc: use MFC_BUF_FLAG_EOS to identify last buffers in decoder capture queue
2018-01-25 9:58 ` Andrzej Hajda
@ 2018-01-25 10:14 ` Hans Verkuil
2018-01-25 12:25 ` Dan Carpenter
1 sibling, 0 replies; 10+ messages in thread
From: Hans Verkuil @ 2018-01-25 10:14 UTC (permalink / raw)
To: Andrzej Hajda, Dan Carpenter
Cc: linux-media, Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart
On 01/25/2018 10:58 AM, Andrzej Hajda wrote:
> On 23.01.2018 09:32, Dan Carpenter wrote:
>> Hello Andrzej Hajda,
>>
>> The patch 4d0b0ed63660: "[media] s5p-mfc: use MFC_BUF_FLAG_EOS to
>> identify last buffers in decoder capture queue" from Oct 7, 2015,
>> leads to the following static checker warning:
>>
>> drivers/media/platform/s5p-mfc/s5p_mfc_dec.c:658 vidioc_dqbuf()
>> error: buffer overflow 'ctx->dst_bufs' 32 user_rl = '0-u32max'
>>
>> drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
>> 635 /* Dequeue a buffer */
>> 636 static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
>> 637 {
>> 638 const struct v4l2_event ev = {
>> 639 .type = V4L2_EVENT_EOS
>> 640 };
>> 641 struct s5p_mfc_ctx *ctx = fh_to_ctx(priv);
>> 642 int ret;
>> 643
>> 644 if (ctx->state == MFCINST_ERROR) {
>> 645 mfc_err_limited("Call on DQBUF after unrecoverable error\n");
>> 646 return -EIO;
>> 647 }
>> 648
>> 649 switch (buf->type) {
>> 650 case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
>> 651 return vb2_dqbuf(&ctx->vq_src, buf, file->f_flags & O_NONBLOCK);
>> 652 case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
>> 653 ret = vb2_dqbuf(&ctx->vq_dst, buf, file->f_flags & O_NONBLOCK);
>> 654 if (ret)
>> 655 return ret;
>> 656
>> 657 if (ctx->state == MFCINST_FINISHED &&
>> 658 (ctx->dst_bufs[buf->index].flags & MFC_BUF_FLAG_EOS))
>> ^^^^^^^^^^
>> Smatch is complaining that "buf->index" is not capped. So far as I can
>> see this is true. I would have expected it to be checked in
>> check_array_args() or video_usercopy() but I couldn't find the check.
>
> I did not work in V4L2 area for long time, so I could be wrong, but I
> hope the code is correct, below my explanation.
> User provides only type, memory and reserved fields in buf, other fields
> are filled by vb2_dqbuf (line 653) core function, ie index field is
> copied from buffer which was queued by qbuf.
> And vidioc_qbuf calls vb2_qbuf, which calls vb2_queue_or_prepare_buf,
> which checks index bounds [1].
>
> So I suppose this code is correct.
> Btw, I have also looked at other drivers and it looks omap driver
> handles it incorrectly, ie it uses index field provided by user -
> possible memory leak. CC Hans and Mauro, since there is no driver
> maintainer of OMAP.
Ouch, vidioc_dqbuf in drivers/media/platform/omap/omap_vout.c is really, really
bad code. I'm not sure how it could ever work to be honest. For one, b->index
should be returned by the driver, not set by userspace.
Luckily this driver is almost never used anymore.
Mauro, Laurent, I wonder if we just should use this reason to move this driver
to staging for one kernel release and then delete it. Rather than trying to
fix this crappy code.
Regards,
Hans
>
> Btw2, is it possible to check in smatch which fields of passed struct
> given callback can read or fill ? For example here API restrict dqbuf
> callback to read only three fields of buf, and fill the others.
>
> [1]:
> http://elixir.free-electrons.com/linux/latest/source/drivers/media/v4l2-core/videobuf2-v4l2.c#L165
> [2]:
> http://elixir.free-electrons.com/linux/latest/source/drivers/media/platform/omap/omap_vout.c#L1520
>
> Regards
> Andrzej
>>
>> 659 v4l2_event_queue_fh(&ctx->fh, &ev);
>> 660 return 0;
>> 661 default:
>> 662 return -EINVAL;
>> 663 }
>> 664 }
>>
>>
>> regards,
>> dan carpenter
>>
>>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [bug report] [media] s5p-mfc: use MFC_BUF_FLAG_EOS to identify last buffers in decoder capture queue
2018-01-23 8:32 ` Dan Carpenter
@ 2018-01-25 9:58 ` Andrzej Hajda
2018-01-25 10:14 ` Hans Verkuil
2018-01-25 12:25 ` Dan Carpenter
0 siblings, 2 replies; 10+ messages in thread
From: Andrzej Hajda @ 2018-01-25 9:58 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-media, Mauro Carvalho Chehab, Hans Verkuil
On 23.01.2018 09:32, Dan Carpenter wrote:
> Hello Andrzej Hajda,
>
> The patch 4d0b0ed63660: "[media] s5p-mfc: use MFC_BUF_FLAG_EOS to
> identify last buffers in decoder capture queue" from Oct 7, 2015,
> leads to the following static checker warning:
>
> drivers/media/platform/s5p-mfc/s5p_mfc_dec.c:658 vidioc_dqbuf()
> error: buffer overflow 'ctx->dst_bufs' 32 user_rl = '0-u32max'
>
> drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> 635 /* Dequeue a buffer */
> 636 static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
> 637 {
> 638 const struct v4l2_event ev = {
> 639 .type = V4L2_EVENT_EOS
> 640 };
> 641 struct s5p_mfc_ctx *ctx = fh_to_ctx(priv);
> 642 int ret;
> 643
> 644 if (ctx->state == MFCINST_ERROR) {
> 645 mfc_err_limited("Call on DQBUF after unrecoverable error\n");
> 646 return -EIO;
> 647 }
> 648
> 649 switch (buf->type) {
> 650 case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
> 651 return vb2_dqbuf(&ctx->vq_src, buf, file->f_flags & O_NONBLOCK);
> 652 case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
> 653 ret = vb2_dqbuf(&ctx->vq_dst, buf, file->f_flags & O_NONBLOCK);
> 654 if (ret)
> 655 return ret;
> 656
> 657 if (ctx->state == MFCINST_FINISHED &&
> 658 (ctx->dst_bufs[buf->index].flags & MFC_BUF_FLAG_EOS))
> ^^^^^^^^^^
> Smatch is complaining that "buf->index" is not capped. So far as I can
> see this is true. I would have expected it to be checked in
> check_array_args() or video_usercopy() but I couldn't find the check.
I did not work in V4L2 area for long time, so I could be wrong, but I
hope the code is correct, below my explanation.
User provides only type, memory and reserved fields in buf, other fields
are filled by vb2_dqbuf (line 653) core function, ie index field is
copied from buffer which was queued by qbuf.
And vidioc_qbuf calls vb2_qbuf, which calls vb2_queue_or_prepare_buf,
which checks index bounds [1].
So I suppose this code is correct.
Btw, I have also looked at other drivers and it looks omap driver
handles it incorrectly, ie it uses index field provided by user -
possible memory leak. CC Hans and Mauro, since there is no driver
maintainer of OMAP.
Btw2, is it possible to check in smatch which fields of passed struct
given callback can read or fill ? For example here API restrict dqbuf
callback to read only three fields of buf, and fill the others.
[1]:
http://elixir.free-electrons.com/linux/latest/source/drivers/media/v4l2-core/videobuf2-v4l2.c#L165
[2]:
http://elixir.free-electrons.com/linux/latest/source/drivers/media/platform/omap/omap_vout.c#L1520
Regards
Andrzej
>
> 659 v4l2_event_queue_fh(&ctx->fh, &ev);
> 660 return 0;
> 661 default:
> 662 return -EINVAL;
> 663 }
> 664 }
>
>
> regards,
> dan carpenter
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [bug report] [media] s5p-mfc: use MFC_BUF_FLAG_EOS to identify last buffers in decoder capture queue
@ 2018-01-23 8:32 ` Dan Carpenter
2018-01-25 9:58 ` Andrzej Hajda
0 siblings, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2018-01-23 8:32 UTC (permalink / raw)
To: a.hajda; +Cc: linux-media
Hello Andrzej Hajda,
The patch 4d0b0ed63660: "[media] s5p-mfc: use MFC_BUF_FLAG_EOS to
identify last buffers in decoder capture queue" from Oct 7, 2015,
leads to the following static checker warning:
drivers/media/platform/s5p-mfc/s5p_mfc_dec.c:658 vidioc_dqbuf()
error: buffer overflow 'ctx->dst_bufs' 32 user_rl = '0-u32max'
drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
635 /* Dequeue a buffer */
636 static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
637 {
638 const struct v4l2_event ev = {
639 .type = V4L2_EVENT_EOS
640 };
641 struct s5p_mfc_ctx *ctx = fh_to_ctx(priv);
642 int ret;
643
644 if (ctx->state == MFCINST_ERROR) {
645 mfc_err_limited("Call on DQBUF after unrecoverable error\n");
646 return -EIO;
647 }
648
649 switch (buf->type) {
650 case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
651 return vb2_dqbuf(&ctx->vq_src, buf, file->f_flags & O_NONBLOCK);
652 case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
653 ret = vb2_dqbuf(&ctx->vq_dst, buf, file->f_flags & O_NONBLOCK);
654 if (ret)
655 return ret;
656
657 if (ctx->state == MFCINST_FINISHED &&
658 (ctx->dst_bufs[buf->index].flags & MFC_BUF_FLAG_EOS))
^^^^^^^^^^
Smatch is complaining that "buf->index" is not capped. So far as I can
see this is true. I would have expected it to be checked in
check_array_args() or video_usercopy() but I couldn't find the check.
659 v4l2_event_queue_fh(&ctx->fh, &ev);
660 return 0;
661 default:
662 return -EINVAL;
663 }
664 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-04-05 11:33 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20190404154231epcas4p11c6ce6ab44d5b61c66e246fab78972e9@epcas4p1.samsung.com>
2019-04-04 15:42 ` [bug report] [media] s5p-mfc: use MFC_BUF_FLAG_EOS to identify last buffers in decoder capture queue Dan Carpenter
2019-04-05 10:02 ` Andrzej Hajda
2019-04-05 11:33 ` Dan Carpenter
[not found] <CGME20180123083259epcas3p1fb9a8b4e4ad34eb245fca67d4204cba4@epcas3p1.samsung.com>
2018-01-23 8:32 ` Dan Carpenter
2018-01-25 9:58 ` Andrzej Hajda
2018-01-25 10:14 ` Hans Verkuil
2018-01-25 12:25 ` Dan Carpenter
2018-01-25 12:31 ` Hans Verkuil
2018-01-25 13:10 ` Dan Carpenter
2018-01-25 12:46 ` Andrzej Hajda
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.