Skip to contentSkip to conversation

Fix leaks in rc core

PATCH Superseded Sean Young 3 participants 15 messages Jul 13 – Jul 16

Subsystems (4)

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Sean Young

author maintainer
#0 lore raw

v2 cover Fix leaks in rc core

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 (11):
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()

 drivers/media/rc/ir-hix5hd2.c  |  5 +++--
 drivers/media/rc/meson-ir-tx.c | 14 ++++++--------
 drivers/media/rc/rc-ir-raw.c   |  8 ++++++--
 drivers/media/rc/redrat3.c     | 28 ++++++++++++++++++++++++----
 drivers/media/rc/streamzap.c   |  1 +
 drivers/media/rc/sunxi-cir.c   |  2 +-
 6 files changed, 41 insertions(+), 17 deletions(-)
Signature · 1 line
2.55.0
author maintainer
#1 lore raw

PATCH v2 1/11 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")
---
 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 v2 2/11 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")
---
 drivers/media/rc/redrat3.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/media/rc/redrat3.c +4 -1
index 3f828a564e19..391d01143491 100644
@@ -1111,17 +1111,20 @@ static int redrat3_dev_probe(struct usb_interface *intf,
 	/* 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(rr3->rc);
 led_free:
 	led_classdev_unregister(&rr3->led);
 redrat_free:
 	redrat3_delete(rr3, rr3->udev);
+	rc_free_device(rr3->rc);
 
 no_endpoints:
 	return retval;
Signature · 1 line
2.55.0
author maintainer
#3 lore raw

PATCH v2 3/11 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")
---
 drivers/media/rc/redrat3.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
drivers/media/rc/redrat3.c +16 -0
index 391d01143491..44678ee322c2 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 v2 4/11 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")
---
 drivers/media/rc/redrat3.c | 1 +
 1 file changed, 1 insertion(+)
drivers/media/rc/redrat3.c +1 -0
index 44678ee322c2..468907d6c3a8 100644
@@ -1167,6 +1167,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 v2 5/11 media: redrat3: Error path leaves device in transmitting state

In reply to Sean Young

If the allocation fails, transmitting is left as true and the transmitter cannot be used any more.

---
 drivers/media/rc/redrat3.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/media/rc/redrat3.c +3 -3
index 468907d6c3a8..b64c6440b062 100644
@@ -781,9 +781,6 @@ static int redrat3_transmit_ir(struct rc_dev *rcdev, unsigned *txbuf,
 	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 +791,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
author maintainer
#6 lore raw

PATCH v2 6/11 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")
---
 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
#7 lore raw

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

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

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

Pre-existing issues:
- [High] The teardown sequence fails to synchronize with or disable the active devm-managed IRQ handler, leaving a race window that guarantees either a fatal bus error (unclocked register access) or a Use-After-Free.
- [High] Device interfaces and the IRQ handler are registered before hardware initialization in probe(), allowing userspace or interrupts to access unclocked registers and cause a bus fault.
- [High] The suspend callback disables clocks without synchronizing or disabling the device interrupt, allowing an unclocked MMIO access race.
--

--- Patch [6]: [PATCH v2 06/11] media: sunxi-cir: Ensure no more interrupts can occur before free --- commit 2912f57e54d0f869a5daead9e5ddf88c0db3d980
Author: Sean Young <[email protected]>
media: sunxi-cir: Ensure no more interrupts can occur before free

This commit attempts to fix a race condition during module removal by moving the call to rc_free_device() after sunxi_ir_hw_exit().

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

Quoted 10 lines
drivers/media/rc/sunxi-cir.c +1 -1
@@ -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 isn't a bug introduced by this patch, but does the teardown sequence leave a race window with the devm-managed IRQ handler?

The commit message mentions ensuring no more interrupts can occur before free, but sunxi_ir_hw_exit() only disables the clocks and asserts reset:

sunxi_ir_hw_exit() {
        clk_disable_unprepare(ir->clk);
        clk_disable_unprepare(ir->apb_clk);
        reset_control_assert(ir->rst);
}

Because the IRQ is managed by devm_request_irq(), it remains active until after sunxi_ir_remove() returns. If an interrupt fires concurrently after the clocks are disabled, sunxi_ir_irq() will attempt to access unclocked MMIO registers:

sunxi_ir_irq() {
        ...
        status = readl(ir->base + SUNXI_IR_RXSTA_REG);

Could this lead to a synchronous external abort? Should there be an explicit disable_irq() or similar synchronization before disabling the clocks?

Additionally, this same pattern appears to exist in sunxi_ir_suspend(), which also calls sunxi_ir_hw_exit() while the IRQ is still globally active.

[Severity: High]
This is also a pre-existing issue, but is there a similar unclocked access risk during driver initialization?

In sunxi_ir_probe(), rc_register_device() and devm_request_irq() are called before the hardware clocks are enabled via sunxi_ir_hw_init():

sunxi_ir_probe() {
        ...
        ret = rc_register_device(ir->rc);
        ...
        ret = devm_request_irq(dev, ir->irq, sunxi_ir_irq, 0, SUNXI_IR_DEV, ir);
        ...
        ret = sunxi_ir_hw_init(dev);
}

If userspace immediately issues a LIRC_SET_REC_TIMEOUT ioctl after the device is registered, sunxi_ir_set_timeout() will write to SUNXI_IR_CIR_REG before the clocks are enabled. Similarly, a pending interrupt could cause sunxi_ir_irq() to run before sunxi_ir_hw_init().

Should the hardware initialization be moved before exposing the device and requesting the IRQ?

Quoted 1 line
drivers/media/rc/sunxi-cir.c +0 -0
 }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
author maintainer
#8 lore raw

PATCH v2 7/11 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")
---
 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
#9 lore raw

Re: [PATCH v2 07/11] 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:
- [High] Use-After-Free in the redrat3 probe error path.

Pre-existing issues:
- [High] Incomplete fix for IRQ race condition during remove, leading to kernel panic and use-after-free. --

--- Patch [7]: [PATCH v2 07/11] media: meson-ir-tx: Ensure clock is disabled on unbind --- commit 7936d37b9d5412a12d7a610528273a3bb7e7ada2
Author: Sean Young <[email protected]>

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

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

Quoted 4 lines
drivers/media/rc/redrat3.c +0 -0
index 3f828a564e192..b64c6440b062d 100644

[ ... ]

Quoted 9 lines
drivers/media/rc/redrat3.c +1 -1
@@ -1111,17 +1127,20 @@ static int redrat3_dev_probe(struct usb_interface *intf,
 	/* 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);

drivers/media/rc/redrat3.c static int redrat3_dev_probe(struct usb_interface *intf, · line 1143

Quoted 10 lines
drivers/media/rc/redrat3.c +3 -0
 
 	return 0;
 
+rc_free:
+	rc_unregister_device(rr3->rc);
 led_free:
 	led_classdev_unregister(&rr3->led);
 redrat_free:
 	redrat3_delete(rr3, rr3->udev);
+	rc_free_device(rr3->rc);

[Severity: High]
Can this error path trigger a use-after-free on the rr3 pointer?

When redrat3_delete() is called in the error handling path, it unconditionally frees the rr3 structure:

drivers/media/rc/redrat3.c:redrat3_delete() {
    ...
    kfree(rr3);
}

Directly after this, rc_free_device(rr3->rc) is executed, which dereferences the newly freed rr3 pointer to access the rc member.

Should rc_free_device() be called before redrat3_delete() to avoid this?

Quoted 4 lines
drivers/media/rc/sunxi-cir.c +0 -0
index 28e840a7e5b8c..af1ee08ffdbec 100644

[ ... ]

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

Quoted 8 lines
drivers/media/rc/sunxi-cir.c +1 -1
@@ -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 moving rc_free_device() after sunxi_ir_hw_exit() still leave a race condition that could cause a kernel panic?

Because sunxi_ir_probe() uses devm_request_irq(), the interrupt remains active until devres cleanup executes after sunxi_ir_remove() returns. If an interrupt fires after sunxi_ir_hw_exit() disables the APB clocks but before devres deregisters the IRQ, sunxi_ir_irq() will execute and read from the unclocked SUNXI_IR_RXSTA_REG register.

On ARM architectures, accessing this register when the APB clock is disabled causes a synchronous external abort, resulting in a kernel panic. If the execution somehow proceeds, the handler would also dereference the newly freed ir->rc pointer.

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
#10 lore raw

PATCH v2 8/11 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")
---
 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
#11 lore raw

PATCH v2 9/11 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")
---
 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
reviewer Reviewed-by on v2 9/11
#12 lore raw

Re: [PATCH v2 09/11] media: meson-ir-tx: Ensure probe error is propagated

Quoted 6 lines · 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")

Thank you for including this one!

author maintainer
#13 lore raw

PATCH v2 10/11 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")
---
 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
#14 lore raw

PATCH v2 11/11 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).

Fixes dccc0c3ddf8f ("media: rc: fix race between unregister and urb/irq callbacks")
---
 drivers/media/rc/rc-ir-raw.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/media/rc/rc-ir-raw.c +7 -2
index ba24c2f22d39..f066176c9f37 100644
@@ -637,6 +637,7 @@ int ir_raw_event_register(struct rc_dev *dev)
 	if (IS_ERR(thread))
 		return PTR_ERR(thread);
 
+	get_task_struct(thread);
 	dev->raw->thread = thread;
 
 	mutex_lock(&ir_raw_handler_lock);
@@ -648,8 +649,12 @@ 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) {
+		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)
Signature · 1 line
2.55.0