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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8edf1056-072d-9718-74ac-e6b8c0339228@linux.com>
Date:   Sat, 2 Nov 2019 23:50:08 +0300
From:   Alexander Popov <alex.popov@...ux.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        Security Officers <security@...nel.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/1] media: vivid: Fix wrong locking that causes race
 conditions on streaming stop

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@...ux.com>
> Acked-by: Linus Torvalds <torvalds@...ux-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)
>