Skip to content

Commit 90cd366

Browse files
Shuah Khanmchehab
Shuah Khan
authored and
mchehab
committed
[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 <[email protected]> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
1 parent 92fbeb4 commit 90cd366

File tree

4 files changed

+47
-26
lines changed

4 files changed

+47
-26
lines changed

‎drivers/media/dvb-core/dvb_frontend.c‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2533,9 +2533,13 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
25332533
fepriv->voltage = -1;
25342534

25352535
#ifdef CONFIG_MEDIA_CONTROLLER_DVB
2536-
if (fe->dvb->mdev && fe->dvb->mdev->enable_source) {
2537-
ret = fe->dvb->mdev->enable_source(dvbdev->entity,
2536+
if (fe->dvb->mdev) {
2537+
mutex_lock(&fe->dvb->mdev->graph_mutex);
2538+
if (fe->dvb->mdev->enable_source)
2539+
ret = fe->dvb->mdev->enable_source(
2540+
dvbdev->entity,
25382541
&fepriv->pipe);
2542+
mutex_unlock(&fe->dvb->mdev->graph_mutex);
25392543
if (ret) {
25402544
dev_err(fe->dvb->device,
25412545
"Tuner is busy. Error %d\n", ret);
@@ -2559,8 +2563,12 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
25592563

25602564
err3:
25612565
#ifdef CONFIG_MEDIA_CONTROLLER_DVB
2562-
if (fe->dvb->mdev && fe->dvb->mdev->disable_source)
2563-
fe->dvb->mdev->disable_source(dvbdev->entity);
2566+
if (fe->dvb->mdev) {
2567+
mutex_lock(&fe->dvb->mdev->graph_mutex);
2568+
if (fe->dvb->mdev->disable_source)
2569+
fe->dvb->mdev->disable_source(dvbdev->entity);
2570+
mutex_unlock(&fe->dvb->mdev->graph_mutex);
2571+
}
25642572
err2:
25652573
#endif
25662574
dvb_generic_release(inode, file);
@@ -2592,8 +2600,12 @@ static int dvb_frontend_release(struct inode *inode, struct file *file)
25922600
if (dvbdev->users == -1) {
25932601
wake_up(&fepriv->wait_queue);
25942602
#ifdef CONFIG_MEDIA_CONTROLLER_DVB
2595-
if (fe->dvb->mdev && fe->dvb->mdev->disable_source)
2596-
fe->dvb->mdev->disable_source(dvbdev->entity);
2603+
if (fe->dvb->mdev) {
2604+
mutex_lock(&fe->dvb->mdev->graph_mutex);
2605+
if (fe->dvb->mdev->disable_source)
2606+
fe->dvb->mdev->disable_source(dvbdev->entity);
2607+
mutex_unlock(&fe->dvb->mdev->graph_mutex);
2608+
}
25972609
#endif
25982610
if (fe->exit != DVB_FE_NO_EXIT)
25992611
wake_up(&dvbdev->wait_queue);

‎drivers/media/usb/au0828/au0828-core.c‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -149,9 +149,11 @@ static void au0828_unregister_media_device(struct au0828_dev *dev)
149149
}
150150

151151
/* clear enable_source, disable_source */
152+
mutex_lock(&mdev->graph_mutex);
152153
dev->media_dev->source_priv = NULL;
153154
dev->media_dev->enable_source = NULL;
154155
dev->media_dev->disable_source = NULL;
156+
mutex_unlock(&mdev->graph_mutex);
155157

156158
media_device_unregister(dev->media_dev);
157159
media_device_cleanup(dev->media_dev);
@@ -274,6 +276,7 @@ static void au0828_media_graph_notify(struct media_entity *new,
274276
}
275277
}
276278

279+
/* Callers should hold graph_mutex */
277280
static int au0828_enable_source(struct media_entity *entity,
278281
struct media_pipeline *pipe)
279282
{
@@ -287,8 +290,6 @@ static int au0828_enable_source(struct media_entity *entity,
287290
if (!mdev)
288291
return -ENODEV;
289292

290-
mutex_lock(&mdev->graph_mutex);
291-
292293
dev = mdev->source_priv;
293294

294295
/*
@@ -415,12 +416,12 @@ static int au0828_enable_source(struct media_entity *entity,
415416
dev->active_source->name, dev->active_sink->name,
416417
dev->active_link_owner->name, ret);
417418
end:
418-
mutex_unlock(&mdev->graph_mutex);
419419
pr_debug("au0828_enable_source() end %s %d %d\n",
420420
entity->name, entity->function, ret);
421421
return ret;
422422
}
423423

424+
/* Callers should hold graph_mutex */
424425
static void au0828_disable_source(struct media_entity *entity)
425426
{
426427
int ret = 0;
@@ -430,13 +431,10 @@ static void au0828_disable_source(struct media_entity *entity)
430431
if (!mdev)
431432
return;
432433

433-
mutex_lock(&mdev->graph_mutex);
434434
dev = mdev->source_priv;
435435

436-
if (!dev->active_link) {
437-
ret = -ENODEV;
438-
goto end;
439-
}
436+
if (!dev->active_link)
437+
return;
440438

441439
/* link is active - stop pipeline from source (tuner) */
442440
if (dev->active_link->sink->entity == dev->active_sink &&
@@ -446,7 +444,7 @@ static void au0828_disable_source(struct media_entity *entity)
446444
* has active pipeline
447445
*/
448446
if (dev->active_link_owner != entity)
449-
goto end;
447+
return;
450448
__media_pipeline_stop(entity);
451449
ret = __media_entity_setup_link(dev->active_link, 0);
452450
if (ret)
@@ -461,9 +459,6 @@ static void au0828_disable_source(struct media_entity *entity)
461459
dev->active_source = NULL;
462460
dev->active_sink = NULL;
463461
}
464-
465-
end:
466-
mutex_unlock(&mdev->graph_mutex);
467462
}
468463
#endif
469464

@@ -545,9 +540,11 @@ static int au0828_media_device_register(struct au0828_dev *dev,
545540
return ret;
546541
}
547542
/* set enable_source */
543+
mutex_lock(&dev->media_dev->graph_mutex);
548544
dev->media_dev->source_priv = (void *) dev;
549545
dev->media_dev->enable_source = au0828_enable_source;
550546
dev->media_dev->disable_source = au0828_disable_source;
547+
mutex_unlock(&dev->media_dev->graph_mutex);
551548
#endif
552549
return 0;
553550
}

‎drivers/media/v4l2-core/v4l2-mc.c‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -198,23 +198,33 @@ EXPORT_SYMBOL_GPL(v4l2_mc_create_media_graph);
198198
int v4l_enable_media_source(struct video_device *vdev)
199199
{
200200
struct media_device *mdev = vdev->entity.graph_obj.mdev;
201-
int ret;
201+
int ret = 0, err;
202202

203-
if (!mdev || !mdev->enable_source)
203+
if (!mdev)
204204
return 0;
205-
ret = mdev->enable_source(&vdev->entity, &vdev->pipe);
206-
if (ret)
207-
return -EBUSY;
208-
return 0;
205+
206+
mutex_lock(&mdev->graph_mutex);
207+
if (!mdev->enable_source)
208+
goto end;
209+
err = mdev->enable_source(&vdev->entity, &vdev->pipe);
210+
if (err)
211+
ret = -EBUSY;
212+
end:
213+
mutex_unlock(&mdev->graph_mutex);
214+
return ret;
209215
}
210216
EXPORT_SYMBOL_GPL(v4l_enable_media_source);
211217

212218
void v4l_disable_media_source(struct video_device *vdev)
213219
{
214220
struct media_device *mdev = vdev->entity.graph_obj.mdev;
215221

216-
if (mdev && mdev->disable_source)
217-
mdev->disable_source(&vdev->entity);
222+
if (mdev) {
223+
mutex_lock(&mdev->graph_mutex);
224+
if (mdev->disable_source)
225+
mdev->disable_source(&vdev->entity);
226+
mutex_unlock(&mdev->graph_mutex);
227+
}
218228
}
219229
EXPORT_SYMBOL_GPL(v4l_disable_media_source);
220230

‎include/media/media-device.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,8 @@ struct media_device_ops {
121121
* bridge driver finds the media_device during probe.
122122
* Bridge driver sets source_priv with information
123123
* necessary to run @enable_source and @disable_source handlers.
124+
* Callers should hold graph_mutex to access and call @enable_source
125+
* and @disable_source handlers.
124126
*/
125127
struct media_device {
126128
/* dev->driver_data points to this struct. */

0 commit comments

Comments
 (0)