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@xxxxxxxxx>
> Acked-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> ---
>  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)
>