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 <[email protected]>
Acked-by: Linus Torvalds <[email protected]>
---
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
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 <[email protected]>
> Acked-by: Linus Torvalds <[email protected]>
> ---
> 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)
>
On Sat, Nov 2, 2019 at 12:03 PM Alexander Popov <[email protected]> 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
On 03.11.2019 19:45, Linus Torvalds wrote:
> On Sat, Nov 2, 2019 at 12:03 PM Alexander Popov <[email protected]> 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
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 <[email protected]>
Acked-by: Linus Torvalds <[email protected]>
---
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