Fix leaks in rc core
Subsystems (4)
Sean Young
maintainerv2 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
Sean Young
maintainerPATCH 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.
--- drivers/media/rc/streamzap.c | 1 + 1 file changed, 1 insertion(+)
drivers/media/rc/streamzap.c +1 -0
@@ -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
Sean Young
maintainerPATCH v2 2/11 media: redrat3: Ensure rc device is freed if enable_detector() fails
In reply to Sean YoungThis particular error path does not free the rc device at all, with its priv pointer still pointing at freed memory.
--- drivers/media/rc/redrat3.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/media/rc/redrat3.c +4 -1
@@ -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
Sean Young
maintainerPATCH v2 3/11 media: redrat3: Ensure we don't read beyond the end of the packet
In reply to Sean YoungThe length and offset is provided by the usb device, so it should be validated.
--- drivers/media/rc/redrat3.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
drivers/media/rc/redrat3.c +16 -0
@@ -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
Sean Young
maintainerPATCH v2 4/11 media: redrat3: Ensure all urbs are suspended
In reply to Sean YoungEnsure that the learn urb is stopped before suspend.
--- drivers/media/rc/redrat3.c | 1 + 1 file changed, 1 insertion(+)
drivers/media/rc/redrat3.c +1 -0
@@ -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
Sean Young
maintainerPATCH v2 5/11 media: redrat3: Error path leaves device in transmitting state
In reply to Sean YoungIf 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
@@ -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
Sean Young
maintainerPATCH v2 6/11 media: sunxi-cir: Ensure no more interrupts can occur before free
In reply to Sean YoungOnly call rc_free_device() once the hardware has been stopped.
--- drivers/media/rc/sunxi-cir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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); }static void sunxi_ir_shutdown (struct platform_device *pdev)
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [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
Sean Young
maintainerPATCH v2 7/11 media: meson-ir-tx: Ensure clock is disabled on unbind
In reply to Sean Youngclk_prepare_enable() needs a call to clk_disable_unprepare() on driver unbind. Make it devm managed.
--- drivers/media/rc/meson-ir-tx.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
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 ;
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [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
[ ... ]
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
-0return 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
[ ... ]
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
Sean Young
maintainerPATCH v2 8/11 media: meson-ir-tx: Ensure rc_free_device() is called on unbind
In reply to Sean YoungMake rc_dev devm managed.
--- drivers/media/rc/meson-ir-tx.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/media/rc/meson-ir-tx.c +2 -4
@@ -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
Sean Young
maintainerPATCH v2 9/11 media: meson-ir-tx: Ensure probe error is propagated
In reply to Sean Youngdevm_clk_get_enabled() may return -EPROBE_DEFER which needs to be propagated else the probe will not be deferred, it will fail instead.
--- drivers/media/rc/meson-ir-tx.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/rc/meson-ir-tx.c +2 -2
@@ -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
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")Signed-off-by Sean Young <[email protected]>
Thank you for including this one!
Sean Young
maintainerPATCH v2 10/11 media: ir-hix5hd2: Ensure rdev is setup before interrupts are enabled
In reply to Sean YoungOnce the interrupt handler is enabled, priv->rdev can be used. Ensure it is setup correctly so there is no race condition.
--- drivers/media/rc/ir-hix5hd2.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/media/rc/ir-hix5hd2.c +3 -2
@@ -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
Sean Young
maintainerPATCH v2 11/11 media: rc: Use after free in ir_raw_event_handle()
In reply to Sean YoungIf 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).
--- drivers/media/rc/rc-ir-raw.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/media/rc/rc-ir-raw.c +7 -2
@@ -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