Skip to contentSkip to conversation

Fix leaks in rc core

PATCH In review Sean Young 4 participants 22 messages Jul 29 – Aug 18

Subsystems (9)
Stable branch Media (V4L/DVB) Kernel janitors BPF +5 more subsystems
lore
Share & tools
lore

Sean Young

author maintainer
#0 lore raw

v5 cover Fix leaks in rc core

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(-)
Signature · 1 line
2.55.0
author maintainer
#1 lore raw

PATCH v5 1/15 media: streamzap: Add missing rc_unregister_device()

If usb_submit_urb() fails during probe, then the error path is missing a call to rc_unregister_device(), which will leak various things like the input device.

Fixes 42844992664f ("media: rc: streamzap: Error handling in probe")
Signed-off-by Sean Young <[email protected]>
Cc [email protected]
---
 drivers/media/rc/streamzap.c | 1 +
 1 file changed, 1 insertion(+)
drivers/media/rc/streamzap.c +1 -0
index 307985d74fe8..41195ad82734 100644
@@ -365,6 +365,7 @@ static int streamzap_probe(struct usb_interface *intf,
 
 	return 0;
 rc_submit_fail:
+	rc_unregister_device(sz->rdev);
 	rc_free_device(sz->rdev);
 	usb_set_intfdata(intf, NULL);
 rc_dev_fail:
Signature · 1 line
2.55.0
author maintainer
#2 lore raw

PATCH v5 2/15 media: redrat3: Ensure rc device is freed if enable_detector() fails

In reply to Sean Young

This particular error path does not free the rc device at all, with its priv pointer still pointing at freed memory.

Fixes 2154be651b90 ("[media] redrat3: new rc-core IR transceiver device driver")
Signed-off-by Sean Young <[email protected]>
Cc [email protected]
---
 drivers/media/rc/redrat3.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
drivers/media/rc/redrat3.c +9 -3
index 3f828a564e19..2b639fe59923 100644
@@ -979,6 +979,7 @@ static int redrat3_dev_probe(struct usb_interface *intf,
 	struct usb_endpoint_descriptor *ep_narrow = NULL;
 	struct usb_endpoint_descriptor *ep_wide = NULL;
 	struct usb_endpoint_descriptor *ep_out = NULL;
+	struct rc_dev *rc = NULL;
 	u8 addr, attrs;
 	int pipe, i;
 	int retval = -ENOMEM;

@@ -1102,26 +1103,31 @@ static int redrat3_dev_probe(struct usb_interface *intf,
 	if (retval)
 		goto redrat_free;
 
-	rr3->rc = redrat3_init_rc_dev(rr3);
-	if (!rr3->rc) {
+	rc = redrat3_init_rc_dev(rr3);
+	if (!rc) {
 		retval = -ENOMEM;
 		goto led_free;
 	}
 
+	rr3->rc = rc;
+
 	/* might be all we need to do? */
 	retval = redrat3_enable_detector(rr3);
 	if (retval < 0)
-		goto led_free;
+		goto rc_free;
 
 	/* we can register the device now, as it is ready */
 	usb_set_intfdata(intf, rr3);
 
 	return 0;
 
+rc_free:
+	rc_unregister_device(rc);
 led_free:
 	led_classdev_unregister(&rr3->led);
 redrat_free:
 	redrat3_delete(rr3, rr3->udev);
+	rc_free_device(rc);
 
 no_endpoints:
 	return retval;
Signature · 1 line
2.55.0
author maintainer
#3 lore raw

PATCH v5 3/15 media: redrat3: Ensure we don't read beyond the end of the packet

In reply to Sean Young

The length and offset is provided by the usb device, so it should be validated.

Fixes 2154be651b90 ("[media] redrat3: new rc-core IR transceiver device driver")
Signed-off-by Sean Young <[email protected]>
Cc [email protected]
---
 drivers/media/rc/redrat3.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
drivers/media/rc/redrat3.c +16 -0
index 2b639fe59923..f86efcb74e6e 100644
@@ -358,8 +358,24 @@ static void redrat3_process_ir_data(struct redrat3_dev *rr3)
 
 	/* process each rr3 encoded byte into an int */
 	sig_size = be16_to_cpu(rr3->irdata.sig_size);
+
+	/*
+	 * Note we are not checking if we are reading beyond the end of the
+	 * packet which was sent, and reading stale data. If the device
+	 * sends a packet which is short then we get garbage IR, but no
+	 * out of bounds read.
+	 */
+	if (sig_size > RR3_MAX_SIG_SIZE) {
+		dev_err(dev, "length %u is incorrect\n", sig_size);
+		return;
+	}
+
 	for (i = 0; i < sig_size; i++) {
 		offset = rr3->irdata.sigdata[i];
+		if (offset >= RR3_DRIVER_MAXLENS) {
+			dev_err(dev, "offset %u is incorrect\n", offset);
+			return;
+		}
 		val = get_unaligned_be16(&rr3->irdata.lens[offset]);
 
 		/* we should always get pulse/space/pulse/space samples */
Signature · 1 line
2.55.0
author maintainer
#4 lore raw

PATCH v5 4/15 media: redrat3: Ensure all urbs are suspended

In reply to Sean Young

Ensure that the learn urb is stopped before suspend.

Fixes c49fcdde38cb ("[media] redrat3: enable carrier reports using wideband receiver")
Signed-off-by Sean Young <[email protected]>
Cc [email protected]
---
 drivers/media/rc/redrat3.c | 1 +
 1 file changed, 1 insertion(+)
drivers/media/rc/redrat3.c +1 -0
index f86efcb74e6e..994d4864520c 100644
@@ -1170,6 +1170,7 @@ static int redrat3_dev_suspend(struct usb_interface *intf, pm_message_t message)
 	usb_kill_urb(rr3->narrow_urb);
 	usb_kill_urb(rr3->wide_urb);
 	usb_kill_urb(rr3->flash_urb);
+	usb_kill_urb(rr3->learn_urb);
 	return 0;
 }
Signature · 1 line
2.55.0
author maintainer
#5 lore raw

PATCH v5 5/15 media: redrat3: Error path leaves device in transmitting state

In reply to Sean Young

In the allocation failure error path, the transmitting boolean is not cleared. If this error path is followed the transmitter cannot be used again until the device is replugged or the module reloaded.

Fix the error path by moving the assignment statement.

Remove the check for the transmitting state in transmit, since rc-core ensures there is never a reentrant call to the driver transmit function; the transmit function is always called with dev->lock held.

The transmitting boolean is only used for interpretting firmware errors.

Fixes fac59136bc22 ("[media] RedRat3: Return directly after a failed kcalloc() in redrat3_transmit_ir()")"
Signed-off-by Sean Young <[email protected]>
Cc [email protected]
---
 drivers/media/rc/redrat3.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)
drivers/media/rc/redrat3.c +3 -8
index 994d4864520c..602b84dd628d 100644
@@ -773,17 +773,9 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 	u8 curlencheck = 0;
 	unsigned i, sendbuf_len;
 
-	if (rr3->transmitting) {
-		dev_warn(dev, "%s: transmitter already in use\n", __func__);
-		return -EAGAIN;
-	}
-
 	if (count > RR3_MAX_SIG_SIZE - RR3_TX_TRAILER_LEN)
 		return -EINVAL;
 
-	/* rr3 will disable rc detector on transmit */
-	rr3->transmitting = true;
-
 	sample_lens = kzalloc_objs(*sample_lens, RR3_DRIVER_MAXLENS);
 	if (!sample_lens)
 		return -ENOMEM;

@@ -794,6 +786,9 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 		goto out;
 	}
 
+	/* rr3 will disable rc detector on transmit */
+	rr3->transmitting = true;
+
 	for (i = 0; i < count; i++) {
 		cur_sample_len = redrat3_us_to_len(txbuf[i]);
 		if (cur_sample_len > 0xffff) {
Signature · 1 line
2.55.0
#6 lore raw

Re: [PATCH v5 05/15] media: redrat3: Error path leaves device in transmitting state

…

Quoted 1 line

The transmitting boolean is only used for interpretting firmware errors.

interpreting?

Thanks for another try to improve the change description besides mentioned implementation details.

Quoted 1 line
Fixes fac59136bc22 ("[media] RedRat3: Return directly after a failed kcalloc() in redrat3_transmit_ir()")"

Can it be that commits were improvable already before I dared to adjust also this function implementation?

Regards,
Markus

#7 lore raw

Re: [PATCH v5 05/15] media: redrat3: Error path leaves device in transmitting state

In reply to Sean Young

…

Quoted 1 line

Fix the error path by moving the assignment statement.

How do you think about to increase the application of scope-based resource management also for such a function implementation?

…

Quoted 1 line
Fixes fac59136bc22 ("[media] RedRat3: Return directly after a failed kcalloc() in redrat3_transmit_ir()")"…

Would a revert be needed for this commit before further adjustments will be considered?

Regards,
Markus

author maintainer
#8 lore raw

PATCH v5 6/15 media: sunxi-cir: Ensure no more interrupts can occur before free

In reply to Sean Young

Only call rc_free_device() once the hardware has been stopped.

Fixes dccc0c3ddf8f ("media: rc: fix race between unregister and urb/irq callbacks")
Signed-off-by Sean Young <[email protected]>
Cc [email protected]
---
 drivers/media/rc/sunxi-cir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/rc/sunxi-cir.c +1 -1
index 28e840a7e5b8..af1ee08ffdbe 100644
@@ -374,8 +374,8 @@ static void sunxi_ir_remove(struct platform_device *pdev)
 	struct sunxi_ir *ir = platform_get_drvdata(pdev);
 
 	rc_unregister_device(ir->rc);
-	rc_free_device(ir->rc);
 	sunxi_ir_hw_exit(&pdev->dev);
+	rc_free_device(ir->rc);
 }
 
 static void sunxi_ir_shutdown(struct platform_device *pdev)
Signature · 1 line
2.55.0
AI review
#9 lore raw

Re: [PATCH v5 06/15] media: sunxi-cir: Ensure no more interrupts can occur before free

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

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

Pre-existing issues:
- [High] The teardown race between sunxi_ir_remove() and sunxi_ir_irq() is not resolved by the patch. The interrupt remains active and unsynchronized while clocks are disabled and memory is freed, leading to synchronous external aborts and use-after-free vulnerabilities. A similar issue exists in probe() where the IRQ is requested before clocks are enabled. --

--- Patch [6]: [PATCH v5 06/15] media: sunxi-cir: Ensure no more interrupts can occur before free --- commit 485344025b667511dbe1afe16a5fb9cab1f312f6
Author: Sean Young <[email protected]>

media: sunxi-cir: Ensure no more interrupts can occur before free

This commit delays rc_free_device() until after sunxi_ir_hw_exit() in the remove path. It attempts to ensure the hardware is stopped and no further interrupts can occur before the device memory is freed.

Cc [email protected]

drivers/media/rc/sunxi-cir.c static void sunxi_ir_remove(struct platform_device *pdev) · line 378

Quoted 11 lines
drivers/media/rc/sunxi-cir.c +1 -1
index 28e840a7e5b8c..af1ee08ffdbec 100644
@@ -374,8 +374,8 @@ static void sunxi_ir_remove(struct platform_device *pdev)
 	struct sunxi_ir *ir = platform_get_drvdata(pdev);
 
 	rc_unregister_device(ir->rc);
-	rc_free_device(ir->rc);
 	sunxi_ir_hw_exit(&pdev->dev);
+	rc_free_device(ir->rc);

[Severity: High]
This is a pre-existing issue, but does this actually prevent interrupts from occurring before the free?

The driver requests the IRQ via devm_request_irq() in sunxi_ir_probe(), which means the IRQ is not freed or synchronized until devres cleanup runs after sunxi_ir_remove() returns.

While sunxi_ir_hw_exit() disables the APB clock, it does not explicitly disable or synchronize the IRQ handler. If an interrupt executes concurrently, can it fault when reading unclocked MMIO registers?

drivers/media/rc/sunxi-cir.c:sunxi_ir_irq() {
	...
	status = readl(ir->base + SUNXI_IR_RXSTA_REG);
	...
	ir_raw_event_store_with_filter(ir->rc, &rawir);
}

Because the IRQ handler is unsynchronized, could it also dereference the freed ir->rc if it races with or runs after rc_free_device()?

Additionally, is there a similar window in sunxi_ir_probe()?

drivers/media/rc/sunxi-cir.c:sunxi_ir_probe() {
	...
	ret = devm_request_irq(dev, ir->irq, sunxi_ir_irq, 0, SUNXI_IR_DEV, ir);
	...
	ret = sunxi_ir_hw_init(dev);
	...
}

The IRQ handler is requested and enabled before sunxi_ir_hw_init() enables the APB clock. Could a pending interrupt trigger the handler and access unclocked registers during boot?

Quoted 3 lines
drivers/media/rc/sunxi-cir.c +0 -0
 }
 
 static void sunxi_ir_shutdown(struct platform_device *pdev)
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
author maintainer
#10 lore raw

PATCH v5 7/15 media: meson-ir-tx: Ensure clock is disabled on unbind

In reply to Sean Young

clk_prepare_enable() needs a call to clk_disable_unprepare() on driver unbind. Make it devm managed.

Fixes 49be1c78d575 ("media: rc: introduce Meson IR TX driver")
Signed-off-by Sean Young <[email protected]>
Reviewed-by Martin Blumenstingl <[email protected]>
Cc [email protected]
---
 drivers/media/rc/meson-ir-tx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/rc/meson-ir-tx.c +2 -2
index fded2c256f2a..e7bb107e6a84 100644
@@ -288,8 +288,8 @@ static int meson_irtx_mod_clock_probe(struct meson_irtx *ir,
 	if (!np)
 		return -ENODEV;
 
-	clock = devm_clk_get(ir->dev, "xtal");
-	if (IS_ERR(clock) || clk_prepare_enable(clock))
+	clock = devm_clk_get_enabled(ir->dev, "xtal");
+	if (IS_ERR(clock))
 		return -ENODEV;
 
 	*clk_nr = IRB_MOD_XTAL3_CLK;
Signature · 1 line
2.55.0
AI review
#11 lore raw

Re: [PATCH v5 07/15] media: meson-ir-tx: Ensure clock is disabled on unbind

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

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

New issues:
- [Medium] The clock is automatically disabled on unbind via devres, but the hardware is not gracefully shut down first, violating the hardware state machine.

Pre-existing issues:
- [High] The driver overwrites genuine error codes (including -EPROBE_DEFER) with a hardcoded -ENODEV, breaking deferred probing. --

--- Patch [7]: [PATCH v5 07/15] media: meson-ir-tx: Ensure clock is disabled on unbind --- commit ed37040061f2257376fd955ba2cd00d04a46719c
Author: Sean Young <[email protected]>

media: meson-ir-tx: Ensure clock is disabled on unbind

This commit updates the meson-ir-tx driver to use devm_clk_get_enabled() so that the clock is automatically disabled during driver unbind.

drivers/media/rc/meson-ir-tx.c static int meson_irtx_mod_clock_probe(struct meson_irtx *ir, · line 293

Quoted 13 lines
drivers/media/rc/meson-ir-tx.c +2 -2
index fded2c256f2a6..e7bb107e6a843 100644
@@ -288,8 +288,8 @@ static int meson_irtx_mod_clock_probe(struct meson_irtx *ir,
 	if (!np)
 		return -ENODEV;
 
-	clock = devm_clk_get(ir->dev, "xtal");
-	if (IS_ERR(clock) || clk_prepare_enable(clock))
+	clock = devm_clk_get_enabled(ir->dev, "xtal");
+	if (IS_ERR(clock))
 		return -ENODEV;

[Severity: High]
This is a pre-existing issue, but does returning -ENODEV here in meson_irtx_mod_clock_probe() overwrite the actual error code returned by devm_clk_get_enabled(), such as -EPROBE_DEFER?

If the clock provider is not yet ready, returning -ENODEV instead of propagating the error code could permanently fail to probe instead of deferring. A similar override happens in meson_irtx_probe() with platform_get_irq().

[Severity: Medium]
With devm_clk_get_enabled(), the clock is automatically disabled on unbind via devres.

Is it safe to disable the clock without first shutting down the hardware block?

The driver configures the hardware in meson_irtx_probe(), but since there is no .remove callback in meson_irtx_pd (or a corresponding devm action) to gracefully shut down the hardware, it is left running. Disabling the clock while the hardware is still active might freeze the hardware block or cause bus lockups when the driver is unbound.

Quoted 2 lines
drivers/media/rc/meson-ir-tx.c +0 -0
 	*clk_nr = IRB_MOD_XTAL3_CLK;
Signature · 6 lines
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic
author maintainer
#12 lore raw

PATCH v5 8/15 media: meson-ir-tx: Ensure rc_free_device() is called on unbind

In reply to Sean Young

Make rc_dev devm managed.

Fixes dccc0c3ddf8f ("media: rc: fix race between unregister and urb/irq callbacks")
Signed-off-by Sean Young <[email protected]>
Reviewed-by Martin Blumenstingl <[email protected]>
Reviewed-by Neil Armstrong <[email protected]>
Cc [email protected]
---
 drivers/media/rc/meson-ir-tx.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/media/rc/meson-ir-tx.c +2 -4
index e7bb107e6a84..abb107d19e8c 100644
@@ -345,7 +345,7 @@ static int meson_irtx_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "irq request failed\n");
 
-	rc = rc_allocate_device(RC_DRIVER_IR_RAW_TX);
+	rc = devm_rc_allocate_device(dev, RC_DRIVER_IR_RAW_TX);
 	if (!rc)
 		return -ENOMEM;
 

@@ -358,10 +358,8 @@ static int meson_irtx_probe(struct platform_device *pdev)
 	rc->s_tx_duty_cycle = meson_irtx_set_duty_cycle;
 
 	ret = devm_rc_register_device(dev, rc);
-	if (ret < 0) {
-		rc_free_device(rc);
+	if (ret < 0)
 		return dev_err_probe(dev, ret, "rc_dev registration failed\n");
-	}
 
 	return 0;
 }
Signature · 1 line
2.55.0
author maintainer
#13 lore raw

PATCH v5 9/15 media: meson-ir-tx: Ensure probe error is propagated

In reply to Sean Young

devm_clk_get_enabled() may return -EPROBE_DEFER which needs to be propagated else the probe will not be deferred, it will fail instead.

Fixes 49be1c78d575 ("media: rc: introduce Meson IR TX driver")
Signed-off-by Sean Young <[email protected]>
Reviewed-by Martin Blumenstingl <[email protected]>
Reviewed-by Neil Armstrong <[email protected]>
Cc [email protected]
---
 drivers/media/rc/meson-ir-tx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/rc/meson-ir-tx.c +2 -2
index abb107d19e8c..174d5135e1bb 100644
@@ -290,7 +290,7 @@ static int meson_irtx_mod_clock_probe(struct meson_irtx *ir,
 
 	clock = devm_clk_get_enabled(ir->dev, "xtal");
 	if (IS_ERR(clock))
-		return -ENODEV;
+		return PTR_ERR(clock);
 
 	*clk_nr = IRB_MOD_XTAL3_CLK;
 	ir->clk_rate = clk_get_rate(clock) / 3;

@@ -324,7 +324,7 @@ static int meson_irtx_probe(struct platform_device *pdev)
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
-		return -ENODEV;
+		return irq;
 
 	ir->dev = dev;
 	ir->carrier = MIRTX_DEFAULT_CARRIER;
Signature · 1 line
2.55.0
author maintainer
#14 lore raw

PATCH v5 10/15 media: ir-hix5hd2: Ensure rdev is setup before interrupts are enabled

In reply to Sean Young

Once the interrupt handler is enabled, priv->rdev can be used. Ensure it is setup correctly so there is no race condition.

Fixes a84fcdaa9058 ("[media] rc: Introduce hix5hd2 IR transmitter driver")
Signed-off-by Sean Young <[email protected]>
Cc [email protected]
---
 drivers/media/rc/ir-hix5hd2.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/media/rc/ir-hix5hd2.c +3 -2
index 1b061e4a3dcf..aa3de4d57a58 100644
@@ -316,6 +316,9 @@ static int hix5hd2_ir_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto clkerr;
 
+	priv->rdev = rdev;
+	priv->dev = dev;
+
 	if (devm_request_irq(dev, priv->irq, hix5hd2_ir_rx_interrupt,
 			     0, pdev->name, priv) < 0) {
 		dev_err(dev, "IRQ %d register failed\n", priv->irq);

@@ -323,8 +326,6 @@ static int hix5hd2_ir_probe(struct platform_device *pdev)
 		goto regerr;
 	}
 
-	priv->rdev = rdev;
-	priv->dev = dev;
 	platform_set_drvdata(pdev, priv);
 
 	return ret;
Signature · 1 line
2.55.0
author maintainer
#15 lore raw

PATCH v5 11/15 media: cx88: Specify rc type at rc_allocate_type()

In reply to Sean Young

Changing the type of the rc device after allocation is ugly and this will also break in a future commit. Ensure that the allocation happens with the correct type in the first place.

Signed-off-by Sean Young <[email protected]>
---
 drivers/media/pci/cx88/cx88-input.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)
drivers/media/pci/cx88/cx88-input.c +12 -10
index 5d9ce4f9af01..c96d289e5ab7 100644
@@ -259,7 +259,7 @@ static void cx88_ir_close(struct rc_dev *rc)
 int cx88_ir_init(struct cx88_core *core, struct pci_dev *pci)
 {
 	struct cx88_IR *ir;
-	struct rc_dev *dev;
+	struct rc_dev *dev = NULL;
 	char *ir_codes = NULL;
 	u64 rc_proto = RC_PROTO_BIT_OTHER;
 	int err = -ENOMEM;

@@ -268,11 +268,8 @@ int cx88_ir_init(struct cx88_core *core, struct pci_dev *pci)
 				 */
 
 	ir = kzalloc_obj(*ir);
-	dev = rc_allocate_device(RC_DRIVER_IR_RAW);
-	if (!ir || !dev)
-		goto err_out_free;
-
-	ir->dev = dev;
+	if (!ir)
+		return -ENOMEM;
 
 	/* detect & configure */
 	switch (core->boardnr) {

@@ -439,6 +436,13 @@ int cx88_ir_init(struct cx88_core *core, struct pci_dev *pci)
 		goto err_out_free;
 	}
 
+	dev = rc_allocate_device(ir->sampling ?
+				 RC_DRIVER_IR_RAW : RC_DRIVER_SCANCODE);
+	if (!dev)
+		goto err_out_free;
+
+	ir->dev = dev;
+
 	/*
 	 * The usage of mask_keycode were very convenient, due to several
 	 * reasons. Among others, the scancode tables were using the scancode

@@ -477,12 +481,10 @@ int cx88_ir_init(struct cx88_core *core, struct pci_dev *pci)
 	dev->close = cx88_ir_close;
 	dev->scancode_mask = hardware_mask;
 
-	if (ir->sampling) {
+	if (ir->sampling)
 		dev->timeout = MS_TO_US(10); /* 10 ms */
-	} else {
-		dev->driver_type = RC_DRIVER_SCANCODE;
+	else
 		dev->allowed_protocols = rc_proto;
-	}
 
 	ir->core = core;
 	core->ir = ir;
Signature · 1 line
2.55.0
author maintainer
#16 lore raw

PATCH v5 12/15 media: saa7134: Specify rc type at rc_allocate_type()

In reply to Sean Young

Changing the type of the rc device after allocation is ugly and this will also break in a future commit. Ensure that the allocation happens with the correct type in the first place.

Signed-off-by Sean Young <[email protected]>
---
 drivers/media/pci/saa7134/saa7134-input.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
drivers/media/pci/saa7134/saa7134-input.c +3 -4
index 7f6680de3156..b2a2372d149a 100644
@@ -769,7 +769,8 @@ int saa7134_input_init1(struct saa7134_dev *dev)
 	}
 
 	ir = kzalloc_obj(*ir);
-	rc = rc_allocate_device(RC_DRIVER_SCANCODE);
+	rc = rc_allocate_device(raw_decode ?
+				RC_DRIVER_IR_RAW : RC_DRIVER_SCANCODE);
 	if (!ir || !rc) {
 		err = -ENOMEM;
 		goto err_out_free;

@@ -792,10 +793,8 @@ int saa7134_input_init1(struct saa7134_dev *dev)
 	rc->priv = dev;
 	rc->open = saa7134_ir_open;
 	rc->close = saa7134_ir_close;
-	if (raw_decode) {
-		rc->driver_type = RC_DRIVER_IR_RAW;
+	if (raw_decode)
 		rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
-	}
 
 	rc->device_name = saa7134_boards[dev->board].name;
 	rc->input_phys = ir->phys;
Signature · 1 line
2.55.0
author maintainer
#17 lore raw

PATCH v5 13/15 media: rc: Fix race condition during rc_register_device()

In reply to Sean Young

Device drivers can call ir_raw_event_store() or ir_raw_event_handle() right after rc_allocate_device(). This means that while rc_register_device() is being called, those functions can be called from e.g. interrupt handlers.

Currently dev->raw is being allocated and populated by ir_raw_event_prepare() which is called from rc_register_device(). This is not done in a safe way, e.g. dev->raw is being set before the raw members are populated.

Call ir_raw_event_prepare() from rc_allocate_device() instead.

Fixes a3572c34da8d ("V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core")
Signed-off-by Sean Young <[email protected]>
Cc [email protected]
---
 drivers/media/rc/rc-ir-raw.c | 20 +-------------------
 drivers/media/rc/rc-main.c   | 19 +++++++++++--------
 2 files changed, 12 insertions(+), 27 deletions(-)
drivers/media/rc/rc-ir-raw.c +1 -19
index ba24c2f22d39..46c50f423798 100644
@@ -71,9 +71,6 @@ static int ir_raw_event_thread(void *data)
  */
 int ir_raw_event_store(struct rc_dev *dev, struct ir_raw_event *ev)
 {
-	if (!dev->raw)
-		return -EINVAL;
-
 	dev_dbg(&dev->dev, "sample: (%05dus %s)\n",
 		ev->duration, TO_STR(ev->pulse));
 

@@ -102,9 +99,6 @@ int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse)
 	ktime_t			now;
 	struct ir_raw_event	ev = {};
 
-	if (!dev->raw)
-		return -EINVAL;
-
 	now = ktime_get();
 	ev.duration = ktime_to_us(ktime_sub(now, dev->raw->last_event));
 	ev.pulse = !pulse;

@@ -129,9 +123,6 @@ int ir_raw_event_store_with_timeout(struct rc_dev *dev, struct ir_raw_event *ev)
 	ktime_t		now;
 	int		rc = 0;
 
-	if (!dev->raw)
-		return -EINVAL;
-
 	now = ktime_get();
 
 	spin_lock(&dev->raw->edge_spinlock);

@@ -166,9 +157,6 @@ EXPORT_SYMBOL_GPL(ir_raw_event_store_with_timeout);
  */
 int ir_raw_event_store_with_filter(struct rc_dev *dev, struct ir_raw_event *ev)
 {
-	if (!dev->raw)
-		return -EINVAL;
-
 	/* Ignore spaces in idle mode */
 	if (dev->idle && !ev->pulse)
 		return 0;

@@ -200,9 +188,6 @@ EXPORT_SYMBOL_GPL(ir_raw_event_store_with_filter);
  */
 void ir_raw_event_set_idle(struct rc_dev *dev, bool idle)
 {
-	if (!dev->raw)
-		return;
-
 	dev_dbg(&dev->dev, "%s idle mode\n", idle ? "enter" : "leave");
 
 	if (idle) {

@@ -226,7 +211,7 @@ EXPORT_SYMBOL_GPL(ir_raw_event_set_idle);
  */
 void ir_raw_event_handle(struct rc_dev *dev)
 {
-	if (!dev->raw || !dev->raw->thread)
+	if (!dev->raw->thread)
 		return;
 
 	wake_up_process(dev->raw->thread);

@@ -612,9 +597,6 @@ EXPORT_SYMBOL(ir_raw_encode_carrier);
  */
 int ir_raw_event_prepare(struct rc_dev *dev)
 {
-	if (!dev)
-		return -EINVAL;
-
 	dev->raw = kzalloc_obj(*dev->raw);
 	if (!dev->raw)
 		return -ENOMEM;
drivers/media/rc/rc-main.c +11 -8
index dda3479ea3ad..d93e98189c1a 100644
@@ -1701,14 +1701,24 @@ static const struct device_type rc_dev_type = {
 struct rc_dev *rc_allocate_device(enum rc_driver_type type)
 {
 	struct rc_dev *dev;
+	int ret;
 
 	dev = kzalloc_obj(*dev);
 	if (!dev)
 		return NULL;
 
+	if (type == RC_DRIVER_IR_RAW) {
+		ret = ir_raw_event_prepare(dev);
+		if (ret < 0) {
+			kfree(dev);
+			return NULL;
+		}
+	}
+
 	if (type != RC_DRIVER_IR_RAW_TX) {
 		dev->input_dev = input_allocate_device();
 		if (!dev->input_dev) {
+			ir_raw_event_free(dev);
 			kfree(dev);
 			return NULL;
 		}

@@ -1724,6 +1734,7 @@ struct rc_dev *rc_allocate_device(enum rc_driver_type type)
 		spin_lock_init(&dev->rc_map.lock);
 		spin_lock_init(&dev->keylock);
 	}
+
 	mutex_init(&dev->lock);
 
 	dev->dev.type = &rc_dev_type;

@@ -1917,12 +1928,6 @@ int rc_register_device(struct rc_dev *dev)
 		dev->sysfs_groups[attr++] = &rc_dev_wakeup_filter_attr_grp;
 	dev->sysfs_groups[attr++] = NULL;
 
-	if (dev->driver_type == RC_DRIVER_IR_RAW) {
-		rc = ir_raw_event_prepare(dev);
-		if (rc < 0)
-			goto out_minor;
-	}
-
 	if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
 		rc = rc_prepare_rx_device(dev);
 		if (rc)

@@ -1979,8 +1984,6 @@ int rc_register_device(struct rc_dev *dev)
 out_rx_free:
 	ir_free_table(&dev->rc_map);
 out_raw:
-	ir_raw_event_free(dev);
-out_minor:
 	ida_free(&rc_ida, minor);
 	return rc;
 }
Signature · 1 line
2.55.0
author maintainer
#18 lore raw

PATCH v5 14/15 media: rc: Use after free in ir_raw_event_handle()

In reply to Sean Young

If rc_unregister_device() is called while IR is being processed, then ir_raw_event_handle() could call wake_up_process(dev->raw->thread) after kthread_stop(dev->raw->thread). Avoid use after free by using get_task_struct().

Fixes dccc0c3ddf8f ("media: rc: fix race between unregister and urb/irq callbacks")
Signed-off-by Sean Young <[email protected]>
Cc [email protected]
---
 drivers/media/rc/rc-ir-raw.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)
drivers/media/rc/rc-ir-raw.c +14 -2
index 46c50f423798..86de1b26731d 100644
@@ -630,8 +630,13 @@ int ir_raw_event_register(struct rc_dev *dev)
 
 void ir_raw_event_free(struct rc_dev *dev)
 {
-	kfree(dev->raw);
-	dev->raw = NULL;
+	if (dev->raw) {
+		timer_delete_sync(&dev->raw->edge_handle);
+		if (dev->raw->thread)
+			put_task_struct(dev->raw->thread);
+		kfree(dev->raw);
+		dev->raw = NULL;
+	}
 }
 
 void ir_raw_event_unregister(struct rc_dev *dev)

@@ -641,6 +646,13 @@ void ir_raw_event_unregister(struct rc_dev *dev)
 	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
+	 * may call wake_up_process(dev->raw->thread). Ensure this memory
+	 * is not freed by kthread_stop().
+	 */
+	get_task_struct(dev->raw->thread);
 	kthread_stop(dev->raw->thread);
 	timer_delete_sync(&dev->raw->edge_handle);
Signature · 1 line
2.55.0
author maintainer
#19 lore raw

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

In reply to 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 <[email protected]>
Cc [email protected]
---
 drivers/media/rc/bpf-lirc.c  | 18 +++++++++++++-----
 drivers/media/rc/rc-ir-raw.c | 11 +++--------
 2 files changed, 16 insertions(+), 13 deletions(-)
drivers/media/rc/bpf-lirc.c +13 -5
index 2f7564f26445..14ab611e7445 100644
@@ -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;
drivers/media/rc/rc-ir-raw.c +3 -8
index 86de1b26731d..54b323becb1f 100644
@@ -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);
 }
Signature · 1 line
2.55.0
AI review
#20 lore raw

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

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

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 <[email protected]>

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.

drivers/media/rc/bpf-lirc.c static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog) · line 151

Quoted 11 lines
drivers/media/rc/bpf-lirc.c +1 -2
index 2f7564f26445c..14ab611e74457 100644
@@ -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?

Quoted 4 lines
drivers/media/rc/bpf-lirc.c +0 -0
 		ret = -ENODEV;
 		goto unlock;
 	}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15
Reviewed-by on the v5 series
#21 lore raw
In reply to Sean Young

Hi Sean,

For this series:

Reviewed-by Hans Verkuil <[email protected]>

Regards,

Hans

Quoted 55 lines · 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(-)