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