[PATCH v5 00/15] Fix leaks in rc core

[PATCH v5 00/15] Fix leaks in rc core

[Sean Young]

Changes since v4:
 - media: redrat3: Error path leaves device in transmitting state 
   - this now removes the check for transmitting boolean in transmit as it
     is now redundant.
   - Reworded commit message
 - media: rc: Use after free in ir_raw_event_handle()
 - media: rc: Fix race condition during rc_register_device()
   - Reworded commit message
 - commits affecting particular modules are sorted together

Changes since v3:
 - sashiko had many more good review comments

Changes since v2:
 - sashiko had many more good review comments

Changes since v1:
 - sashiko had many good review comments
 - Added fix rc: Use after free in ir_raw_event_handle()
 - Added fix meson-ir-tx: Ensure probe error is propagated
 - Added fix redrat3: Ensure all urbs are suspended
 - Added fix media: redrat3: Error path leaves device in transmitting state
 - streamzap fix was incorrect
 - Other minor fixes


Sean Young (15):
  media: streamzap: Add missing rc_unregister_device()
  media: redrat3: Ensure rc device is freed if enable_detector() fails
  media: redrat3: Ensure we don't read beyond the end of the packet
  media: redrat3: Ensure all urbs are suspended
  media: redrat3: Error path leaves device in transmitting state
  media: sunxi-cir: Ensure no more interrupts can occur before free
  media: meson-ir-tx: Ensure clock is disabled on unbind
  media: meson-ir-tx: Ensure rc_free_device() is called on unbind
  media: meson-ir-tx: Ensure probe error is propagated
  media: ir-hix5hd2: Ensure rdev is setup before interrupts are enabled
  media: cx88: Specify rc type at rc_allocate_type()
  media: saa7134: Specify rc type at rc_allocate_type()
  media: rc: Fix race condition during rc_register_device()
  media: rc: Use after free in ir_raw_event_handle()
  media: rc: Fix use after free in bpf progs

 drivers/media/pci/cx88/cx88-input.c       | 22 ++++++-----
 drivers/media/pci/saa7134/saa7134-input.c |  7 ++--
 drivers/media/rc/bpf-lirc.c               | 18 ++++++---
 drivers/media/rc/ir-hix5hd2.c             |  5 ++-
 drivers/media/rc/meson-ir-tx.c            | 14 +++----
 drivers/media/rc/rc-ir-raw.c              | 47 +++++++++--------------
 drivers/media/rc/rc-main.c                | 19 +++++----
 drivers/media/rc/redrat3.c                | 40 +++++++++++++------
 drivers/media/rc/streamzap.c              |  1 +
 drivers/media/rc/sunxi-cir.c              |  2 +-
 10 files changed, 97 insertions(+), 78 deletions(-)

-- 
2.55.0

[PATCH v5 15/15] media: rc: Fix use after free in bpf progs

[Sean Young]

Since commit dccc0c3ddf8f ("media: rc: fix race between unregister and
urb/irq callbacks"), rcdev->raw is no longer set to NULL after device
unregister. raw->progs could point to stale data.

Fixes: dccc0c3ddf8f ("media: rc: fix race between unregister and urb/irq callbacks")
Signed-off-by: Sean Young <sean@mess.org>
Cc: stable@vger.kernel.org
---
 drivers/media/rc/bpf-lirc.c  | 18 +++++++++++++-----
 drivers/media/rc/rc-ir-raw.c | 11 +++--------
 2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
index 2f7564f26445..14ab611e7445 100644
--- a/drivers/media/rc/bpf-lirc.c
+++ b/drivers/media/rc/bpf-lirc.c
@@ -148,12 +148,13 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
 	if (ret)
 		return ret;
 
-	raw = rcdev->raw;
-	if (!raw) {
+	if (!rcdev->registered) {
 		ret = -ENODEV;
 		goto unlock;
 	}
 
+	raw = rcdev->raw;
+
 	old_array = lirc_rcu_dereference(raw->progs);
 	if (old_array && bpf_prog_array_length(old_array) >= BPF_MAX_PROGS) {
 		ret = -E2BIG;
@@ -186,12 +187,13 @@ static int lirc_bpf_detach(struct rc_dev *rcdev, struct bpf_prog *prog)
 	if (ret)
 		return ret;
 
-	raw = rcdev->raw;
-	if (!raw) {
+	if (!rcdev->registered) {
 		ret = -ENODEV;
 		goto unlock;
 	}
 
+	raw = rcdev->raw;
+
 	old_array = lirc_rcu_dereference(raw->progs);
 	ret = bpf_prog_array_copy(old_array, prog, NULL, 0, &new_array);
 	/*
@@ -235,7 +237,8 @@ void lirc_bpf_free(struct rc_dev *rcdev)
 	struct bpf_prog_array_item *item;
 	struct bpf_prog_array *array;
 
-	array = lirc_rcu_dereference(rcdev->raw->progs);
+	array = rcu_replace_pointer(rcdev->raw->progs, NULL,
+				    lockdep_is_held(&ir_raw_handler_lock));
 	if (!array)
 		return;
 
@@ -316,6 +319,11 @@ int lirc_prog_query(const union bpf_attr *attr, union bpf_attr __user *uattr)
 	if (ret)
 		goto put;
 
+	if (!rcdev->registered) {
+		ret = -ENODEV;
+		goto unlock;
+	}
+
 	progs = lirc_rcu_dereference(rcdev->raw->progs);
 	cnt = progs ? bpf_prog_array_length(progs) : 0;
 
diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index 86de1b26731d..54b323becb1f 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -632,8 +632,11 @@ void ir_raw_event_free(struct rc_dev *dev)
 {
 	if (dev->raw) {
 		timer_delete_sync(&dev->raw->edge_handle);
+		mutex_lock(&ir_raw_handler_lock);
 		if (dev->raw->thread)
 			put_task_struct(dev->raw->thread);
+		lirc_bpf_free(dev);
+		mutex_unlock(&ir_raw_handler_lock);
 		kfree(dev->raw);
 		dev->raw = NULL;
 	}
@@ -643,9 +646,6 @@ void ir_raw_event_unregister(struct rc_dev *dev)
 {
 	struct ir_raw_handler *handler;
 
-	if (!dev || !dev->raw)
-		return;
-
 	/*
 	 * After ir_raw_event_unregister() is called, an sync
 	 * call to ir_raw_event_handle() can still arrive. This function
@@ -665,11 +665,6 @@ void ir_raw_event_unregister(struct rc_dev *dev)
 
 	lirc_bpf_free(dev);
 
-	/*
-	 * A user can be calling bpf(BPF_PROG_{QUERY|ATTACH|DETACH}), so
-	 * ensure that the raw member is null on unlock; this is how
-	 * "device gone" is checked.
-	 */
 	mutex_unlock(&ir_raw_handler_lock);
 }
 
-- 
2.55.0

Re: [PATCH v5 15/15] media: rc: Fix use after free in bpf progs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] If `rc_register_device()` fails after making the LIRC cdev available, `dev->registered` is left as `true`. This allows a concurrent user holding an open file descriptor to bypass the registration check and trigger driver callbacks on freed private data.
--

--- Patch [15]: [PATCH v5 15/15] media: rc: Fix use after free in bpf progs ---
commit 56728bdc06bc2bfe04a9a68caffbbf3e5379fc20
Author: Sean Young <sean@mess.org>

media: rc: Fix use after free in bpf progs

This commit fixes a potential use-after-free in BPF programs by checking
rcdev->registered instead of rcdev->raw during BPF attach, detach, and
query operations. It also updates the raw event handler cleanup logic
during device unregistration.

> diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
> index 2f7564f26445c..14ab611e74457 100644
> --- a/drivers/media/rc/bpf-lirc.c
> +++ b/drivers/media/rc/bpf-lirc.c
> @@ -148,12 +148,13 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
>  	if (ret)
>  		return ret;
>  
> -	raw = rcdev->raw;
> -	if (!raw) {
> +	if (!rcdev->registered) {

[Severity: High]
This is a pre-existing issue, but does relying on rcdev->registered completely
prevent use-after-free scenarios if the device registration fails?

Looking at rc_register_device() in drivers/media/rc/rc-main.c, if an error
occurs after making the LIRC cdev available, dev->registered is left as true
in the error unwind path:

drivers/media/rc/rc-main.c:rc_register_device() {
        ...
        dev->registered = true;
        ...
        if (error)
                goto out_rx;
        ...
out_rx:
        rc_free_rx_device(dev);
out_lirc:
        if (dev->allowed_protocols != RC_PROTO_BIT_CEC)
                lirc_unregister(dev);
        ...
}

If a user-space process opens the newly registered /dev/lircX device before
rc_register_device() completes and fails, could it bypass this new check since
dev->registered remains true?

Similarly, could this cause issues in lirc_transmit() (in
drivers/media/rc/lirc_dev.c) where it also checks if (!dev->registered),
potentially allowing a freed driver's tx_ir callback to be invoked?

>  		ret = -ENODEV;
>  		goto unlock;
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785338381.git.sean@mess.org?part=15