Re: [PATCH v3 12/13] media: rc: Fix use after free in bpf progs

[sashiko-bot@kernel.org]

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 <sean@mess.org>

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.

> diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
> index 2f7564f26445c..14ab611e74457 100644
> --- a/drivers/media/rc/bpf-lirc.c
> +++ b/drivers/media/rc/bpf-lirc.c
> @@ -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);

[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.

[ ... ]

> @@ -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);

[Severity: High]
Could the same NULL pointer dereference occur here in lirc_bpf_detach() if
userspace races with device registration failure?

[ ... ]

> @@ -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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784715737.git.sean@mess.org?part=12