From 6868fb5df0ac873fce648e1bd23389b19849329d Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Tue, 31 Oct 2023 12:53:33 +0300 Subject: [PATCH 01/11] media: dvbdev: drop refcount on error path in dvb_device_open() If call to file->f_op->open() fails, then call dvb_device_put(dvbdev). Fixes: 0fc044b2b5e2 ("media: dvbdev: adopts refcnt to avoid UAF") Signed-off-by: Dan Carpenter Signed-off-by: Hans Verkuil --- drivers/media/dvb-core/dvbdev.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c index 305bb21d843c..49f0eb7d0b9d 100644 --- a/drivers/media/dvb-core/dvbdev.c +++ b/drivers/media/dvb-core/dvbdev.c @@ -104,6 +104,8 @@ static int dvb_device_open(struct inode *inode, struct file *file) err = file->f_op->open(inode, file); up_read(&minor_rwsem); mutex_unlock(&dvbdev_mutex); + if (err) + dvb_device_put(dvbdev); return err; } fail: From dcd0b3c0f8a77c47716dbf8d7f8d6a83095c3a95 Mon Sep 17 00:00:00 2001 From: Zhipeng Lu Date: Sat, 3 Feb 2024 14:40:43 +0100 Subject: [PATCH 02/11] media: edia: dvbdev: fix a use-after-free In dvb_register_device, *pdvbdev is set equal to dvbdev, which is freed in several error-handling paths. However, *pdvbdev is not set to NULL after dvbdev's deallocation, causing use-after-frees in many places, for example, in the following call chain: budget_register |-> dvb_dmxdev_init |-> dvb_register_device |-> dvb_dmxdev_release |-> dvb_unregister_device |-> dvb_remove_device |-> dvb_device_put |-> kref_put When calling dvb_unregister_device, dmxdev->dvbdev (i.e. *pdvbdev in dvb_register_device) could point to memory that had been freed in dvb_register_device. Thereafter, this pointer is transferred to kref_put and triggering a use-after-free. Link: https://lore.kernel.org/linux-media/20240203134046.3120099-1-alexious@zju.edu.cn Fixes: b61901024776 ("V4L/DVB (5244): Dvbdev: fix illegal re-usage of fileoperations struct") Signed-off-by: Zhipeng Lu Signed-off-by: Mauro Carvalho Chehab --- drivers/media/dvb-core/dvbdev.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c index 49f0eb7d0b9d..733d0bc4b4cc 100644 --- a/drivers/media/dvb-core/dvbdev.c +++ b/drivers/media/dvb-core/dvbdev.c @@ -490,6 +490,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev, dvbdevfops = kmemdup(template->fops, sizeof(*dvbdevfops), GFP_KERNEL); if (!dvbdevfops) { kfree(dvbdev); + *pdvbdev = NULL; mutex_unlock(&dvbdev_register_lock); return -ENOMEM; } @@ -498,6 +499,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev, if (!new_node) { kfree(dvbdevfops); kfree(dvbdev); + *pdvbdev = NULL; mutex_unlock(&dvbdev_register_lock); return -ENOMEM; } @@ -531,6 +533,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev, } list_del(&dvbdev->list_head); kfree(dvbdev); + *pdvbdev = NULL; up_write(&minor_rwsem); mutex_unlock(&dvbdev_register_lock); return -EINVAL; @@ -553,6 +556,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev, dvb_media_device_free(dvbdev); list_del(&dvbdev->list_head); kfree(dvbdev); + *pdvbdev = NULL; mutex_unlock(&dvbdev_register_lock); return ret; } @@ -571,6 +575,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev, dvb_media_device_free(dvbdev); list_del(&dvbdev->list_head); kfree(dvbdev); + *pdvbdev = NULL; mutex_unlock(&dvbdev_register_lock); return PTR_ERR(clsdev); } From 1b4240d3167bda368b283c40eae176e4159da7ae Mon Sep 17 00:00:00 2001 From: Ricardo Ribalda Date: Mon, 25 Mar 2024 14:50:25 +0000 Subject: [PATCH 03/11] media: dvbdev: Initialize sbuf MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because the size passed to copy_from_user() cannot be known beforehand, it needs to be checked during runtime with check_object_size. That makes gcc believe that the content of sbuf can be used before init. Fix: ./include/linux/thread_info.h:215:17: warning: ‘sbuf’ may be used uninitialized [-Wmaybe-uninitialized] Signed-off-by: Ricardo Ribalda Signed-off-by: Hans Verkuil --- drivers/media/dvb-core/dvbdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c index 733d0bc4b4cc..b43695bc51e7 100644 --- a/drivers/media/dvb-core/dvbdev.c +++ b/drivers/media/dvb-core/dvbdev.c @@ -956,7 +956,7 @@ int dvb_usercopy(struct file *file, int (*func)(struct file *file, unsigned int cmd, void *arg)) { - char sbuf[128]; + char sbuf[128] = {}; void *mbuf = NULL; void *parg = NULL; int err = -EINVAL; From 53e6d7adf45013e6655ed275d5ff9e4c620144d7 Mon Sep 17 00:00:00 2001 From: Mauro Carvalho Chehab Date: Tue, 15 Oct 2024 15:23:01 +0200 Subject: [PATCH 04/11] media: dvbdev: prevent the risk of out of memory access The dvbdev contains a static variable used to store dvb minors. The behavior of it depends if CONFIG_DVB_DYNAMIC_MINORS is set or not. When not set, dvb_register_device() won't check for boundaries, as it will rely that a previous call to dvb_register_adapter() would already be enforcing it. On a similar way, dvb_device_open() uses the assumption that the register functions already did the needed checks. This can be fragile if some device ends using different calls. This also generate warnings on static check analysers like Coverity. So, add explicit guards to prevent potential risk of OOM issues. Fixes: 5dd3f3071070 ("V4L/DVB (9361): Dynamic DVB minor allocation") Signed-off-by: Mauro Carvalho Chehab --- drivers/media/dvb-core/dvbdev.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c index b43695bc51e7..14f323fbada7 100644 --- a/drivers/media/dvb-core/dvbdev.c +++ b/drivers/media/dvb-core/dvbdev.c @@ -86,10 +86,15 @@ static DECLARE_RWSEM(minor_rwsem); static int dvb_device_open(struct inode *inode, struct file *file) { struct dvb_device *dvbdev; + unsigned int minor = iminor(inode); + + if (minor >= MAX_DVB_MINORS) + return -ENODEV; mutex_lock(&dvbdev_mutex); down_read(&minor_rwsem); - dvbdev = dvb_minors[iminor(inode)]; + + dvbdev = dvb_minors[minor]; if (dvbdev && dvbdev->fops) { int err = 0; @@ -525,7 +530,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev, for (minor = 0; minor < MAX_DVB_MINORS; minor++) if (!dvb_minors[minor]) break; - if (minor == MAX_DVB_MINORS) { + if (minor >= MAX_DVB_MINORS) { if (new_node) { list_del(&new_node->list_head); kfree(dvbdevfops); @@ -540,6 +545,14 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev, } #else minor = nums2minor(adap->num, type, id); + if (minor >= MAX_DVB_MINORS) { + dvb_media_device_free(dvbdev); + list_del(&dvbdev->list_head); + kfree(dvbdev); + *pdvbdev = NULL; + mutex_unlock(&dvbdev_register_lock); + return ret; + } #endif dvbdev->minor = minor; dvb_minors[minor] = dvb_device_get(dvbdev); From 8eefe2a716a7e8b4afe6bd88506456a4dcd70885 Mon Sep 17 00:00:00 2001 From: Mauro Carvalho Chehab Date: Wed, 6 Nov 2024 21:50:55 +0100 Subject: [PATCH 05/11] media: dvbdev: fix the logic when DVB_DYNAMIC_MINORS is not set When CONFIG_DVB_DYNAMIC_MINORS, ret is not initialized, and a semaphore is left at the wrong state, in case of errors. Make the code simpler and avoid mistakes by having just one error check logic used weather DVB_DYNAMIC_MINORS is used or not. Reported-by: kernel test robot Reported-by: Dan Carpenter Closes: https://lore.kernel.org/r/202410201717.ULWWdJv8-lkp@intel.com/ Signed-off-by: Mauro Carvalho Chehab Link: https://lore.kernel.org/r/9e067488d8935b8cf00959764a1fa5de85d65725.1730926254.git.mchehab+huawei@kernel.org --- drivers/media/dvb-core/dvbdev.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c index 14f323fbada7..9df7c213716a 100644 --- a/drivers/media/dvb-core/dvbdev.c +++ b/drivers/media/dvb-core/dvbdev.c @@ -530,6 +530,9 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev, for (minor = 0; minor < MAX_DVB_MINORS; minor++) if (!dvb_minors[minor]) break; +#else + minor = nums2minor(adap->num, type, id); +#endif if (minor >= MAX_DVB_MINORS) { if (new_node) { list_del(&new_node->list_head); @@ -543,17 +546,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev, mutex_unlock(&dvbdev_register_lock); return -EINVAL; } -#else - minor = nums2minor(adap->num, type, id); - if (minor >= MAX_DVB_MINORS) { - dvb_media_device_free(dvbdev); - list_del(&dvbdev->list_head); - kfree(dvbdev); - *pdvbdev = NULL; - mutex_unlock(&dvbdev_register_lock); - return ret; - } -#endif + dvbdev->minor = minor; dvb_minors[minor] = dvb_device_get(dvbdev); up_write(&minor_rwsem); From 04e8cbcdad0db073cefbcbb27d967b336ba56fbc Mon Sep 17 00:00:00 2001 From: Philipp Stanner Date: Thu, 2 Nov 2023 20:16:34 +0100 Subject: [PATCH 06/11] media: drivers/media/dvb-core: copy user arrays safely At several positions in dvb_frontend.c, memdup_user() is utilized to copy userspace arrays. This is done without overflow checks. Use the new wrapper memdup_array_user() to copy the arrays more safely. Link: https://lore.kernel.org/linux-media/20231102191633.52592-2-pstanner@redhat.com Suggested-by: Dave Airlie Signed-off-by: Philipp Stanner Signed-off-by: Mauro Carvalho Chehab --- drivers/media/dvb-core/dvb_frontend.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c index 9293b058ab99..93d3378a0df4 100644 --- a/drivers/media/dvb-core/dvb_frontend.c +++ b/drivers/media/dvb-core/dvb_frontend.c @@ -2168,7 +2168,8 @@ static int dvb_frontend_handle_compat_ioctl(struct file *file, unsigned int cmd, if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS)) return -EINVAL; - tvp = memdup_user(compat_ptr(tvps->props), tvps->num * sizeof(*tvp)); + tvp = memdup_array_user(compat_ptr(tvps->props), + tvps->num, sizeof(*tvp)); if (IS_ERR(tvp)) return PTR_ERR(tvp); @@ -2199,7 +2200,8 @@ static int dvb_frontend_handle_compat_ioctl(struct file *file, unsigned int cmd, if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS)) return -EINVAL; - tvp = memdup_user(compat_ptr(tvps->props), tvps->num * sizeof(*tvp)); + tvp = memdup_array_user(compat_ptr(tvps->props), + tvps->num, sizeof(*tvp)); if (IS_ERR(tvp)) return PTR_ERR(tvp); @@ -2379,7 +2381,8 @@ static int dvb_get_property(struct dvb_frontend *fe, struct file *file, if (!tvps->num || tvps->num > DTV_IOCTL_MAX_MSGS) return -EINVAL; - tvp = memdup_user((void __user *)tvps->props, tvps->num * sizeof(*tvp)); + tvp = memdup_array_user((void __user *)tvps->props, + tvps->num, sizeof(*tvp)); if (IS_ERR(tvp)) return PTR_ERR(tvp); @@ -2457,7 +2460,8 @@ static int dvb_frontend_handle_ioctl(struct file *file, if (!tvps->num || (tvps->num > DTV_IOCTL_MAX_MSGS)) return -EINVAL; - tvp = memdup_user((void __user *)tvps->props, tvps->num * sizeof(*tvp)); + tvp = memdup_array_user((void __user *)tvps->props, + tvps->num, sizeof(*tvp)); if (IS_ERR(tvp)) return PTR_ERR(tvp); From df21b7152cf1bd05666e17e2513e840d0cee2bed Mon Sep 17 00:00:00 2001 From: Kevin Hao Date: Thu, 21 Dec 2023 10:22:26 +0800 Subject: [PATCH 07/11] media: dvb_frontend: Use wait_event_freezable_timeout() for freezable kthread A freezable kernel thread can enter frozen state during freezing by either calling try_to_freeze() or using wait_event_freezable() and its variants. So for the following snippet of code in a kernel thread loop: wait_event_interruptible_timeout(); try_to_freeze(); We can change it to a simple wait_event_freezable_timeout() and then eliminate a function call. Signed-off-by: Kevin Hao Signed-off-by: Hans Verkuil --- drivers/media/dvb-core/dvb_frontend.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c index 93d3378a0df4..4f78f30b3646 100644 --- a/drivers/media/dvb-core/dvb_frontend.c +++ b/drivers/media/dvb-core/dvb_frontend.c @@ -679,12 +679,10 @@ static int dvb_frontend_thread(void *data) set_freezable(); while (1) { up(&fepriv->sem); /* is locked when we enter the thread... */ -restart: - wait_event_interruptible_timeout(fepriv->wait_queue, - dvb_frontend_should_wakeup(fe) || - kthread_should_stop() || - freezing(current), - fepriv->delay); + wait_event_freezable_timeout(fepriv->wait_queue, + dvb_frontend_should_wakeup(fe) || + kthread_should_stop(), + fepriv->delay); if (kthread_should_stop() || dvb_frontend_is_exiting(fe)) { /* got signal or quitting */ @@ -694,9 +692,6 @@ restart: break; } - if (try_to_freeze()) - goto restart; - if (down_interruptible(&fepriv->sem)) break; From a351a382c8e865993ab9a1dd3b9a3800e8f91d4a Mon Sep 17 00:00:00 2001 From: Mauro Carvalho Chehab Date: Tue, 15 Oct 2024 16:05:16 +0200 Subject: [PATCH 08/11] media: dvb_frontend: don't play tricks with underflow values fepriv->auto_sub_step is unsigned. Setting it to -1 is just a trick to avoid calling continue, as reported by Coverity. It relies to have this code just afterwards: if (!ready) fepriv->auto_sub_step++; Simplify the code by simply setting it to zero and use continue to return to the while loop. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Mauro Carvalho Chehab --- drivers/media/dvb-core/dvb_frontend.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c index 4f78f30b3646..a05aa271a1ba 100644 --- a/drivers/media/dvb-core/dvb_frontend.c +++ b/drivers/media/dvb-core/dvb_frontend.c @@ -443,8 +443,8 @@ static int dvb_frontend_swzigzag_autotune(struct dvb_frontend *fe, int check_wra default: fepriv->auto_step++; - fepriv->auto_sub_step = -1; /* it'll be incremented to 0 in a moment */ - break; + fepriv->auto_sub_step = 0; + continue; } if (!ready) fepriv->auto_sub_step++; From 56f6f0e47ff9ea8bd37492a87fd21d9e6cc40241 Mon Sep 17 00:00:00 2001 From: "jieqing.wang" Date: Thu, 1 Dec 2022 13:27:05 +0800 Subject: [PATCH 09/11] media: fix secfeed undefined when filter alloc fail In dvb_dmxdev_filter_start, when secfeed allocate secfilter fail. the secfeed of current dmxfilter, need set to NULL; Instead of call start_filtering, which already handled in dvb_dmxdev_feed_restart Signed-off-by: jieqing.wang Link: https://lore.kernel.org/r/20221201052705.2313911-1-standback@126.com Signed-off-by: Mauro Carvalho Chehab --- drivers/media/dvb-core/dmxdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/dvb-core/dmxdev.c b/drivers/media/dvb-core/dmxdev.c index 9ce5f010de3f..6063782e937a 100644 --- a/drivers/media/dvb-core/dmxdev.c +++ b/drivers/media/dvb-core/dmxdev.c @@ -731,7 +731,7 @@ static int dvb_dmxdev_filter_start(struct dmxdev_filter *filter) ret = (*secfeed)->allocate_filter(*secfeed, secfilter); if (ret < 0) { dvb_dmxdev_feed_restart(filter); - filter->feed.sec->start_filtering(*secfeed); + *secfeed = NULL; dprintk("could not get filter\n"); return ret; } From 7457f7d86c7b730dde240b58255f1b1c791413ce Mon Sep 17 00:00:00 2001 From: Arnd Bergmann Date: Mon, 11 Nov 2024 11:41:42 +0100 Subject: [PATCH 10/11] media: cxd2841er: fix 64-bit division on gcc-9 It appears that do_div() once more gets confused by a complex expression that ends up not quite being constant despite __builtin_constant_p() thinking it is: ERROR: modpost: "__aeabi_uldivmod" [drivers/media/dvb-frontends/cxd2841er.ko] undefined! Use div_u64() instead, forcing the expression to be evaluated first, and making it a bit more readable. Cc: Dan Carpenter Reported-by: Naresh Kamboju Closes: https://lore.kernel.org/linux-media/CA+G9fYvvNm-aYodLaAwwTjEGtX0YxR-1R14FOA5aHKt0sSVsYg@mail.gmail.com/ Reported-by: Linux Kernel Functional Testing Closes: https://lore.kernel.org/linux-media/CA+G9fYvvNm-aYodLaAwwTjEGtX0YxR-1R14FOA5aHKt0sSVsYg@mail.gmail.com/ Signed-off-by: Arnd Bergmann Signed-off-by: Hans Verkuil [hverkuil: added Closes tags] --- drivers/media/dvb-frontends/cxd2841er.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/drivers/media/dvb-frontends/cxd2841er.c b/drivers/media/dvb-frontends/cxd2841er.c index e9d1eef40c62..798da5042136 100644 --- a/drivers/media/dvb-frontends/cxd2841er.c +++ b/drivers/media/dvb-frontends/cxd2841er.c @@ -311,12 +311,8 @@ static int cxd2841er_set_reg_bits(struct cxd2841er_priv *priv, static u32 cxd2841er_calc_iffreq_xtal(enum cxd2841er_xtal xtal, u32 ifhz) { - u64 tmp; - - tmp = (u64) ifhz * 16777216; - do_div(tmp, ((xtal == SONY_XTAL_24000) ? 48000000 : 41000000)); - - return (u32) tmp; + return div_u64(ifhz * 16777216ull, + (xtal == SONY_XTAL_24000) ? 48000000 : 41000000); } static u32 cxd2841er_calc_iffreq(u32 ifhz) From d7c508c5ad80bb7afcdc3f4d86320ae8b877f4d9 Mon Sep 17 00:00:00 2001 From: Junlin Li Date: Tue, 2 Jul 2024 21:24:13 +0800 Subject: [PATCH 11/11] drivers: media: dvb-frontends/rtl2832: fix an out-of-bounds write error Ensure index in rtl2832_pid_filter does not exceed 31 to prevent out-of-bounds access. dev->filters is a 32-bit value, so set_bit and clear_bit functions should only operate on indices from 0 to 31. If index is 32, it will attempt to access a non-existent 33rd bit, leading to out-of-bounds access. Change the boundary check from index > 32 to index >= 32 to resolve this issue. Signed-off-by: Junlin Li Signed-off-by: Hans Verkuil Fixes: 4b01e01a81b6 ("[media] rtl2832: implement PID filter") [hverkuil: added fixes tag, rtl2830_pid_filter -> rtl2832_pid_filter in logmsg] --- drivers/media/dvb-frontends/rtl2832.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/dvb-frontends/rtl2832.c b/drivers/media/dvb-frontends/rtl2832.c index 5142820b1b3d..76c3f40443b2 100644 --- a/drivers/media/dvb-frontends/rtl2832.c +++ b/drivers/media/dvb-frontends/rtl2832.c @@ -983,7 +983,7 @@ static int rtl2832_pid_filter(struct dvb_frontend *fe, u8 index, u16 pid, index, pid, onoff, dev->slave_ts); /* skip invalid PIDs (0x2000) */ - if (pid > 0x1fff || index > 32) + if (pid > 0x1fff || index >= 32) return 0; if (onoff)