Skip to contentSkip to conversation

Fix leaks in rc core

PATCH Superseded Sean Young 5 participants 30 messages Jul 27 – Jul 29

Subsystems (9)

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Sean Young

author maintainer
#0 lore raw

v4 cover Fix leaks in rc core

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

PATCH v4 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")
---
 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 v4 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")
---
 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 v4 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")
---
 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 v4 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")
---
 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 v4 5/15 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.

Fixes fac59136bc22 ("[media] RedRat3: Return directly after a failed kcalloc() in redrat3_transmit_ir()")"
---
 drivers/media/rc/redrat3.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/media/rc/redrat3.c +3 -3
index 994d4864520c..bc75f68ca535 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
#6 lore raw

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

Quoted 2 lines

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

See also once more:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc5#n94

…

Quoted 2 lines

---
drivers/media/rc/redrat3.c | 6 +++---…

Some contributors would appreciate patch version descriptions.
https://lore.kernel.org/all/?q=%22This+looks+like+a+new+version+of+a+previously+submitted+patch%22
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc5#n310

Regards,
Markus

author maintainer
#7 lore raw

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

Quoted 5 lines · Markus Elfring
Quoted 2 lines

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

See also once more:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc5#n94

I've tried to re-write the message in imperative mood, but doesn't look any better. It can be improved though, how about:

In the allocation failure error path, the transmitting boolean is not cleared and the transmitter cannot be used until the device is unplugged or the module reloaded.

Is that better?

(I don't think we need a v5 just for a change of the commit message before commit.)

Quoted 7 lines

…

Quoted 2 lines

---
drivers/media/rc/redrat3.c | 6 +++---…

Some contributors would appreciate patch version descriptions.
https://lore.kernel.org/all/?q=%22This+looks+like+a+new+version+of+a+previously+submitted+patch%22
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc5#n310

This patch has not changed since earlier versions.

Sean

#8 lore raw

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

Quoted 6 lines
Quoted 5 lines
Quoted 2 lines

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

See also once more:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc5#n94

Quoted 8 lines

I've tried to re-write the message in imperative mood, but doesn't look any better. It can be improved though, how about:

In the allocation failure error path, the transmitting boolean is not cleared and the transmitter cannot be used until the device is unplugged or the module reloaded.

Is that better?

Not really according to known wording requirements.

* Which allocation would have failed here?

* How do you think about to add a wording like the following to the introduction paragraph?

Thus move an assignment statement.

Quoted 4 lines

(I don't think we need a v5 just for a change of the commit message before commit.)

Quoted 1 line

…

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

---
drivers/media/rc/redrat3.c | 6 +++---…

Some contributors would appreciate patch version descriptions.
https://lore.kernel.org/all/?q=%22This+looks+like+a+new+version+of+a+previously+submitted+patch%22
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.2-rc5#n310

This patch has not changed since earlier versions.

Can the running patch review trigger further extensions for the changelog (in the cover letter)?
https://lore.kernel.org/linux-media/[email protected]/

Regards,
Markus

author maintainer
#9 lore raw

PATCH v4 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")
---
 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
#10 lore raw

Re: [PATCH v4 06/15] 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 patch attempts to fix a use-after-free by delaying rc_free_device() until after sunxi_ir_hw_exit(). However, sunxi_ir_hw_exit() only disables the hardware clocks and asserts reset; it does not mask the devm-managed IRQ or wait for in-flight interrupt handlers to complete via synchronize_irq(). This leaves the driver vulnerable to two fatal issues: 1) If an IRQ handler is executing concurrently, disabling the APB clock while it accesses memory-mapped registers will cause a synchronous external abort (bus hang). 2) If the handler continues executing after sunxi_ir_hw_exit() returns, it will access ir->rc while it is being freed by rc_free_device(), resulting in a use-after-free.
- [High] The IRQ is requested via devm_request_irq() before the hardware clocks are initialized by sunxi_ir_hw_init(), risking a synchronous external abort (bus hang) on spurious or pending interrupts during probe.
- [High] The devm-managed IRQ handler can execute and access a freed RC device if sunxi_ir_hw_init() fails in sunxi_ir_probe().
--

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

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

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 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 sunxi_ir_hw_exit() adequately prevent further interrupts from executing?

sunxi_ir_hw_exit() disables clocks and asserts reset, but it doesn't appear to mask the devm-managed IRQ or wait for in-flight handlers to finish via synchronize_irq().

If an interrupt fires concurrently or an IRQ handler is already running:

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

Could disabling the APB clock while this handler is accessing memory-mapped registers cause a synchronous external abort? Furthermore, if the handler continues executing after sunxi_ir_hw_exit() returns, could it access the freed ir->rc device pointer?

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

Quoted 1 line
drivers/media/rc/sunxi-cir.c +0 -0
 }

Looking at the probe function in the same file:

drivers/media/rc/sunxi-cir.c:sunxi_ir_probe() {
    ...
    ret = devm_request_irq(dev, ir->irq, sunxi_ir_irq, 0, SUNXI_IR_DEV, ir);
    if (ret) {
        dev_err(dev, "failed request irq\n");
        goto exit_unregister_dev;
    }

    ret = sunxi_ir_hw_init(dev);
    if (ret)
        goto exit_unregister_dev;
    ...
exit_unregister_dev:
    rc_unregister_device(ir->rc);

exit_free_dev:

    rc_free_device(ir->rc);
}

[Severity: High]
This is a pre-existing issue, but is it safe to request the IRQ before the hardware clocks are initialized?

If a pending interrupt from the bootloader or a spurious IRQ triggers sunxi_ir_irq() before sunxi_ir_hw_init() enables the clock, could this lead to a synchronous external abort when the handler reads ir->base?

[Severity: High]
This is another pre-existing issue, but does the error path for a failed sunxi_ir_hw_init() leave the devm-managed IRQ active until the probe function returns?

If an interrupt fires during this window, could the handler dereference ir->rc after it has been freed by rc_free_device() at the exit_free_dev label?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
author maintainer
#11 lore raw

PATCH v4 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")
---
 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
#12 lore raw

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

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

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

Pre-existing issues:
- [High] Memory leak of the rc_dev struct on driver unbind due to mixing unmanaged allocation with managed registration.
- [High] Integer underflow and division by zero when configuring high carrier frequencies.
- [High] Lockless check of completion_done in the IRQ handler exposes a race that can overflow the hardware FIFO.
- [Medium] Error codes from core kernel APIs are hardcoded to -ENODEV, breaking probe deferral.
--

--- Patch [7]: [PATCH v4 07/15] media: meson-ir-tx: Ensure clock is disabled on unbind --- commit b425f7e336876fa07197476c4560b56dec0eb33d
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 5 lines
drivers/media/rc/meson-ir-tx.c +0 -0
index fded2c256f2a6..e7bb107e6a843 100644

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

Quoted 10 lines
drivers/media/rc/meson-ir-tx.c +2 -2
@@ -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;

[Severity: Medium]
This isn't a bug introduced by this patch, but returning -ENODEV here prevents proper probe deferral. If the clock is not yet ready and devm_clk_get_enabled() returns -EPROBE_DEFER, the initialization will permanently fail instead of trying again later.

Also in meson_irtx_probe(), the same pattern exists for the IRQ:

drivers/media/rc/meson-ir-tx.c:meson_irtx_probe() {
    ...
	irq = platform_get_irq(pdev, 0);
	if (irq < 0)
		return -ENODEV;
    ...
}

Should these functions return the exact error pointer (e.g. PTR_ERR(clock)) or error code rather than hardcoding -ENODEV?

[Severity: High]
This is a pre-existing issue, but does this code cause an integer underflow and division by zero when configuring high carrier frequencies?

drivers/media/rc/meson-ir-tx.c:meson_irtx_set_mod() {
    ...
	unsigned int cnt = DIV_ROUND_CLOSEST(ir->clk_rate, ir->carrier);
	unsigned int pulse_cnt = DIV_ROUND_CLOSEST(cnt * ir->duty_cycle, 100);
	unsigned int space_cnt = cnt - pulse_cnt;

	dev_dbg(ir->dev, "F_mod = %uHz, T_mod = %luns, duty_cycle = %u%%\n",
		ir->carrier, NSEC_PER_SEC / ir->clk_rate * cnt,
		100 * pulse_cnt / cnt);
    ...
}

If a user space process provides a carrier frequency greater than twice the modulator clock rate (ir->clk_rate), cnt will be 0. This results in pulse_cnt being 0, which triggers a division by zero in the dev_dbg() statement.

Could this also cause an underflow when writing to the hardware register via IRB_MOD_COUNT(pulse_cnt, space_cnt)?

[Severity: High]
This is a pre-existing issue, but does the lockless check of completion_done() in the IRQ handler expose a race condition?

drivers/media/rc/meson-ir-tx.c:meson_irtx_irqhandler() {
    ...
	if (completion_done(&ir->completion))
		return IRQ_HANDLED;

	spin_lock_irqsave(&ir->lock, flags);
	if (ir->buf_head < ir->buf_len)
		meson_irtx_send_buffer(ir);
	else
		complete(&ir->completion);
	spin_unlock_irqrestore(&ir->lock, flags);
    ...
}

If an edge interrupt for the FIFO threshold is delayed, and a new transmission begins by acquiring the lock and reinitializing the completion, could the delayed IRQ handler evaluate completion_done() outside the lock, see it as false, and then blindly write up to 96 more words, overflowing the hardware FIFO?

[Severity: High]
This is a pre-existing issue, but does this code leak the rc_dev structure on driver unbind?

drivers/media/rc/meson-ir-tx.c:meson_irtx_probe() {
    ...
	rc = rc_allocate_device(RC_DRIVER_IR_RAW_TX);
	if (!rc)
		return -ENOMEM;
    ...
	ret = devm_rc_register_device(dev, rc);
	if (ret < 0) {
		rc_free_device(rc);
		return dev_err_probe(dev, ret, "rc_dev registration failed\n");
	}
    ...
}

Because rc is allocated with rc_allocate_device() instead of a managed variant like devm_rc_allocate_device(), when the driver is unbound, devm_rc_register_device() will automatically unregister the device, but the memory for the rc_dev structure is never freed since there is no remove callback.

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

PATCH v4 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")
---
 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
maintainer Reviewed-by on v4 8/15
#14 lore raw

Re: [PATCH v4 08/15] media: meson-ir-tx: Ensure rc_free_device() is called on unbind

Quoted 25 lines · 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 +1 -1
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;

drivers/media/rc/meson-ir-tx.c static int meson_irtx_probe(struct platform_device *pdev) · line 365

Quoted 10 lines
drivers/media/rc/meson-ir-tx.c +1 -3
 
  	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;
  }

Thanks,
Neil

author maintainer
#15 lore raw

PATCH v4 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")
---
 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
maintainer Reviewed-by on v4 9/15
#16 lore raw

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

Quoted 25 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")
---
  drivers/media/rc/meson-ir-tx.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/rc/meson-ir-tx.c +1 -1
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;

drivers/media/rc/meson-ir-tx.c static int meson_irtx_probe(struct platform_device *pdev) · line 330

Quoted 8 lines
drivers/media/rc/meson-ir-tx.c +1 -1
@@ -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;

Thanks,
Neil

author maintainer
#17 lore raw

PATCH v4 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")
---
 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
#18 lore raw

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

Fixes dccc0c3ddf8f ("media: rc: fix race between unregister and urb/irq callbacks")
---
 drivers/media/rc/rc-ir-raw.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/media/rc/rc-ir-raw.c +8 -2
index ba24c2f22d39..96849faafde3 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,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)
Signature · 1 line
2.55.0
#19 lore raw

Re: [PATCH v4 11/15] media: rc: Use after free in ir_raw_event_handle()

Quoted 29 lines · 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 | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/media/rc/rc-ir-raw.c +1 -2
index ba24c2f22d39..96849faafde3 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,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;

drivers/media/rc/rc-ir-raw.c int ir_raw_event_register(struct rc_dev *dev) · line 661

Quoted 10 lines
drivers/media/rc/rc-ir-raw.c +7 -0
+	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)

I think this patch could do with some comments in the code explaining why you need get/put_task_struct.

It's the first time I've seen these functions being used, and I'm not sure why they solve the issue or whether this is the right approach.

Regards,

Hans

author maintainer
#20 lore raw

PATCH v4 12/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")
---
 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 96849faafde3..b32f3cff9401 100644
@@ -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);
 }
 
Signature · 1 line
2.55.0
#21 lore raw

Re: [PATCH v4 12/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: - [High] Replacing the `!raw` check with `!rcdev->registered` introduces a reproducible NULL pointer…

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

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.

Quoted 8 lines
drivers/media/rc/bpf-lirc.c +0 -0
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;
 

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

Quoted 10 lines
drivers/media/rc/bpf-lirc.c +3 -2
-	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?

[ ... ]

Quoted 4 lines
drivers/media/rc/bpf-lirc.c +0 -0
@@ -186,12 +187,13 @@ static int lirc_bpf_detach(struct rc_dev *rcdev, struct bpf_prog *prog)
 	if (ret)
 		return ret;
 

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

Quoted 10 lines
drivers/media/rc/bpf-lirc.c +3 -2
-	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()?

[ ... ]

drivers/media/rc/bpf-lirc.c int lirc_prog_query(const union bpf_attr *attr, union bpf_attr __user *uattr) · line 327

Quoted 10 lines
drivers/media/rc/bpf-lirc.c +5 -0
@@ -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?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12
author maintainer
#22 lore raw

PATCH v4 13/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.

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

PATCH v4 14/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.

---
 drivers/media/pci/saa7134/saa7134-input.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/media/pci/saa7134/saa7134-input.c +2 -6
index 7f6680de3156..81620e629449 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,11 +793,6 @@ 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;
-		rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
-	}
-
 	rc->device_name = saa7134_boards[dev->board].name;
 	rc->input_phys = ir->phys;
 	rc->input_id.bustype = BUS_PCI;
Signature · 1 line
2.55.0
author maintainer
#24 lore raw

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

In reply to Sean Young

The correct sequence for rc core is so:

rc_allocation_device()

/*
* setup hardware and now calls to ir_raw_event_store etc are
* permitted, as well as rc_keydown. There is no rc device in sysfs
* or lirc chardev.
*/
rc_register_device()

/*
* After rc_register_device(), the rc device can be used now from lirc
* chardev, sysfs and IR is now decoded and reported.
*/

rc_unregister_device()
/*
* User space can no longer access /dev/lirc or the rc sysfs
* attributes. They will get -ENODEV if they still have a file
* descriptor open.
* Calls to ir_raw_event_handle() etc or rc_keydown() are permitted
* but they must stop before the call to rc_free_device(), so
* this is the time to stop the hardware.
*/
rc_free_device()

This means that during rc_register_device(), we can get calls to ir_raw_event_{store,handle,overflow,store_with_filter}. Ensure this is done race-free.

Fixes a3572c34da8d ("V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core")
---
 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 b32f3cff9401..bebb45ffa971 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
#25 lore raw

Re: [PATCH v4 15/15] media: rc: Fix race condition during rc_register_device()

Quoted 152 lines · Sean Young

The correct sequence for rc core is so:

rc_allocation_device()

/*
* setup hardware and now calls to ir_raw_event_store etc are
* permitted, as well as rc_keydown. There is no rc device in sysfs
* or lirc chardev.
*/
rc_register_device()

/*
* After rc_register_device(), the rc device can be used now from lirc
* chardev, sysfs and IR is now decoded and reported.
*/

rc_unregister_device()
/*
* User space can no longer access /dev/lirc or the rc sysfs
* attributes. They will get -ENODEV if they still have a file
* descriptor open.
* Calls to ir_raw_event_handle() etc or rc_keydown() are permitted
* but they must stop before the call to rc_free_device(), so
* this is the time to stop the hardware.
*/
rc_free_device()

This means that during rc_register_device(), we can get calls to ir_raw_event_{store,handle,overflow,store_with_filter}. Ensure this is done race-free.

Fixes a3572c34da8d ("V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core")
---
 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 b32f3cff9401..bebb45ffa971 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 -0
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;

drivers/media/rc/rc-main.c int rc_register_device(struct rc_dev *dev) · line 1933

Quoted 10 lines
drivers/media/rc/rc-main.c +0 -6
 
-	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)

I don't see what the race condition is you are trying to solve. AFAICT the sysfs files aren't created until the device_add() function that is right after this line.

I'm probably missing something, but that would mean that the commit log needs to be improved.

Regards,

Hans

Quoted 9 lines
drivers/media/rc/rc-main.c +0 -2
@@ -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;
 }
author maintainer
#26 lore raw

Re: [PATCH v4 15/15] media: rc: Fix race condition during rc_register_device()

Quoted 167 lines · Hans Verkuil
Quoted 152 lines · Sean Young

The correct sequence for rc core is so:

rc_allocation_device()

/*
* setup hardware and now calls to ir_raw_event_store etc are
* permitted, as well as rc_keydown. There is no rc device in sysfs
* or lirc chardev.
*/
rc_register_device()

/*
* After rc_register_device(), the rc device can be used now from lirc
* chardev, sysfs and IR is now decoded and reported.
*/

rc_unregister_device()
/*
* User space can no longer access /dev/lirc or the rc sysfs
* attributes. They will get -ENODEV if they still have a file
* descriptor open.
* Calls to ir_raw_event_handle() etc or rc_keydown() are permitted
* but they must stop before the call to rc_free_device(), so
* this is the time to stop the hardware.
*/
rc_free_device()

This means that during rc_register_device(), we can get calls to ir_raw_event_{store,handle,overflow,store_with_filter}. Ensure this is done race-free.

Fixes a3572c34da8d ("V4L/DVB: ir-core: Add logic to decode IR protocols at the IR core")
---
 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 b32f3cff9401..bebb45ffa971 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 -0
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;

drivers/media/rc/rc-main.c int rc_register_device(struct rc_dev *dev) · line 1933

Quoted 10 lines
drivers/media/rc/rc-main.c +0 -6
 
-	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)

I don't see what the race condition is you are trying to solve. AFAICT the sysfs files aren't created until the device_add() function that is right after this line.

Quoted 2 lines

I'm probably missing something, but that would mean that the commit log needs to be improved.

You're right, the commit message is terrible. I've reworded it for v5.

Sean

#27 lore raw

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

In reply to Sean Young

…

Quoted 4 lines

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

…

How do you think about to reorder desirable adjustments for affected modules?

Regards,
Markus

author maintainer
#28 lore raw

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

Quoted 8 lines · Markus Elfring

…

Quoted 4 lines

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

…

How do you think about to reorder desirable adjustments for affected modules?

I'm not entirely sure what you mean. Do you mean just sort the commits so that media: rc: .. sits next to each other?

Sean

#29 lore raw

Re: [v4 00/15] media: Fix leaks in rc core

Quoted 11 lines
Quoted 8 lines

…

Quoted 4 lines

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

…

How do you think about to reorder desirable adjustments for affected modules?

I'm not entirely sure what you mean. Do you mean just sort the commits so that media: rc: .. sits next to each other?

Yes (for example).

Regards,
Markus