[v2] media: dvb-core: fix use-after-free in dvb_frontend_open()

[PATCH v2] media: dvb-core: fix use-after-free in dvb_frontend_open()

Posted by Yun Zhou 3 weeks, 1 day ago

dvb_frontend_open() calls dvb_generic_release() in its error path after
dvb_generic_open() succeeds. dvb_generic_release() drops the device
reference via dvb_device_put(), and then dvb_device_open() drops it again
in its error handling, causing a use-after-free and refcount underflow.

Fix this by introducing __dvb_generic_release() which only restores the
users/readers/writers counters without dropping the device reference. Use
it in dvb_frontend_open()'s error path so that dvb_device_open() remains
the sole owner of the dvb_device_put() on open failure.

Reported-by: syzbot+40339ea82afa8184ad5d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=40339ea82afa8184ad5d
Cc: stable@vger.kernel.org
Fixes: 0fc044b2b5e2 ("media: dvbdev: adopts refcnt to avoid UAF")
Signed-off-by: Yun Zhou <yun.zhou@windriver.com>
---
v2:
 - Fix Fixes tag commit title
 - Add Closes: link after Reported-by
 - Cc stable@vger.kernel.org

 drivers/media/dvb-core/dvb_frontend.c |  2 +-
 drivers/media/dvb-core/dvbdev.c       | 17 ++++++++++++-----
 include/media/dvbdev.h                | 12 ++++++++++++
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
index d082b6c57c76..497f5920b267 100644
--- a/drivers/media/dvb-core/dvb_frontend.c
+++ b/drivers/media/dvb-core/dvb_frontend.c
@@ -2887,7 +2887,7 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
 	mutex_unlock(&fe->dvb->mdev_lock);
 err2:
 #endif
-	dvb_generic_release(inode, file);
+	__dvb_generic_release(inode, file);
 err1:
 	if (dvbdev->users == -1 && fe->ops.ts_bus_ctrl)
 		fe->ops.ts_bus_ctrl(fe, 0);
diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index d753d329502a..3e0ad67b79a3 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -152,19 +152,26 @@ int dvb_generic_open(struct inode *inode, struct file *file)
 }
 EXPORT_SYMBOL(dvb_generic_open);
 
-int dvb_generic_release(struct inode *inode, struct file *file)
+void __dvb_generic_release(struct inode *inode, struct file *file)
 {
 	struct dvb_device *dvbdev = file->private_data;
 
-	if (!dvbdev)
-		return -ENODEV;
-
 	if ((file->f_flags & O_ACCMODE) == O_RDONLY)
 		dvbdev->readers++;
 	else
 		dvbdev->writers++;
-
 	dvbdev->users++;
+}
+EXPORT_SYMBOL(__dvb_generic_release);
+
+int dvb_generic_release(struct inode *inode, struct file *file)
+{
+	struct dvb_device *dvbdev = file->private_data;
+
+	if (!dvbdev)
+		return -ENODEV;
+
+	__dvb_generic_release(inode, file);
 
 	dvb_device_put(dvbdev);
 
diff --git a/include/media/dvbdev.h b/include/media/dvbdev.h
index e5a00d126612..9e6e5cb43dcb 100644
--- a/include/media/dvbdev.h
+++ b/include/media/dvbdev.h
@@ -343,6 +343,18 @@ int dvb_create_media_graph(struct dvb_adapter *adap,
  */
 int dvb_generic_open(struct inode *inode, struct file *file);
 
+/*
+ * __dvb_generic_release - Undo dvb_generic_open() counters WITHOUT
+ *      dropping the device reference.
+ *
+ * @inode: pointer to &struct inode.
+ * @file: pointer to &struct file.
+ *
+ * Used in cases where the caller handles dvb_device_put() and ensures
+ * that dvbdev is valid.
+ */
+void __dvb_generic_release(struct inode *inode, struct file *file);
+
 /**
  * dvb_generic_release - Digital TV close function, used by DVB devices
  *
-- 
2.43.0

Re: [PATCH v2] media: dvb-core: fix use-after-free in dvb_frontend_open()

Posted by Hillf Danton 3 weeks ago

On Tue, 19 May 2026 13:10:20 +0800 Yun Zhou wrote:
> dvb_frontend_open() calls dvb_generic_release() in its error path after
> dvb_generic_open() succeeds. dvb_generic_release() drops the device
> reference via dvb_device_put(), and then dvb_device_open() drops it again
> in its error handling, causing a use-after-free and refcount underflow.
> 
> Fix this by introducing __dvb_generic_release() which only restores the
> users/readers/writers counters without dropping the device reference. Use
> it in dvb_frontend_open()'s error path so that dvb_device_open() remains
> the sole owner of the dvb_device_put() on open failure.
> 
Given the relevant code snippet

	dvb_device_open()
	  mutex_lock(&dvbdev_mutex);
	  down_read(&minor_rwsem);
	  dvbdev = dvb_minors[minor];
	  if (dvbdev && dvbdev->fops) {
	    dvb_device_get(dvbdev);
	    err = file->f_op->open(inode, file);
	    up_read(&minor_rwsem);
	    mutex_unlock(&dvbdev_mutex);
	    if (err)
	       dvb_device_put(dvbdev);
	  }

a) the frontend open callback is invoked with refcount incremented, so
why could a single put in the err path drop refcount to ground?
b) worse dvbdev is freed without clearing dvb_minors[minor].

One explanation sounds like

	dvb_device_open(); // err with refcount dropped but
			   // without clearing dvb_minors[minor]
	dvb_device_open(); // single put frees dvbdev

so a simpler fix looks like incrementing refcount before
dvb_generic_release() in the err path.

> Reported-by: syzbot+40339ea82afa8184ad5d@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=40339ea82afa8184ad5d
> Cc: stable@vger.kernel.org
> Fixes: 0fc044b2b5e2 ("media: dvbdev: adopts refcnt to avoid UAF")
> Signed-off-by: Yun Zhou <yun.zhou@windriver.com>
> ---
> v2:
>  - Fix Fixes tag commit title
>  - Add Closes: link after Reported-by
>  - Cc stable@vger.kernel.org
> 
>  drivers/media/dvb-core/dvb_frontend.c |  2 +-
>  drivers/media/dvb-core/dvbdev.c       | 17 ++++++++++++-----
>  include/media/dvbdev.h                | 12 ++++++++++++
>  3 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
> index d082b6c57c76..497f5920b267 100644
> --- a/drivers/media/dvb-core/dvb_frontend.c
> +++ b/drivers/media/dvb-core/dvb_frontend.c
> @@ -2887,7 +2887,7 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
>  	mutex_unlock(&fe->dvb->mdev_lock);
>  err2:
>  #endif
> -	dvb_generic_release(inode, file);
> +	__dvb_generic_release(inode, file);
>  err1:
>  	if (dvbdev->users == -1 && fe->ops.ts_bus_ctrl)
>  		fe->ops.ts_bus_ctrl(fe, 0);
> diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
> index d753d329502a..3e0ad67b79a3 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -152,19 +152,26 @@ int dvb_generic_open(struct inode *inode, struct file *file)
>  }
>  EXPORT_SYMBOL(dvb_generic_open);
>  
> -int dvb_generic_release(struct inode *inode, struct file *file)
> +void __dvb_generic_release(struct inode *inode, struct file *file)
>  {
>  	struct dvb_device *dvbdev = file->private_data;
>  
> -	if (!dvbdev)
> -		return -ENODEV;
> -
>  	if ((file->f_flags & O_ACCMODE) == O_RDONLY)
>  		dvbdev->readers++;
>  	else
>  		dvbdev->writers++;
> -
>  	dvbdev->users++;
> +}
> +EXPORT_SYMBOL(__dvb_generic_release);
> +
> +int dvb_generic_release(struct inode *inode, struct file *file)
> +{
> +	struct dvb_device *dvbdev = file->private_data;
> +
> +	if (!dvbdev)
> +		return -ENODEV;
> +
> +	__dvb_generic_release(inode, file);
>  
>  	dvb_device_put(dvbdev);
>  
> diff --git a/include/media/dvbdev.h b/include/media/dvbdev.h
> index e5a00d126612..9e6e5cb43dcb 100644
> --- a/include/media/dvbdev.h
> +++ b/include/media/dvbdev.h
> @@ -343,6 +343,18 @@ int dvb_create_media_graph(struct dvb_adapter *adap,
>   */
>  int dvb_generic_open(struct inode *inode, struct file *file);
>  
> +/*
> + * __dvb_generic_release - Undo dvb_generic_open() counters WITHOUT
> + *      dropping the device reference.
> + *
> + * @inode: pointer to &struct inode.
> + * @file: pointer to &struct file.
> + *
> + * Used in cases where the caller handles dvb_device_put() and ensures
> + * that dvbdev is valid.
> + */
> +void __dvb_generic_release(struct inode *inode, struct file *file);
> +
>  /**
>   * dvb_generic_release - Digital TV close function, used by DVB devices
>   *
> -- 
> 2.43.0

Re: [PATCH v2] media: dvb-core: fix use-after-free in dvb_frontend_open()

Posted by Zhou, Yun 2 weeks, 5 days ago

On 5/20/26 05:15, Hillf Danton wrote:
> Given the relevant code snippet
>
>          dvb_device_open()
>            mutex_lock(&dvbdev_mutex);
>            down_read(&minor_rwsem);
>            dvbdev = dvb_minors[minor];
>            if (dvbdev && dvbdev->fops) {
>              dvb_device_get(dvbdev);
>              err = file->f_op->open(inode, file);
>              up_read(&minor_rwsem);
>              mutex_unlock(&dvbdev_mutex);
>              if (err)
>                 dvb_device_put(dvbdev);
>            }
>
> a) the frontend open callback is invoked with refcount incremented, so
> why could a single put in the err path drop refcount to ground?
> b) worse dvbdev is freed without clearing dvb_minors[minor].
>
> One explanation sounds like
>
>          dvb_device_open(); // err with refcount dropped but
>                             // without clearing dvb_minors[minor]
>          dvb_device_open(); // single put frees dvbdev
>
> so a simpler fix looks like incrementing refcount before
> dvb_generic_release() in the err path.
Yes, this is a simpler way for the current issue. But dvb_device_get()  
before
dvb_generic_release() always feels odd and easily cause confusion for 
readers.
The most elegant way is to pair open() and release(), get() and put() in 
the same
context. To achieve this goal, the changes may be significant. However, 
I will
follow your suggestion to submit a new patch.