Re: [PATCH v5 15/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:

Pre-existing issues:
- [High] If `rc_register_device()` fails after making the LIRC cdev available, `dev->registered` is left as `true`. This allows a concurrent user holding an open file descriptor to bypass the registration check and trigger driver callbacks on freed private data.
--

--- Patch [15]: [PATCH v5 15/15] media: rc: Fix use after free in bpf progs ---
commit 56728bdc06bc2bfe04a9a68caffbbf3e5379fc20
Author: Sean Young <sean@mess.org>

media: rc: Fix use after free in bpf progs

This commit fixes a potential use-after-free in BPF programs by checking
rcdev->registered instead of rcdev->raw during BPF attach, detach, and
query operations. It also updates the raw event handler cleanup logic
during device unregistration.

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

[Severity: High]
This is a pre-existing issue, but does relying on rcdev->registered completely
prevent use-after-free scenarios if the device registration fails?

Looking at rc_register_device() in drivers/media/rc/rc-main.c, if an error
occurs after making the LIRC cdev available, dev->registered is left as true
in the error unwind path:

drivers/media/rc/rc-main.c:rc_register_device() {
        ...
        dev->registered = true;
        ...
        if (error)
                goto out_rx;
        ...
out_rx:
        rc_free_rx_device(dev);
out_lirc:
        if (dev->allowed_protocols != RC_PROTO_BIT_CEC)
                lirc_unregister(dev);
        ...
}

If a user-space process opens the newly registered /dev/lircX device before
rc_register_device() completes and fails, could it bypass this new check since
dev->registered remains true?

Similarly, could this cause issues in lirc_transmit() (in
drivers/media/rc/lirc_dev.c) where it also checks if (!dev->registered),
potentially allowing a freed driver's tx_ir callback to be invoked?

>  		ret = -ENODEV;
>  		goto unlock;
>  	}
>

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