Re: [bug report] [media] s5p-mfc: use MFC_BUF_FLAG_EOS to identify last buffers in decoder capture queue

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux