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

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

[Sean Young]

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: rc: Use after free in ir_raw_event_handle()
  media: rc: Fix use after free in bpf progs
  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()

 drivers/media/pci/cx88/cx88-input.c       | 22 ++++++------
 drivers/media/pci/saa7134/saa7134-input.c |  8 ++---
 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              | 41 +++++++----------------
 drivers/media/rc/rc-main.c                | 19 ++++++-----
 drivers/media/rc/redrat3.c                | 35 +++++++++++++++----
 drivers/media/rc/streamzap.c              |  1 +
 drivers/media/rc/sunxi-cir.c              |  2 +-
 10 files changed, 90 insertions(+), 75 deletions(-)

-- 
2.55.0

[PATCH v4 12/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 96849faafde3..b32f3cff9401 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -651,8 +651,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;
 	}
@@ -662,9 +665,6 @@ void ir_raw_event_unregister(struct rc_dev *dev)
 {
 	struct ir_raw_handler *handler;
 
-	if (!dev || !dev->raw)
-		return;
-
 	kthread_stop(dev->raw->thread);
 	timer_delete_sync(&dev->raw->edge_handle);
 
@@ -677,11 +677,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 v4 12/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:
- [High] Replacing the `!raw` check with `!rcdev->registered` introduces a reproducible NULL pointer dereference in `lirc_bpf_attach`, `lirc_bpf_detach`, and `lirc_prog_query` during the `rc_register_device` error path.
--

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

media: rc: Fix use after free in bpf progs

This commit replaces the NULL check on rcdev->raw with a check on
rcdev->registered to address a potential use-after-free issue when
unregistering devices. It also moves lirc_bpf_free() inside the
ir_raw_handler_lock.

> 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) {
>  		ret = -ENODEV;
>  		goto unlock;
>  	}
>  
> +	raw = rcdev->raw;
> +
>  	old_array = lirc_rcu_dereference(raw->progs);

[Severity: High]
Can replacing this !raw check with !rcdev->registered cause a NULL pointer
dereference?

In rc_register_device(), dev->registered is set to true. If lirc_register()
succeeds, the character device is created and can be opened by userspace. If a
subsequent initialization step fails (e.g., ir_raw_event_register()), the error
path calls ir_raw_event_free(), which frees dev->raw and sets it to NULL.
However, the error path never sets dev->registered to false.

If a userspace process opens the device and races to call the bpf() syscall
during this failure, will !rcdev->registered incorrectly evaluate to false,
leading to a dereference of raw->progs when raw is NULL?

[ ... ]

> @@ -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);

[Severity: High]
Does this face the same NULL pointer dereference issue during the 
rc_register_device() error path as lirc_bpf_attach()?

[ ... ]

> @@ -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);

[Severity: High]
Could lirc_prog_query() also crash here if rcdev->raw is NULL but
rcdev->registered remains true during a registration failure?

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