lkml.org 
[lkml]   [2019]   [Nov]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 1/1] media: vivid: Fix wrong locking that causes race conditions on streaming stop
From
Date
I've announced this patch at the oss-security ML:
https://www.openwall.com/lists/oss-security/2019/11/02/1

Best regards,
Alexander


On 02.11.2019 22:03, Alexander Popov wrote:
> There is the same incorrect approach to locking implemented in
> vivid_stop_generating_vid_cap(), vivid_stop_generating_vid_out() and
> sdr_cap_stop_streaming().
>
> These functions are called during streaming stopping with vivid_dev.mutex
> locked. And they all do the same mistake while stopping their kthreads,
> which need to lock this mutex as well. See the example from
> vivid_stop_generating_vid_cap():
> /* shutdown control thread */
> vivid_grab_controls(dev, false);
> mutex_unlock(&dev->mutex);
> kthread_stop(dev->kthread_vid_cap);
> dev->kthread_vid_cap = NULL;
> mutex_lock(&dev->mutex);
>
> But when this mutex is unlocked, another vb2_fop_read() can lock it
> instead of vivid_thread_vid_cap() and manipulate the buffer queue.
> That causes a use-after-free access later.
>
> To fix those issues let's:
> 1. avoid unlocking the mutex in vivid_stop_generating_vid_cap(),
> vivid_stop_generating_vid_out() and sdr_cap_stop_streaming();
> 2. use mutex_trylock() with schedule_timeout() in the loops
> of the vivid kthread handlers.
>
> Signed-off-by: Alexander Popov <alex.popov@linux.com>
> Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
> drivers/media/platform/vivid/vivid-kthread-cap.c | 8 +++++---
> drivers/media/platform/vivid/vivid-kthread-out.c | 8 +++++---
> drivers/media/platform/vivid/vivid-sdr-cap.c | 8 +++++---
> 3 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/vivid/vivid-kthread-cap.c b/drivers/media/platform/vivid/vivid-kthread-cap.c
> index 003319d7816d..27b9c78d2d05 100644
> --- a/drivers/media/platform/vivid/vivid-kthread-cap.c
> +++ b/drivers/media/platform/vivid/vivid-kthread-cap.c
> @@ -796,7 +796,11 @@ static int vivid_thread_vid_cap(void *data)
> if (kthread_should_stop())
> break;
>
> - mutex_lock(&dev->mutex);
> + if (!mutex_trylock(&dev->mutex)) {
> + schedule_timeout(1);
> + continue;
> + }
> +
> cur_jiffies = jiffies;
> if (dev->cap_seq_resync) {
> dev->jiffies_vid_cap = cur_jiffies;
> @@ -956,8 +960,6 @@ void vivid_stop_generating_vid_cap(struct vivid_dev *dev, bool *pstreaming)
>
> /* shutdown control thread */
> vivid_grab_controls(dev, false);
> - mutex_unlock(&dev->mutex);
> kthread_stop(dev->kthread_vid_cap);
> dev->kthread_vid_cap = NULL;
> - mutex_lock(&dev->mutex);
> }
> diff --git a/drivers/media/platform/vivid/vivid-kthread-out.c b/drivers/media/platform/vivid/vivid-kthread-out.c
> index ce5bcda2348c..a657b0d20e2f 100644
> --- a/drivers/media/platform/vivid/vivid-kthread-out.c
> +++ b/drivers/media/platform/vivid/vivid-kthread-out.c
> @@ -143,7 +143,11 @@ static int vivid_thread_vid_out(void *data)
> if (kthread_should_stop())
> break;
>
> - mutex_lock(&dev->mutex);
> + if (!mutex_trylock(&dev->mutex)) {
> + schedule_timeout(1);
> + continue;
> + }
> +
> cur_jiffies = jiffies;
> if (dev->out_seq_resync) {
> dev->jiffies_vid_out = cur_jiffies;
> @@ -301,8 +305,6 @@ void vivid_stop_generating_vid_out(struct vivid_dev *dev, bool *pstreaming)
>
> /* shutdown control thread */
> vivid_grab_controls(dev, false);
> - mutex_unlock(&dev->mutex);
> kthread_stop(dev->kthread_vid_out);
> dev->kthread_vid_out = NULL;
> - mutex_lock(&dev->mutex);
> }
> diff --git a/drivers/media/platform/vivid/vivid-sdr-cap.c b/drivers/media/platform/vivid/vivid-sdr-cap.c
> index 9acc709b0740..590080716962 100644
> --- a/drivers/media/platform/vivid/vivid-sdr-cap.c
> +++ b/drivers/media/platform/vivid/vivid-sdr-cap.c
> @@ -141,7 +141,11 @@ static int vivid_thread_sdr_cap(void *data)
> if (kthread_should_stop())
> break;
>
> - mutex_lock(&dev->mutex);
> + if (!mutex_trylock(&dev->mutex)) {
> + schedule_timeout(1);
> + continue;
> + }
> +
> cur_jiffies = jiffies;
> if (dev->sdr_cap_seq_resync) {
> dev->jiffies_sdr_cap = cur_jiffies;
> @@ -303,10 +307,8 @@ static void sdr_cap_stop_streaming(struct vb2_queue *vq)
> }
>
> /* shutdown control thread */
> - mutex_unlock(&dev->mutex);
> kthread_stop(dev->kthread_sdr_cap);
> dev->kthread_sdr_cap = NULL;
> - mutex_lock(&dev->mutex);
> }
>
> static void sdr_cap_buf_request_complete(struct vb2_buffer *vb)
>

\
 
 \ /
  Last update: 2019-11-02 21:51    [W:0.040 / U:1.532 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site