* [PATCH v3 1/1] media: vivid: Fix wrong locking that causes race conditions on streaming stop
@ 2019-11-02 19:03 Alexander Popov
2019-11-02 20:50 ` Alexander Popov
2019-11-03 16:45 ` Linus Torvalds
0 siblings, 2 replies; 5+ messages in thread
From: Alexander Popov @ 2019-11-02 19:03 UTC (permalink / raw)
To: Linus Torvalds, Hans Verkuil, Mauro Carvalho Chehab,
Security Officers, linux-media, linux-kernel, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v3 1/1] media: vivid: Fix wrong locking that causes race conditions on streaming stop
2019-11-02 19:03 [PATCH v3 1/1] media: vivid: Fix wrong locking that causes race conditions on streaming stop Alexander Popov
@ 2019-11-02 20:50 ` Alexander Popov
2019-11-03 16:45 ` Linus Torvalds
1 sibling, 0 replies; 5+ messages in thread
From: Alexander Popov @ 2019-11-02 20:50 UTC (permalink / raw)
To: Linus Torvalds, Hans Verkuil, Mauro Carvalho Chehab,
Security Officers, linux-media, linux-kernel
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)
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3 1/1] media: vivid: Fix wrong locking that causes race conditions on streaming stop
2019-11-02 19:03 [PATCH v3 1/1] media: vivid: Fix wrong locking that causes race conditions on streaming stop Alexander Popov
2019-11-02 20:50 ` Alexander Popov
@ 2019-11-03 16:45 ` Linus Torvalds
2019-11-03 21:44 ` Alexander Popov
1 sibling, 1 reply; 5+ messages in thread
From: Linus Torvalds @ 2019-11-03 16:45 UTC (permalink / raw)
To: Alexander Popov
Cc: Hans Verkuil, Mauro Carvalho Chehab, Security Officers,
Linux Media Mailing List, Linux Kernel Mailing List
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3 1/1] media: vivid: Fix wrong locking that causes race conditions on streaming stop
2019-11-03 16:45 ` Linus Torvalds
@ 2019-11-03 21:44 ` Alexander Popov
2019-11-03 22:17 ` [PATCH v4 " Alexander Popov
0 siblings, 1 reply; 5+ messages in thread
From: Alexander Popov @ 2019-11-03 21:44 UTC (permalink / raw)
To: Linus Torvalds
Cc: Hans Verkuil, Mauro Carvalho Chehab, Security Officers,
Linux Media Mailing List, Linux Kernel Mailing List,
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v4 1/1] media: vivid: Fix wrong locking that causes race conditions on streaming stop
2019-11-03 21:44 ` Alexander Popov
@ 2019-11-03 22:17 ` Alexander Popov
0 siblings, 0 replies; 5+ messages in thread
From: Alexander Popov @ 2019-11-03 22:17 UTC (permalink / raw)
To: Linus Torvalds, Hans Verkuil, Mauro Carvalho Chehab,
Security Officers, linux-media, linux-kernel, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-11-03 22:17 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-02 19:03 [PATCH v3 1/1] media: vivid: Fix wrong locking that causes race conditions on streaming stop Alexander Popov
2019-11-02 20:50 ` Alexander Popov
2019-11-03 16:45 ` Linus Torvalds
2019-11-03 21:44 ` Alexander Popov
2019-11-03 22:17 ` [PATCH v4 " Alexander Popov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).