[media] media: Protect enable_source and disable_source handler code paths

Drivers might try to access and run enable_source and disable_source
handlers when the driver that implements these handlers is clearing
the handlers during its unregister.

Fix the following race condition:

process 1				process 2

request video streaming			unbind au0828
v4l2 checks if tuner is free
...					...

					au0828_unregister_media_device()
...					...
					(doesn't hold graph_mutex)
					mdev->enable_source = NULL;
if (mdev && mdev->enable_source)	mdev->disable_source = NULL;
	mdev->enable_source()
(enable_source holds graph_mutex)

As shown above enable_source check is done without holding the graph_mutex.
If unbind happens to be in progress, au0828 could clear enable_source and
disable_source handlers leading to null pointer de-reference.

Fix it by protecting enable_source and disable_source set and clear and
protecting enable_source and disable_source handler access and the call
itself.

process 1				process 2

request video streaming			unbind au0828
v4l2 checks if tuner is free
...					...

					au0828_unregister_media_device()
...					...
					(hold graph_mutex while clearing)
					mdev->enable_source = NULL;
if (mdev)				mdev->disable_source = NULL;
(hold graph_mutex to check and
 call enable_source)
    if (mdev->enable_source)
	mdev->enable_source()

If graph_mutex is held to just heck for handler being null and needs to be
released before calling the handler, there will be another window for the
handlers to be cleared. Hence, enable_source and disable_source handlers
no longer hold the graph_mutex and expect callers to hold it to avoid
forcing them release the graph_mutex before calling the handlers.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 13dca47ea91e5f119fc16105432c928585b0eff5..85ae3669aa668baa656ed396ddb4913583e48777 100644 (file)
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -2533,9 +2533,13 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
                fepriv->voltage = -1;
 
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB
-               if (fe->dvb->mdev && fe->dvb->mdev->enable_source) {
-                       ret = fe->dvb->mdev->enable_source(dvbdev->entity,
+               if (fe->dvb->mdev) {
+                       mutex_lock(&fe->dvb->mdev->graph_mutex);
+                       if (fe->dvb->mdev->enable_source)
+                               ret = fe->dvb->mdev->enable_source(
+                                                          dvbdev->entity,
                                                           &fepriv->pipe);
+                       mutex_unlock(&fe->dvb->mdev->graph_mutex);
                        if (ret) {
                                dev_err(fe->dvb->device,
                                        "Tuner is busy. Error %d\n", ret);
@@ -2559,8 +2563,12 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
 
 err3:
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB
-       if (fe->dvb->mdev && fe->dvb->mdev->disable_source)
-               fe->dvb->mdev->disable_source(dvbdev->entity);
+       if (fe->dvb->mdev) {
+               mutex_lock(&fe->dvb->mdev->graph_mutex);
+               if (fe->dvb->mdev->disable_source)
+                       fe->dvb->mdev->disable_source(dvbdev->entity);
+               mutex_unlock(&fe->dvb->mdev->graph_mutex);
+       }
 err2:
 #endif
        dvb_generic_release(inode, file);
@@ -2592,8 +2600,12 @@ static int dvb_frontend_release(struct inode *inode, struct file *file)
        if (dvbdev->users == -1) {
                wake_up(&fepriv->wait_queue);
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB
-               if (fe->dvb->mdev && fe->dvb->mdev->disable_source)
-                       fe->dvb->mdev->disable_source(dvbdev->entity);
+               if (fe->dvb->mdev) {
+                       mutex_lock(&fe->dvb->mdev->graph_mutex);
+                       if (fe->dvb->mdev->disable_source)
+                               fe->dvb->mdev->disable_source(dvbdev->entity);
+                       mutex_unlock(&fe->dvb->mdev->graph_mutex);
+               }
 #endif
                if (fe->exit != DVB_FE_NO_EXIT)
                        wake_up(&dvbdev->wait_queue);

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
index 8c0494042d0681c0c9848c0c5468b36b21c3ad8f..739df61cec4f7049a838d27d6c80b4d7ec682040 100644 (file)
--- a/drivers/media/usb/au0828/au0828-core.c
+++ b/drivers/media/usb/au0828/au0828-core.c
@@ -149,9 +149,11 @@ static void au0828_unregister_media_device(struct au0828_dev *dev)
        }
 
        /* clear enable_source, disable_source */
+       mutex_lock(&mdev->graph_mutex);
        dev->media_dev->source_priv = NULL;
        dev->media_dev->enable_source = NULL;
        dev->media_dev->disable_source = NULL;
+       mutex_unlock(&mdev->graph_mutex);
 
        media_device_unregister(dev->media_dev);
        media_device_cleanup(dev->media_dev);
@@ -274,6 +276,7 @@ create_link:
        }
 }
 
+/* Callers should hold graph_mutex */
 static int au0828_enable_source(struct media_entity *entity,
                                struct media_pipeline *pipe)
 {
@@ -287,8 +290,6 @@ static int au0828_enable_source(struct media_entity *entity,
        if (!mdev)
                return -ENODEV;
 
-       mutex_lock(&mdev->graph_mutex);
-
        dev = mdev->source_priv;
 
        /*
@@ -415,12 +416,12 @@ static int au0828_enable_source(struct media_entity *entity,
                 dev->active_source->name, dev->active_sink->name,
                 dev->active_link_owner->name, ret);
 end:
-       mutex_unlock(&mdev->graph_mutex);
        pr_debug("au0828_enable_source() end %s %d %d\n",
                 entity->name, entity->function, ret);
        return ret;
 }
 
+/* Callers should hold graph_mutex */
 static void au0828_disable_source(struct media_entity *entity)
 {
        int ret = 0;
@@ -430,13 +431,10 @@ static void au0828_disable_source(struct media_entity *entity)
        if (!mdev)
                return;
 
-       mutex_lock(&mdev->graph_mutex);
        dev = mdev->source_priv;
 
-       if (!dev->active_link) {
-               ret = -ENODEV;
-               goto end;
-       }
+       if (!dev->active_link)
+               return;
 
        /* link is active - stop pipeline from source (tuner) */
        if (dev->active_link->sink->entity == dev->active_sink &&
@@ -446,7 +444,7 @@ static void au0828_disable_source(struct media_entity *entity)
                 * has active pipeline
                */
                if (dev->active_link_owner != entity)
-                       goto end;
+                       return;
                __media_pipeline_stop(entity);
                ret = __media_entity_setup_link(dev->active_link, 0);
                if (ret)
@@ -461,9 +459,6 @@ static void au0828_disable_source(struct media_entity *entity)
                dev->active_source = NULL;
                dev->active_sink = NULL;
        }
-
-end:
-       mutex_unlock(&mdev->graph_mutex);
 }
 #endif
 
@@ -545,9 +540,11 @@ static int au0828_media_device_register(struct au0828_dev *dev,
                return ret;
        }
        /* set enable_source */
+       mutex_lock(&dev->media_dev->graph_mutex);
        dev->media_dev->source_priv = (void *) dev;
        dev->media_dev->enable_source = au0828_enable_source;
        dev->media_dev->disable_source = au0828_disable_source;
+       mutex_unlock(&dev->media_dev->graph_mutex);
 #endif
        return 0;
 }

diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
index fcf614a82bb8a609873a891500ea2542c41e5225..303980b71aae26f2a750c2f1bfe96ebe1046148e 100644 (file)
--- a/drivers/media/v4l2-core/v4l2-mc.c
+++ b/drivers/media/v4l2-core/v4l2-mc.c
@@ -198,14 +198,20 @@ EXPORT_SYMBOL_GPL(v4l2_mc_create_media_graph);
 int v4l_enable_media_source(struct video_device *vdev)
 {
        struct media_device *mdev = vdev->entity.graph_obj.mdev;
-       int ret;
+       int ret = 0, err;
 
-       if (!mdev || !mdev->enable_source)
+       if (!mdev)
                return 0;
-       ret = mdev->enable_source(&vdev->entity, &vdev->pipe);
-       if (ret)
-               return -EBUSY;
-       return 0;
+
+       mutex_lock(&mdev->graph_mutex);
+       if (!mdev->enable_source)
+               goto end;
+       err = mdev->enable_source(&vdev->entity, &vdev->pipe);
+       if (err)
+               ret = -EBUSY;
+end:
+       mutex_unlock(&mdev->graph_mutex);
+       return ret;
 }
 EXPORT_SYMBOL_GPL(v4l_enable_media_source);
 
@@ -213,8 +219,12 @@ void v4l_disable_media_source(struct video_device *vdev)
 {
        struct media_device *mdev = vdev->entity.graph_obj.mdev;
 
-       if (mdev && mdev->disable_source)
-               mdev->disable_source(&vdev->entity);
+       if (mdev) {
+               mutex_lock(&mdev->graph_mutex);
+               if (mdev->disable_source)
+                       mdev->disable_source(&vdev->entity);
+               mutex_unlock(&mdev->graph_mutex);
+       }
 }
 EXPORT_SYMBOL_GPL(v4l_disable_media_source);
 

diff --git a/include/media/media-device.h b/include/media/media-device.h
index ae4eef416d70a6145ca5f2ebd50b26ad1e0ce5b4..6896266031b9905ff6ba61eb0706e5550e85f616 100644 (file)
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -121,6 +121,8 @@ struct media_device_ops {
  *    bridge driver finds the media_device during probe.
  *    Bridge driver sets source_priv with information
  *    necessary to run @enable_source and @disable_source handlers.
+ *    Callers should hold graph_mutex to access and call @enable_source
+ *    and @disable_source handlers.
  */
 struct media_device {
        /* dev->driver_data points to this struct. */