Fix leaks in rc core
Subsystems (5)
Sean Young
maintainerv3 cover Fix leaks in rc core
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 (13):
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: rc: Fix race condition during rc_register_device()
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 | 40 ++++++++++------------------------ drivers/media/rc/rc-main.c | 18 +++++++++------ drivers/media/rc/redrat3.c | 35 ++++++++++++++++++++++++----- drivers/media/rc/streamzap.c | 1 + drivers/media/rc/sunxi-cir.c | 2 +- 8 files changed, 75 insertions(+), 58 deletions(-)
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v3 1/13 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 v3 2/13 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 | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
drivers/media/rc/redrat3.c +9 -3
@@ -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
Sean Young
maintainerPATCH v3 3/13 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 v3 4/13 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
@@ -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
Sean Young
maintainerPATCH v3 5/13 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 v3 6/13 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 v3 06/13] media: sunxi-cir: Ensure no more interrupts can occur before free
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:
Pre-existing issues:
- [High] The patch attempts to fix a race condition by reordering rc_free_device() after sunxi_ir_hw_exit(), but fails to synchronize or explicitly disable the devm-managed IRQ, leaving the race open.
- [High] The device is registered with the RC core before its hardware clocks are enabled, resulting in MMIO accesses to unclocked registers during probe.
--
--- Patch [6]: [PATCH v3 06/13] media: sunxi-cir: Ensure no more interrupts can occur before free --- commit a8d7020d0e53fc07953c4088225c769898287e76
Author: Sean Young <[email protected]>
media: sunxi-cir: Ensure no more interrupts can occur before free
This commit reorders rc_free_device() in the remove function so that it is called after the hardware is shut down. This is intended to fix a race between device unregistration and the interrupt callback.
drivers/media/rc/sunxi-cir.c static void sunxi_ir_remove(struct platform_device *pdev) · line 379
Quoted 12 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 this actually close the race condition?
In sunxi_ir_remove(), sunxi_ir_hw_exit() disables the hardware clocks. However, if the devm-managed interrupt is not explicitly disabled and synchronized (for example, using disable_irq()), couldn't an interrupt still run concurrently or immediately after clocks are disabled?
If sunxi_ir_irq() fires after sunxi_ir_hw_exit() runs, it will attempt to read SUNXI_IR_RXSTA_REG while the clocks are disabled, which could cause a crash. Additionally, since the IRQ is devm-managed, it could also race with rc_free_device(), leading to a use-after-free on ir->rc.
This same issue appears to exist in sunxi_ir_suspend(), where clocks are disabled while system interrupts are still active.
[Severity: High]
This is a pre-existing issue, but I noticed another potential problem in the probe path. Does sunxi_ir_probe() access unclocked registers during device registration?
In sunxi_ir_probe(), rc_register_device() is called before sunxi_ir_hw_init():
ret = rc_register_device(ir->rc);
if (ret) {
dev_err(dev, "failed to register rc device\n");
goto exit_free_dev;
}
...
ret = sunxi_ir_hw_init(dev);The registration synchronously invokes core callbacks which unconditionally call the driver's protocol change hook, and in turn sunxi_ir_set_timeout(). This callback writes to the SUNXI_IR_CIR_REG MMIO register. Since ir->clk and ir->apb_clk are not enabled until sunxi_ir_hw_init() runs later, won't this cause an abort or bus fault by accessing APB memory-mapped registers without their clocks enabled?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
Sean Young
maintainerPATCH v3 7/13 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 v3 07/13] 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:
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the error code from devm_clk_get_enabled() is unconditionally masked with -ENODEV, which breaks probe deferral.
- [High] This problem wasn't introduced by this patch, but the error return from platform_get_irq() is masked by returning -ENODEV, breaking probe deferral. --
--- Patch [7]: [PATCH v3 07/13] media: meson-ir-tx: Ensure clock is disabled on unbind --- commit 902362d56de8e550d8a16fef5053c172039aa2ab
Author: Sean Young <[email protected]>
Subject: media: meson-ir-tx: Ensure clock is disabled on unbind
This commit replaces the manual devm_clk_get and clk_prepare_enable calls with devm_clk_get_enabled. This ensures that the clock is properly disabled when the driver is unbound.
drivers/media/rc/meson-ir-tx.c static int meson_irtx_mod_clock_probe(struct meson_irtx *ir, · line 293
Quoted 13 lines
drivers/media/rc/meson-ir-tx.c +2
-2@@ -288,8 +288,8 @@ static int meson_irtx_mod_clock_probe(struct meson_irtx *ir,if (!np)return -ENODEV ; - clock =devm_clk_get (ir->dev,"xtal" ); -if (IS_ERR (clock) ||clk_prepare_enable (clock)) + clock =devm_clk_get_enabled (ir->dev,"xtal" ); +if (IS_ERR (clock))return -ENODEV ;
[Severity: High]
This is a pre-existing issue, but does dropping the actual error code from devm_clk_get_enabled() and returning -ENODEV break probe deferral?
If the clock provider isn't ready, devm_clk_get_enabled() returns -EPROBE_DEFER, which gets masked here. Should this return PTR_ERR(clock) instead?
[ ... ]
While reviewing this file, I also noticed another pre-existing issue in the same driver:
drivers/media/rc/meson-ir-tx.c:meson_irtx_probe() {
...
irq = platform_get_irq(pdev, 0);
if (irq < 0)
return -ENODEV;
...
}[Severity: High]
This isn't a bug introduced by this patch, but would returning a hardcoded -ENODEV here break probe deferral if the interrupt controller is not yet ready?
When platform_get_irq() returns -EPROBE_DEFER, it seems the code currently discards it. Would returning the actual error code be more appropriate?
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 v3 8/13 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 v3 9/13 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
Sean Young
maintainerPATCH v3 10/13 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 v3 11/13 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
Sean Young
maintainerPATCH v3 12/13 media: rc: Fix use after free in bpf progs
In reply to Sean YoungSince 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.
--- 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
@@ -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
@@ -650,8 +650,11 @@ int ir_raw_event_register(struct rc_dev *dev)void ir_raw_event_free (struct rc_dev *dev) {if (dev->raw) { +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 ; }
@@ -661,9 +664,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);
@@ -676,11 +676,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
[email protected]
AI reviewRe: [PATCH v3 12/13] 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] NULL pointer dereference in BPF LIRC operations due to missing `!raw` check on device registration…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] NULL pointer dereference in BPF LIRC operations due to missing !raw check on device registration error path. --
--- Patch [12]: [PATCH v3 12/13] media: rc: Fix use after free in bpf progs --- commit 7dc21e94d34583cca33adec11dc3c1b8af880d26
Author: Sean Young <[email protected]>
media: rc: Fix use after free in bpf progs
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.
Quoted 8 lines
drivers/media/rc/bpf-lirc.c +0
-0@@ -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 this lead to a NULL pointer dereference if rc_register_device() fails?
In rc_register_device(), dev->registered is set to true before lirc_register() makes the device visible to userspace. If a subsequent initialization step fails (such as input_register_device() or ir_raw_event_register()), the error path calls ir_raw_event_free(dev), which frees dev->raw and sets it to NULL.
However, dev->registered is never reverted to false. If a concurrent userspace process opens /dev/lircX and calls bpf(BPF_PROG_ATTACH) while the fd is kept open, the !rcdev->registered check here would pass. The raw pointer would then be NULL, causing lirc_rcu_dereference(raw->progs) to crash.
[ ... ]
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]
Could the same NULL pointer dereference occur here in lirc_bpf_detach() if userspace races with device registration failure?
[ ... ]
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]
Does this code also suffer from the same NULL pointer dereference risk on rcdev->raw when rc_register_device() hits an error path?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12
Sean Young
maintainerPATCH v3 13/13 media: rc: Fix race condition during rc_register_device()
In reply to Sean YoungThe 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.
--- drivers/media/rc/rc-ir-raw.c | 20 +------------------- drivers/media/rc/rc-main.c | 18 +++++++++++------- 2 files changed, 12 insertions(+), 26 deletions(-)
drivers/media/rc/rc-ir-raw.c +1 -19
@@ -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 -7
@@ -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)
@@ -1980,7 +1985,6 @@ int rc_register_device(struct rc_dev *dev)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