[PATCH v3 1/1] media: vivid: Fix wrong locking that causes race conditions on streaming stop

[PATCH v3 1/1] media: vivid: Fix wrong locking that causes race conditions on streaming stop

[Alexander Popov]

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)
-- 
2.21.0

Re: [PATCH v3 1/1] media: vivid: Fix wrong locking that causes race conditions on streaming stop

[Alexander Popov]

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)
> 

Re: [PATCH v3 1/1] media: vivid: Fix wrong locking that causes race conditions on streaming stop

[Linus Torvalds]

On Sat, Nov 2, 2019 at 12:03 PM Alexander Popov <alex.popov@linux.com> wrote:
>
> -               mutex_lock(&dev->mutex);
> +               if (!mutex_trylock(&dev->mutex)) {
> +                       schedule_timeout(1);
> +                       continue;
> +               }
> +

I just realized that this too is wrong. It _works_, but because it
doesn't actually set the task state to anything particular before
scheduling, it's basically pointless. It calls the scheduler, but it
won't delay anything, because the task stays runnable.

So what you presumably want to use is either "cond_resched()" (to make
sure others get to run with no delay) or
"schedule_timeout_uninterruptible(1)" which actually sets the process
state to TASK_UNINTERRUPTIBLE.

The above works, but it's basically nonsensical. My bad for not
noticing earlier.

               Linus

Re: [PATCH v3 1/1] media: vivid: Fix wrong locking that causes race conditions on streaming stop

[Alexander Popov]

On 03.11.2019 19:45, Linus Torvalds wrote:
> On Sat, Nov 2, 2019 at 12:03 PM Alexander Popov <alex.popov@linux.com> wrote:
>>
>> -               mutex_lock(&dev->mutex);
>> +               if (!mutex_trylock(&dev->mutex)) {
>> +                       schedule_timeout(1);
>> +                       continue;
>> +               }
>> +
> 
> I just realized that this too is wrong. It _works_, but because it
> doesn't actually set the task state to anything particular before
> scheduling, it's basically pointless. It calls the scheduler, but it
> won't delay anything, because the task stays runnable.

Linus, thanks for noticing that.

I've double-checked: I didn't manage to get a deadlock with schedule_timeout(1)
on the kernel with CONFIG_FREEZER disabled and CONFIG_PREEMPT_NONE=y.
But setting a bigger timeout argument (e.g. 1000) doesn't change the behavior,
which agrees with your statement.

> So what you presumably want to use is either "cond_resched()" (to make
> sure others get to run with no delay) or
> "schedule_timeout_uninterruptible(1)" which actually sets the process
> state to TASK_UNINTERRUPTIBLE.

I changed it to schedule_timeout_uninterruptible(1).

I'll send the v4 shortly as a reply to this thread, because I refer to it in the
oss-security mailing list.

> The above works, but it's basically nonsensical. My bad for not
> noticing earlier.

Thank you, now I know.

Best regards,
Alexander

[PATCH v4 1/1] media: vivid: Fix wrong locking that causes race conditions on streaming stop

[Alexander Popov]

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>
---
 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..31f78d6a05a4 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_uninterruptible(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..1e165a6a2207 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_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(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..2b7522e16efc 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_uninterruptible(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)
-- 
2.21.0