| From 6dcd5d7a7a29c1e4b8016a06aed78cd650cd8c27 Mon Sep 17 00:00:00 2001 |
| From: Alexander Popov <alex.popov@linux.com> |
| Date: Sun, 3 Nov 2019 23:17:19 +0100 |
| Subject: media: vivid: Fix wrong locking that causes race conditions on streaming stop |
| |
| From: Alexander Popov <alex.popov@linux.com> |
| |
| commit 6dcd5d7a7a29c1e4b8016a06aed78cd650cd8c27 upstream. |
| |
| 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_uninterruptible() 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> |
| Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> |
| Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> |
| Cc: <stable@vger.kernel.org> # for v3.18 and up |
| Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.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(-) |
| |
| --- a/drivers/media/platform/vivid/vivid-kthread-cap.c |
| +++ b/drivers/media/platform/vivid/vivid-kthread-cap.c |
| @@ -763,7 +763,11 @@ static int vivid_thread_vid_cap(void *da |
| if (kthread_should_stop()) |
| break; |
| |
| - mutex_lock(&dev->mutex); |
| + if (!mutex_trylock(&dev->mutex)) { |
| + schedule_timeout_uninterruptible(1); |
| + continue; |
| + } |
| + |
| cur_jiffies = jiffies; |
| if (dev->cap_seq_resync) { |
| dev->jiffies_vid_cap = cur_jiffies; |
| @@ -916,8 +920,6 @@ void vivid_stop_generating_vid_cap(struc |
| |
| /* 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); |
| } |
| --- a/drivers/media/platform/vivid/vivid-kthread-out.c |
| +++ b/drivers/media/platform/vivid/vivid-kthread-out.c |
| @@ -147,7 +147,11 @@ static int vivid_thread_vid_out(void *da |
| if (kthread_should_stop()) |
| break; |
| |
| - mutex_lock(&dev->mutex); |
| + if (!mutex_trylock(&dev->mutex)) { |
| + schedule_timeout_uninterruptible(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(struc |
| |
| /* 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); |
| } |
| --- a/drivers/media/platform/vivid/vivid-sdr-cap.c |
| +++ b/drivers/media/platform/vivid/vivid-sdr-cap.c |
| @@ -151,7 +151,11 @@ static int vivid_thread_sdr_cap(void *da |
| if (kthread_should_stop()) |
| break; |
| |
| - mutex_lock(&dev->mutex); |
| + if (!mutex_trylock(&dev->mutex)) { |
| + schedule_timeout_uninterruptible(1); |
| + continue; |
| + } |
| + |
| cur_jiffies = jiffies; |
| if (dev->sdr_cap_seq_resync) { |
| dev->jiffies_sdr_cap = cur_jiffies; |
| @@ -311,10 +315,8 @@ static void sdr_cap_stop_streaming(struc |
| } |
| |
| /* shutdown control thread */ |
| - mutex_unlock(&dev->mutex); |
| kthread_stop(dev->kthread_sdr_cap); |
| dev->kthread_sdr_cap = NULL; |
| - mutex_lock(&dev->mutex); |
| } |
| |
| const struct vb2_ops vivid_sdr_cap_qops = { |