Re: [RFC PATCH 09/20] rv: Handle unregistered monitors safely in tracefs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The validation check `rv_is_monitor_registered` is vulnerable to the ABA problem, allowing operations on a stale file descriptor to improperly manipulate a newly registered monitor.
--

commit abab0e30c495cd011906fb219545b8b62d268a11
Author: Gabriele Monaco <gmonaco@redhat.com>

rv: Handle unregistered monitors safely in tracefs

This patch adds checks to ensure that a tracefs file descriptor's private_data
still points to a registered monitor before allowing read or write operations.
It aims to prevent use-after-free scenarios when pluggable BPF monitors are
dynamically unregistered while their files are still open.

> diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
> index 33919c5fbd6ad..6ddd3b92da1db 100644
> --- a/kernel/trace/rv/rv.c
> +++ b/kernel/trace/rv/rv.c
> @@ -245,13 +245,31 @@ struct rv_monitor *rv_get_monitor_by_name(const char *name)
>  /*
>   * This section collects the monitor/ files and folders.
>   */
> +
> +bool rv_is_monitor_registered(struct rv_monitor *mon)
> +{
> +	struct rv_monitor *m;
> +
> +	list_for_each_entry_rcu(m, &rv_monitors_list, list,
> +				lockdep_is_held(&rv_interface_lock)) {
> +		if (m == mon)
> +			return true;

[Severity: Medium]
Is it possible for a newly registered monitor to be allocated at the exact
same memory address as a previously unregistered monitor?

If a privileged user opens a monitor's tracefs file, the monitor is then 
unloaded (freeing its memory), and a subsequently loaded BPF monitor happens
to be allocated at that exact same memory address, it appears this pointer 
equality check would incorrectly evaluate to true for the stale file descriptor.

> +	}
> +	return false;
> +}

[ ... ]

> @@ -384,6 +402,9 @@ static ssize_t monitor_enable_write_data(struct file *filp, const char __user *u
>  
>  	guard(mutex)(&rv_interface_lock);
>  
> +	if (!rv_is_monitor_registered(mon))
> +		return -ENODEV;
> +
>  	if (val)
>  		retval = rv_enable_monitor(mon);

[Severity: Medium]
If the registration check passes due to the reallocation scenario described
above, could a write to a stale file descriptor improperly enable or disable
the newly registered, unrelated monitor?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831090524.106845-1-gmonaco@redhat.com?part=9