[PATCH v3] media: dvb-core: skip disable_source handler for a released entity

[YuanZhe Liu]

dvb_remove_device() frees the media entity of a dvb_device through
dvb_media_device_free() and sets dvbdev->entity to NULL.  The media
device of the adapter can still be registered at that point: au0828,
for example, only clears mdev->disable_source in au0828_usb_release(),
which runs after au0828_dvb_unregister() has unregistered the frontend
device.

A file descriptor that survives the disconnect then reaches
dvb_frontend_release() with dvbdev->entity already NULL and calls

	fe->dvb->mdev->disable_source(dvbdev->entity);

au0828_disable_source() dereferences that argument unconditionally:

	struct media_device *mdev = entity->graph_obj.mdev;

which faults:

	general protection fault: 0000 [#1] SMP KASAN
	KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
	RIP: au0828_disable_source+0x27/0xb70 au0828-core.c:445
	Call Trace:
	 dvb_frontend_release+0x2eb/0x500 dvb_frontend.c:2946

The error path of __dvb_frontend_open() has the same call.

Skip the handler when dvbdev->entity is gone.  For that check to be
sufficient, clear dvbdev->entity in dvb_media_device_free() before the
entity is unregistered and freed: both frontend paths read the pointer
while holding mdev->graph_mutex, and media_device_unregister_entity()
takes the same lock, so a reader either still sees a registered entity,
for which unregister waits, or sees NULL.  Clearing the pointer after
the unregister would leave a window in which the entity is read after
it was already freed.

Read the pointer only once, into a local variable, while the lock is
held.  dvb_media_device_free() stores NULL before taking graph_mutex,
so re-reading the field to pass it as the argument could still observe
NULL and hand it to the handler.  That store is not serialised with the
locked readers either, so publish the pointer with WRITE_ONCE() and
read it with READ_ONCE().

Stopping the pipeline the handler started is not done here.  Only the
driver knows how to release the link it owns, and dvb_media_device_free()
runs for every dvb_device, including the demux, dvr and ca entities that
never own a media link; calling the handler for those would deactivate a
link owned by another entity.  The au0828 driver stops the pipeline and
clears its saved link in au0828_usb_disconnect(), before the frontend
entity is unregistered, in a separate patch.

Fixes: 68ff5767ddc4 ("[media] media: dvb-frontend invoke enable/disable_source handlers")
Cc: stable@xxxxxxxxxxxxxxx
Co-developed-by: Bin Yu <byu@xxxxxxxxxxxxx>
Signed-off-by: Bin Yu <byu@xxxxxxxxxxxxx>
Co-developed-by: Cheng Wen <wencheng@xxxxxxxxxxxxx>
Signed-off-by: Cheng Wen <wencheng@xxxxxxxxxxxxx>
Signed-off-by: YuanZhe Liu <25031212351@xxxxxxxxxxxxxxxxx>
---
Changes in v3:
- Publish dvbdev->entity with WRITE_ONCE() and read it with READ_ONCE()
  at both frontend call sites, so the clear in dvb_media_device_free()
  is not a plain data race with the readers that hold graph_mutex.
- State why the active pipeline is torn down by the driver rather than
  in dvb-core.  The matching au0828 change is attached to this mail.

Link: https://lore.kernel.org/r/20261102142508.24277-1-25031212351@xxxxxxxxxxxxxxxxx

Changes in v2:
- Read dvbdev->entity once into a local variable under graph_mutex at both
  call sites.  dvb_media_device_free() clears the pointer without holding
  that lock, so re-reading the field for the call argument could still pass
  NULL to the handler.
---
 drivers/media/dvb-core/dvb_frontend.c | 14 ++++++++++----
 drivers/media/dvb-core/dvbdev.c       | 16 +++++++++++++---
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 0286da57f382..bcee88e16576 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -2826,9 +2826,12 @@ static int __dvb_frontend_open(struct inode *inode, struct file *file)
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB
 	mutex_lock(&fe->dvb->mdev_lock);
 	if (fe->dvb->mdev) {
+		struct media_entity *entity;
+
 		mutex_lock(&fe->dvb->mdev->graph_mutex);
-		if (fe->dvb->mdev->disable_source)
-			fe->dvb->mdev->disable_source(dvbdev->entity);
+		entity = READ_ONCE(dvbdev->entity);
+		if (entity && fe->dvb->mdev->disable_source)
+			fe->dvb->mdev->disable_source(entity);
 		mutex_unlock(&fe->dvb->mdev->graph_mutex);
 	}
 	mutex_unlock(&fe->dvb->mdev_lock);
@@ -2941,9 +2944,12 @@ static int dvb_frontend_release(struct inode *inode, struct file *file)
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB
 		mutex_lock(&fe->dvb->mdev_lock);
 		if (fe->dvb->mdev) {
+			struct media_entity *entity;
+
 			mutex_lock(&fe->dvb->mdev->graph_mutex);
-			if (fe->dvb->mdev->disable_source)
-				fe->dvb->mdev->disable_source(dvbdev->entity);
+			entity = READ_ONCE(dvbdev->entity);
+			if (entity && fe->dvb->mdev->disable_source)
+				fe->dvb->mdev->disable_source(entity);
 			mutex_unlock(&fe->dvb->mdev->graph_mutex);
 		}
 		mutex_unlock(&fe->dvb->mdev_lock);
diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index d753d329502a..bb6369c3abde 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -208,10 +208,20 @@ static void dvb_media_device_free(struct dvb_device *dvbdev)
 {
 #if defined(CONFIG_MEDIA_CONTROLLER_DVB)
 	if (dvbdev->entity) {
-		media_device_unregister_entity(dvbdev->entity);
-		kfree(dvbdev->entity);
+		struct media_entity *entity = dvbdev->entity;
+
+		/*
+		 * Clear the pointer before the entity is unregistered and
+		 * freed.  The frontend release and open paths read it under
+		 * mdev->graph_mutex and pass it to the disable_source
+		 * handler, so a stale entity must not stay reachable.  Publish
+		 * it with WRITE_ONCE() because the store is not serialised by
+		 * that lock.
+		 */
+		WRITE_ONCE(dvbdev->entity, NULL);
+		media_device_unregister_entity(entity);
+		kfree(entity);
 		kfree(dvbdev->pads);
-		dvbdev->entity = NULL;
 		dvbdev->pads = NULL;
 	}
 
-- 
2.34.1