[PATCHv2 0/4] vb2: fix syzkaller race conditions

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

 



These four patches fix syzkaller race conditions. The first patch
fixes the case where VIDIOC_DQBUF (and indirectly via read()/write())
can release the core serialization mutex, thus allowing another thread
to access the same vb2 queue through a dup()ped filehandle.

If no new buffer is available the DQBUF ioctl will block and wait for
a new buffer to arrive. Before it waits it releases the serialization
lock, and afterwards it reacquires it. This is intentional, since you
do not want to block other ioctls while waiting for a buffer.

But this means that you need to flag that you are waiting for a buffer
and check the flag in the appropriate places.

Specifically, that has to happen for VIDIOC_REQBUFS and VIDIOC_CREATE_BUFS
since those can free/reallocate all buffers. Obviously you should not do
that while waiting for a new frame to arrive. The other place where the
flag should be checked is in VIDIOC_DQBUF and read/write since it makes
not sense to call those while another fd is already waiting for a new
frame.

The remaining three patches fix a problem with vivid: due to the way
vivid was designed it had to release the dev->mutex lock when stop_streaming
was called. However, that was the same lock that was assigned to
queue->lock, so that caused a race condition as well. It really is a
vivid bug, which I fixed by giving each queue its own lock instead of
relying on dev->mutex.

It is a good idea to have vivid do this, since, while vb2 has allowed this
for a long time, no drivers were actually using that feature.

But while analyzing the behavior of vivid and vb2 in this scenario I
realized that doing this (i.e. separate mutexes per queue) caused another
race between calling queue_setup and VIDIOC_S_FMT: if queue->lock and
the ioctl serialization lock are the same, then those operations are
nicely serialized. But if the locks are different, then it is possible
that S_FMT changes the buffer size right after queue_setup returns.

So queue_setup might report that each buffer is 1 MB, while the S_FMT
changes it to 2 MB. So there is now a mismatch between what vb2 thinks
the size should be and what the driver thinks.

So to do this correctly the ioctl serialization lock (or whatever the
driver uses for that) should be taken before calling queue_setup and
released once q->num_buffers has been updated (so vb2_is_busy()
will return true).

The final two patches add support for that.

Regards,

	Hans

Hans Verkuil (4):
  vb2: add waiting_in_dqbuf flag
  vivid: use per-queue mutexes instead of one global mutex.
  vb2 core: add new queue_setup_lock/unlock ops
  vivid: add queue_setup_(un)lock ops

 .../media/common/videobuf2/videobuf2-core.c   | 71 +++++++++++++++----
 drivers/media/platform/vivid/vivid-core.c     | 29 ++++++--
 drivers/media/platform/vivid/vivid-core.h     |  8 +++
 .../media/platform/vivid/vivid-kthread-cap.c  |  2 -
 .../media/platform/vivid/vivid-kthread-out.c  |  2 -
 drivers/media/platform/vivid/vivid-sdr-cap.c  |  4 +-
 drivers/media/platform/vivid/vivid-vbi-cap.c  |  2 +
 drivers/media/platform/vivid/vivid-vbi-out.c  |  2 +
 drivers/media/platform/vivid/vivid-vid-cap.c  |  2 +
 drivers/media/platform/vivid/vivid-vid-out.c  |  2 +
 include/media/videobuf2-core.h                | 20 ++++++
 11 files changed, 119 insertions(+), 25 deletions(-)

-- 
2.19.1




[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