Re: [PATCH v4 12/15] 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] 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 <sean@mess.org>

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.

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

[ ... ]

> @@ -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]
Does this face the same NULL pointer dereference issue during the 
rc_register_device() error path as lirc_bpf_attach()?

[ ... ]

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

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