[PATCH] media: pvrusb2: fix disconnect and teardown races

[PATCH] media: pvrusb2: fix disconnect and teardown races

[Sangyun Kim]

pvr2_context_disconnect() queues a notification to the pvrusb2-context
kthread before it stores mp->disconnect_flag:

    pvr2_hdw_disconnect(mp->hdw);
    if (!pvr2_context_shutok())
        pvr2_context_notify(mp);
    mp->disconnect_flag = !0;

The context thread only destroys a context when disconnect_flag is set
and mc_first is NULL. If the notification wakes the thread before the
flag store becomes visible, the thread dequeues the context, runs
pvr2_context_check() with disconnect_flag still observed as 0, decides
that the destroy condition is not met yet, and goes back to sleep.
Nothing wakes the thread again once the flag is finally stored, so the
context stays on the global exist list forever and
pvr2_context_global_done() blocks module unload. commit 0a0b79ea55de
("media: pvrusb2: fix uaf in pvr2_context_set_notify") made this
liveness failure easier to hit by moving the notify earlier in the
disconnect path.

The same teardown sequence still contains a use-after-free.
pvr2_context_exit() inspects disconnect_flag after releasing mp->mutex
and may then call pvr2_context_notify(mp) after the context thread has
already freed the object via pvr2_context_destroy()/kfree(). The hdw
completion callback registered through pvr2_hdw_initialize() can race
the same way. Reordering the disconnect path alone closes the unload
hang, but it still leaves late notifiers able to touch freed memory.

Fix both problems together:

- Split pvr2_context_set_notify() into a locked helper
  (pvr2_context_set_notify_locked()) and a wrapper that acquires
  pvr2_context_mutex. This lets callers update several pieces of
  related state inside a single critical section without relocking.

- In pvr2_context_disconnect(), set disconnect_flag and enqueue the
  thread notification under pvr2_context_mutex. The context thread
  manipulates the notify list under the same mutex, so when it observes
  the queued entry it is guaranteed to observe disconnect_flag = 1 as
  well and the destroy condition evaluates correctly. This eliminates
  the original notify-before-flag liveness hole.

- Add a per-context refcount_t. pvr2_context_create() initialises it to
  1 (creator reference). pvr2_channel_init() and pvr2_channel_done()
  take and drop a reference around each channel's lifetime.
  pvr2_context_disconnect() takes a temporary reference across its body
  so the context cannot be freed while disconnect is still touching it.
  pvr2_context_destroy() no longer calls kfree() directly; it drops its
  reference via pvr2_context_put(), and whichever caller drops the last
  reference performs the actual kfree. This keeps the object alive
  until disconnect and the final channel teardown finish, regardless of
  how the context thread, channel close, and USB disconnect paths
  interleave.

- Add a destroying_flag that pvr2_context_destroy() sets under
  pvr2_context_mutex before unlinking the context from the notify and
  exist lists. pvr2_context_set_notify_locked() refuses to re-enqueue a
  context whose destroying_flag is set, so a late notifier arriving
  after destroy has started cannot resurrect the context on the notify
  list. The dequeue path (fl == 0) still proceeds unconditionally
  because destroy itself must be able to remove any still-queued entry.

- Update pvr2_context_exit() to enqueue through
  pvr2_context_set_notify_locked() after releasing mp->mutex. The
  caller (channel close or disconnect) always holds a reference, so the
  object is stable across the mp->mutex / pvr2_context_mutex hand-off
  and a concurrent destroy cannot free it under us. If destroy has
  already won the race, destroying_flag short-circuits the enqueue into
  a no-op.

Lock ordering: pvr2_context_mutex is only acquired after mp->mutex is
released; no path holds pvr2_context_mutex while acquiring mp->mutex,
so no AB/BA deadlock is introduced. wake_up() on
pvr2_context_sync_data is moved outside pvr2_context_mutex in every
path that grew a new locked section, matching the existing style.

Fixes: 0a0b79ea55de ("media: pvrusb2: fix uaf in pvr2_context_set_notify")
Cc: stable@vger.kernel.org
Signed-off-by: Sangyun Kim <sangyun.kim@snu.ac.kr>
---
 drivers/media/usb/pvrusb2/pvrusb2-context.c | 56 ++++++++++++++++++---
 drivers/media/usb/pvrusb2/pvrusb2-context.h |  3 ++
 2 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/pvrusb2/pvrusb2-context.c b/drivers/media/usb/pvrusb2/pvrusb2-context.c
index 93f5da65ead9..fb9bdbf5c886 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-context.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-context.c
@@ -27,11 +27,19 @@ static int pvr2_context_cleaned_flag;
 static struct task_struct *pvr2_context_thread_ptr;
 
 
-static void pvr2_context_set_notify(struct pvr2_context *mp, int fl)
+static void pvr2_context_put(struct pvr2_context *mp)
+{
+	if (refcount_dec_and_test(&mp->refcount))
+		kfree(mp);
+}
+
+static int pvr2_context_set_notify_locked(struct pvr2_context *mp, int fl)
 {
 	int signal_flag = 0;
-	mutex_lock(&pvr2_context_mutex);
+
 	if (fl) {
+		if (mp->destroying_flag)
+			return 0;
 		if (!mp->notify_flag) {
 			signal_flag = (pvr2_context_notify_first == NULL);
 			mp->notify_prev = pvr2_context_notify_last;
@@ -59,6 +67,15 @@ static void pvr2_context_set_notify(struct pvr2_context *mp, int fl)
 			}
 		}
 	}
+	return signal_flag;
+}
+
+static void pvr2_context_set_notify(struct pvr2_context *mp, int fl)
+{
+	int signal_flag = 0;
+
+	mutex_lock(&pvr2_context_mutex);
+	signal_flag = pvr2_context_set_notify_locked(mp, fl);
 	mutex_unlock(&pvr2_context_mutex);
 	if (signal_flag) wake_up(&pvr2_context_sync_data);
 }
@@ -66,10 +83,13 @@ static void pvr2_context_set_notify(struct pvr2_context *mp, int fl)
 
 static void pvr2_context_destroy(struct pvr2_context *mp)
 {
+	int signal_flag = 0;
+
 	pvr2_trace(PVR2_TRACE_CTXT,"pvr2_context %p (destroy)",mp);
 	pvr2_hdw_destroy(mp->hdw);
-	pvr2_context_set_notify(mp, 0);
 	mutex_lock(&pvr2_context_mutex);
+	mp->destroying_flag = !0;
+	pvr2_context_set_notify_locked(mp, 0);
 	if (mp->exist_next) {
 		mp->exist_next->exist_prev = mp->exist_prev;
 	} else {
@@ -83,10 +103,12 @@ static void pvr2_context_destroy(struct pvr2_context *mp)
 	if (!pvr2_context_exist_first) {
 		/* Trigger wakeup on control thread in case it is waiting
 		   for an exit condition. */
-		wake_up(&pvr2_context_sync_data);
+		signal_flag = !0;
 	}
 	mutex_unlock(&pvr2_context_mutex);
-	kfree(mp);
+	if (signal_flag)
+		wake_up(&pvr2_context_sync_data);
+	pvr2_context_put(mp);
 }
 
 
@@ -209,6 +231,7 @@ struct pvr2_context *pvr2_context_create(
 	pvr2_trace(PVR2_TRACE_CTXT,"pvr2_context %p (create)",mp);
 	mp->setup_func = setup_func;
 	mutex_init(&mp->mutex);
+	refcount_set(&mp->refcount, 1);
 	mutex_lock(&pvr2_context_mutex);
 	mp->exist_prev = pvr2_context_exist_last;
 	mp->exist_next = NULL;
@@ -256,25 +279,41 @@ static void pvr2_context_enter(struct pvr2_context *mp)
 static void pvr2_context_exit(struct pvr2_context *mp)
 {
 	int destroy_flag = 0;
+	int signal_flag = 0;
 	if (!(mp->mc_first || !mp->disconnect_flag)) {
 		destroy_flag = !0;
 	}
 	mutex_unlock(&mp->mutex);
-	if (destroy_flag) pvr2_context_notify(mp);
+	if (destroy_flag) {
+		mutex_lock(&pvr2_context_mutex);
+		signal_flag = pvr2_context_set_notify_locked(mp, !0);
+		mutex_unlock(&pvr2_context_mutex);
+		if (signal_flag)
+			wake_up(&pvr2_context_sync_data);
+	}
 }
 
 
 void pvr2_context_disconnect(struct pvr2_context *mp)
 {
+	int signal_flag = 0;
+
+	refcount_inc(&mp->refcount);
 	pvr2_hdw_disconnect(mp->hdw);
-	if (!pvr2_context_shutok())
-		pvr2_context_notify(mp);
+	mutex_lock(&pvr2_context_mutex);
 	mp->disconnect_flag = !0;
+	if (!pvr2_context_shutok())
+		signal_flag = pvr2_context_set_notify_locked(mp, !0);
+	mutex_unlock(&pvr2_context_mutex);
+	if (signal_flag)
+		wake_up(&pvr2_context_sync_data);
+	pvr2_context_put(mp);
 }
 
 
 void pvr2_channel_init(struct pvr2_channel *cp,struct pvr2_context *mp)
 {
+	refcount_inc(&mp->refcount);
 	pvr2_context_enter(mp);
 	cp->hdw = mp->hdw;
 	cp->mc_head = mp;
@@ -318,6 +357,7 @@ void pvr2_channel_done(struct pvr2_channel *cp)
 	}
 	cp->hdw = NULL;
 	pvr2_context_exit(mp);
+	pvr2_context_put(mp);
 }
 
 
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-context.h b/drivers/media/usb/pvrusb2/pvrusb2-context.h
index 5840b2ce8f1e..4e06530eccb8 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-context.h
+++ b/drivers/media/usb/pvrusb2/pvrusb2-context.h
@@ -7,6 +7,7 @@
 #define __PVRUSB2_CONTEXT_H
 
 #include <linux/mutex.h>
+#include <linux/refcount.h>
 #include <linux/usb.h>
 #include <linux/workqueue.h>
 
@@ -33,9 +34,11 @@ struct pvr2_context {
 	struct pvr2_hdw *hdw;
 	struct pvr2_context_stream video_stream;
 	struct mutex mutex;
+	refcount_t refcount;
 	int notify_flag;
 	int initialized_flag;
 	int disconnect_flag;
+	int destroying_flag;
 
 	/* Called after pvr2_context initialization is complete */
 	void (*setup_func)(struct pvr2_context *);
-- 
2.34.1

Re: [PATCH] media: pvrusb2: fix disconnect and teardown races

[Hans Verkuil]

On 4/20/26 08:06, Sangyun Kim wrote:
> pvr2_context_disconnect() queues a notification to the pvrusb2-context
> kthread before it stores mp->disconnect_flag:
> 
>     pvr2_hdw_disconnect(mp->hdw);
>     if (!pvr2_context_shutok())
>         pvr2_context_notify(mp);
>     mp->disconnect_flag = !0;
> 
> The context thread only destroys a context when disconnect_flag is set
> and mc_first is NULL. If the notification wakes the thread before the
> flag store becomes visible, the thread dequeues the context, runs
> pvr2_context_check() with disconnect_flag still observed as 0, decides
> that the destroy condition is not met yet, and goes back to sleep.
> Nothing wakes the thread again once the flag is finally stored, so the
> context stays on the global exist list forever and
> pvr2_context_global_done() blocks module unload. commit 0a0b79ea55de
> ("media: pvrusb2: fix uaf in pvr2_context_set_notify") made this
> liveness failure easier to hit by moving the notify earlier in the
> disconnect path.
> 
> The same teardown sequence still contains a use-after-free.
> pvr2_context_exit() inspects disconnect_flag after releasing mp->mutex
> and may then call pvr2_context_notify(mp) after the context thread has
> already freed the object via pvr2_context_destroy()/kfree(). The hdw
> completion callback registered through pvr2_hdw_initialize() can race
> the same way. Reordering the disconnect path alone closes the unload
> hang, but it still leaves late notifiers able to touch freed memory.
> 
> Fix both problems together:
> 
> - Split pvr2_context_set_notify() into a locked helper
>   (pvr2_context_set_notify_locked()) and a wrapper that acquires
>   pvr2_context_mutex. This lets callers update several pieces of
>   related state inside a single critical section without relocking.
> 
> - In pvr2_context_disconnect(), set disconnect_flag and enqueue the
>   thread notification under pvr2_context_mutex. The context thread
>   manipulates the notify list under the same mutex, so when it observes
>   the queued entry it is guaranteed to observe disconnect_flag = 1 as
>   well and the destroy condition evaluates correctly. This eliminates
>   the original notify-before-flag liveness hole.
> 
> - Add a per-context refcount_t. pvr2_context_create() initialises it to
>   1 (creator reference). pvr2_channel_init() and pvr2_channel_done()
>   take and drop a reference around each channel's lifetime.
>   pvr2_context_disconnect() takes a temporary reference across its body
>   so the context cannot be freed while disconnect is still touching it.
>   pvr2_context_destroy() no longer calls kfree() directly; it drops its
>   reference via pvr2_context_put(), and whichever caller drops the last
>   reference performs the actual kfree. This keeps the object alive
>   until disconnect and the final channel teardown finish, regardless of
>   how the context thread, channel close, and USB disconnect paths
>   interleave.
> 
> - Add a destroying_flag that pvr2_context_destroy() sets under
>   pvr2_context_mutex before unlinking the context from the notify and
>   exist lists. pvr2_context_set_notify_locked() refuses to re-enqueue a
>   context whose destroying_flag is set, so a late notifier arriving
>   after destroy has started cannot resurrect the context on the notify
>   list. The dequeue path (fl == 0) still proceeds unconditionally
>   because destroy itself must be able to remove any still-queued entry.
> 
> - Update pvr2_context_exit() to enqueue through
>   pvr2_context_set_notify_locked() after releasing mp->mutex. The
>   caller (channel close or disconnect) always holds a reference, so the
>   object is stable across the mp->mutex / pvr2_context_mutex hand-off
>   and a concurrent destroy cannot free it under us. If destroy has
>   already won the race, destroying_flag short-circuits the enqueue into
>   a no-op.
> 
> Lock ordering: pvr2_context_mutex is only acquired after mp->mutex is
> released; no path holds pvr2_context_mutex while acquiring mp->mutex,
> so no AB/BA deadlock is introduced. wake_up() on
> pvr2_context_sync_data is moved outside pvr2_context_mutex in every
> path that grew a new locked section, matching the existing style.

Huge commit log, weird patch. All the hallmarks of AI slop.

Rejected.

Regards,

	Hans

> 
> Fixes: 0a0b79ea55de ("media: pvrusb2: fix uaf in pvr2_context_set_notify")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sangyun Kim <sangyun.kim@snu.ac.kr>
> ---
>  drivers/media/usb/pvrusb2/pvrusb2-context.c | 56 ++++++++++++++++++---
>  drivers/media/usb/pvrusb2/pvrusb2-context.h |  3 ++
>  2 files changed, 51 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-context.c b/drivers/media/usb/pvrusb2/pvrusb2-context.c
> index 93f5da65ead9..fb9bdbf5c886 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-context.c
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-context.c
> @@ -27,11 +27,19 @@ static int pvr2_context_cleaned_flag;
>  static struct task_struct *pvr2_context_thread_ptr;
>  
>  
> -static void pvr2_context_set_notify(struct pvr2_context *mp, int fl)
> +static void pvr2_context_put(struct pvr2_context *mp)
> +{
> +	if (refcount_dec_and_test(&mp->refcount))
> +		kfree(mp);
> +}
> +
> +static int pvr2_context_set_notify_locked(struct pvr2_context *mp, int fl)
>  {
>  	int signal_flag = 0;
> -	mutex_lock(&pvr2_context_mutex);
> +
>  	if (fl) {
> +		if (mp->destroying_flag)
> +			return 0;
>  		if (!mp->notify_flag) {
>  			signal_flag = (pvr2_context_notify_first == NULL);
>  			mp->notify_prev = pvr2_context_notify_last;
> @@ -59,6 +67,15 @@ static void pvr2_context_set_notify(struct pvr2_context *mp, int fl)
>  			}
>  		}
>  	}
> +	return signal_flag;
> +}
> +
> +static void pvr2_context_set_notify(struct pvr2_context *mp, int fl)
> +{
> +	int signal_flag = 0;
> +
> +	mutex_lock(&pvr2_context_mutex);
> +	signal_flag = pvr2_context_set_notify_locked(mp, fl);
>  	mutex_unlock(&pvr2_context_mutex);
>  	if (signal_flag) wake_up(&pvr2_context_sync_data);
>  }
> @@ -66,10 +83,13 @@ static void pvr2_context_set_notify(struct pvr2_context *mp, int fl)
>  
>  static void pvr2_context_destroy(struct pvr2_context *mp)
>  {
> +	int signal_flag = 0;
> +
>  	pvr2_trace(PVR2_TRACE_CTXT,"pvr2_context %p (destroy)",mp);
>  	pvr2_hdw_destroy(mp->hdw);
> -	pvr2_context_set_notify(mp, 0);
>  	mutex_lock(&pvr2_context_mutex);
> +	mp->destroying_flag = !0;
> +	pvr2_context_set_notify_locked(mp, 0);
>  	if (mp->exist_next) {
>  		mp->exist_next->exist_prev = mp->exist_prev;
>  	} else {
> @@ -83,10 +103,12 @@ static void pvr2_context_destroy(struct pvr2_context *mp)
>  	if (!pvr2_context_exist_first) {
>  		/* Trigger wakeup on control thread in case it is waiting
>  		   for an exit condition. */
> -		wake_up(&pvr2_context_sync_data);
> +		signal_flag = !0;
>  	}
>  	mutex_unlock(&pvr2_context_mutex);
> -	kfree(mp);
> +	if (signal_flag)
> +		wake_up(&pvr2_context_sync_data);
> +	pvr2_context_put(mp);
>  }
>  
>  
> @@ -209,6 +231,7 @@ struct pvr2_context *pvr2_context_create(
>  	pvr2_trace(PVR2_TRACE_CTXT,"pvr2_context %p (create)",mp);
>  	mp->setup_func = setup_func;
>  	mutex_init(&mp->mutex);
> +	refcount_set(&mp->refcount, 1);
>  	mutex_lock(&pvr2_context_mutex);
>  	mp->exist_prev = pvr2_context_exist_last;
>  	mp->exist_next = NULL;
> @@ -256,25 +279,41 @@ static void pvr2_context_enter(struct pvr2_context *mp)
>  static void pvr2_context_exit(struct pvr2_context *mp)
>  {
>  	int destroy_flag = 0;
> +	int signal_flag = 0;
>  	if (!(mp->mc_first || !mp->disconnect_flag)) {
>  		destroy_flag = !0;
>  	}
>  	mutex_unlock(&mp->mutex);
> -	if (destroy_flag) pvr2_context_notify(mp);
> +	if (destroy_flag) {
> +		mutex_lock(&pvr2_context_mutex);
> +		signal_flag = pvr2_context_set_notify_locked(mp, !0);
> +		mutex_unlock(&pvr2_context_mutex);
> +		if (signal_flag)
> +			wake_up(&pvr2_context_sync_data);
> +	}
>  }
>  
>  
>  void pvr2_context_disconnect(struct pvr2_context *mp)
>  {
> +	int signal_flag = 0;
> +
> +	refcount_inc(&mp->refcount);
>  	pvr2_hdw_disconnect(mp->hdw);
> -	if (!pvr2_context_shutok())
> -		pvr2_context_notify(mp);
> +	mutex_lock(&pvr2_context_mutex);
>  	mp->disconnect_flag = !0;
> +	if (!pvr2_context_shutok())
> +		signal_flag = pvr2_context_set_notify_locked(mp, !0);
> +	mutex_unlock(&pvr2_context_mutex);
> +	if (signal_flag)
> +		wake_up(&pvr2_context_sync_data);
> +	pvr2_context_put(mp);
>  }
>  
>  
>  void pvr2_channel_init(struct pvr2_channel *cp,struct pvr2_context *mp)
>  {
> +	refcount_inc(&mp->refcount);
>  	pvr2_context_enter(mp);
>  	cp->hdw = mp->hdw;
>  	cp->mc_head = mp;
> @@ -318,6 +357,7 @@ void pvr2_channel_done(struct pvr2_channel *cp)
>  	}
>  	cp->hdw = NULL;
>  	pvr2_context_exit(mp);
> +	pvr2_context_put(mp);
>  }
>  
>  
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-context.h b/drivers/media/usb/pvrusb2/pvrusb2-context.h
> index 5840b2ce8f1e..4e06530eccb8 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-context.h
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-context.h
> @@ -7,6 +7,7 @@
>  #define __PVRUSB2_CONTEXT_H
>  
>  #include <linux/mutex.h>
> +#include <linux/refcount.h>
>  #include <linux/usb.h>
>  #include <linux/workqueue.h>
>  
> @@ -33,9 +34,11 @@ struct pvr2_context {
>  	struct pvr2_hdw *hdw;
>  	struct pvr2_context_stream video_stream;
>  	struct mutex mutex;
> +	refcount_t refcount;
>  	int notify_flag;
>  	int initialized_flag;
>  	int disconnect_flag;
> +	int destroying_flag;
>  
>  	/* Called after pvr2_context initialization is complete */
>  	void (*setup_func)(struct pvr2_context *);