dvb_unregister_device() is known that prone to use-after-free.
That is, the cleanup from dvb_unregister_device() releases the dvb_device
even if there are pointers stored in file->private_data still refer to it.
This patch adds a reference counter into struct dvb_device and delays its
deallocation until no pointer refers to the object.
Signed-off-by: Lin Ma <[email protected]>
---
drivers/media/dvb-core/dvb_ca_en50221.c | 2 +-
drivers/media/dvb-core/dvb_frontend.c | 2 +-
drivers/media/dvb-core/dvbdev.c | 32 +++++++++++++++++++------
include/media/dvbdev.h | 31 +++++++++++++-----------
4 files changed, 44 insertions(+), 23 deletions(-)
diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c b/drivers/media/dvb-core/dvb_ca_en50221.c
index 15a08d8c69ef..c2d2792227f8 100644
--- a/drivers/media/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb-core/dvb_ca_en50221.c
@@ -157,7 +157,7 @@ static void dvb_ca_private_free(struct dvb_ca_private *ca)
{
unsigned int i;
- dvb_free_device(ca->dvbdev);
+ dvb_device_put(ca->dvbdev);
for (i = 0; i < ca->slot_count; i++)
vfree(ca->slot_info[i].rx_buffer.data);
diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index 48e735cdbe6b..c376cd954e5e 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -136,7 +136,7 @@ static void __dvb_frontend_free(struct dvb_frontend *fe)
struct dvb_frontend_private *fepriv = fe->frontend_priv;
if (fepriv)
- dvb_free_device(fepriv->dvbdev);
+ dvb_device_put(fepriv->dvbdev);
dvb_frontend_invoke_release(fe, fe->ops.release);
diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index 675d877a67b2..14f0e140d541 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -97,7 +97,7 @@ static int dvb_device_open(struct inode *inode, struct file *file)
new_fops = fops_get(dvbdev->fops);
if (!new_fops)
goto fail;
- file->private_data = dvbdev;
+ file->private_data = dvb_device_get(dvbdev);
replace_fops(file, new_fops);
if (file->f_op->open)
err = file->f_op->open(inode, file);
@@ -161,6 +161,9 @@ int dvb_generic_release(struct inode *inode, struct file *file)
}
dvbdev->users++;
+
+ dvb_device_put(dvbdev);
+
return 0;
}
EXPORT_SYMBOL(dvb_generic_release);
@@ -477,6 +480,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
return -ENOMEM;
}
+ kref_init(&dvbdev->ref);
memcpy(dvbdev, template, sizeof(struct dvb_device));
dvbdev->type = type;
dvbdev->id = id;
@@ -508,7 +512,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev,
#endif
dvbdev->minor = minor;
- dvb_minors[minor] = dvbdev;
+ dvb_minors[minor] = dvb_device_get(dvbdev);
up_write(&minor_rwsem);
ret = dvb_register_media_device(dvbdev, type, minor, demux_sink_pads);
@@ -553,6 +557,7 @@ void dvb_remove_device(struct dvb_device *dvbdev)
down_write(&minor_rwsem);
dvb_minors[dvbdev->minor] = NULL;
+ dvb_device_put(dvbdev);
up_write(&minor_rwsem);
dvb_media_device_free(dvbdev);
@@ -564,21 +569,34 @@ void dvb_remove_device(struct dvb_device *dvbdev)
EXPORT_SYMBOL(dvb_remove_device);
-void dvb_free_device(struct dvb_device *dvbdev)
+static void dvb_free_device(struct kref *ref)
{
- if (!dvbdev)
- return;
+ struct dvb_device *dvbdev = container_of(ref, struct dvb_device, ref);
kfree (dvbdev->fops);
kfree (dvbdev);
}
-EXPORT_SYMBOL(dvb_free_device);
+
+
+struct dvb_device *dvb_device_get(struct dvb_device *dvbdev)
+{
+ kref_get(&dvbdev->ref);
+ return dvbdev;
+}
+EXPORT_SYMBOL(dvb_device_get);
+
+
+void dvb_device_put(struct dvb_device *dvbdev)
+{
+ if (dvbdev)
+ kref_put(&dvbdev->ref, dvb_free_device);
+}
void dvb_unregister_device(struct dvb_device *dvbdev)
{
dvb_remove_device(dvbdev);
- dvb_free_device(dvbdev);
+ dvb_device_put(dvbdev);
}
EXPORT_SYMBOL(dvb_unregister_device);
diff --git a/include/media/dvbdev.h b/include/media/dvbdev.h
index 2f6b0861322a..149b3d33c24b 100644
--- a/include/media/dvbdev.h
+++ b/include/media/dvbdev.h
@@ -156,6 +156,7 @@ struct dvb_adapter {
*/
struct dvb_device {
struct list_head list_head;
+ struct kref ref;
const struct file_operations *fops;
struct dvb_adapter *adapter;
enum dvb_device_type type;
@@ -187,6 +188,20 @@ struct dvb_device {
void *priv;
};
+/**
+ * dvb_device_get - Increase dvb_device reference
+ *
+ * @dvbdev: pointer to struct dvb_device
+ */
+struct dvb_device *dvb_device_get(struct dvb_device *dvbdev);
+
+/**
+ * dvb_device_get - Decrease dvb_device reference
+ *
+ * @dvbdev: pointer to struct dvb_device
+ */
+void dvb_device_put(struct dvb_device *dvbdev);
+
/**
* dvb_register_adapter - Registers a new DVB adapter
*
@@ -231,29 +246,17 @@ int dvb_register_device(struct dvb_adapter *adap,
/**
* dvb_remove_device - Remove a registered DVB device
*
- * This does not free memory. To do that, call dvb_free_device().
+ * This does not free memory. dvb_free_device() will do that when
+ * reference counter is empty
*
* @dvbdev: pointer to struct dvb_device
*/
void dvb_remove_device(struct dvb_device *dvbdev);
-/**
- * dvb_free_device - Free memory occupied by a DVB device.
- *
- * Call dvb_unregister_device() before calling this function.
- *
- * @dvbdev: pointer to struct dvb_device
- */
-void dvb_free_device(struct dvb_device *dvbdev);
/**
* dvb_unregister_device - Unregisters a DVB device
*
- * This is a combination of dvb_remove_device() and dvb_free_device().
- * Using this function is usually a mistake, and is often an indicator
- * for a use-after-free bug (when a userspace process keeps a file
- * handle to a detached device).
- *
* @dvbdev: pointer to struct dvb_device
*/
void dvb_unregister_device(struct dvb_device *dvbdev);
--
2.36.1
Hi Lin,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on media-tree/master]
[also build test WARNING on linus/master v5.19 next-20220805]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Lin-Ma/media-dvbdev-adopts-refcnt-to-avoid-UAF/20220807-230449
base: git://linuxtv.org/media_tree.git master
reproduce: make htmldocs
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> include/media/dvbdev.h:203: warning: expecting prototype for dvb_device_get(). Prototype was for dvb_device_put() instead
vim +203 include/media/dvbdev.h
187
188 void *priv;
> 189 };
190
191 /**
192 * dvb_device_get - Increase dvb_device reference
193 *
194 * @dvbdev: pointer to struct dvb_device
195 */
196 struct dvb_device *dvb_device_get(struct dvb_device *dvbdev);
197
198 /**
199 * dvb_device_get - Decrease dvb_device reference
200 *
201 * @dvbdev: pointer to struct dvb_device
202 */
> 203 void dvb_device_put(struct dvb_device *dvbdev);
204
--
0-DAY CI Kernel Test Service
https://01.org/lkp