Re: [PATCH] HID: bpf: serialize device reference release in struct_ops destroy path

[Benjamin Tissoires <bentiss@kernel.org>]

Just for the record, because this broke both our brains for long enough
(Jiri and I)

On Aug 03 2026, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Time-of-check to time-of-use (TOCTOU) race leading to a Use-After-Free of `hdev` during lock acquisition.
> - [High] Early return bypasses `list_del_rcu()` and `synchronize_srcu()`, leaving a freed BPF ops object in the device's active program list.
> --
> 
> --- Patch [1]: [PATCH] HID: bpf: serialize device reference release in struct_ops destroy path ---
> commit cfdd4ce27d8a975674b1f3418e6b5078b5d94946
> Author: Shen Yongchao <grayhat@foxmail.com>
> Subject: HID: bpf: serialize device reference release in struct_ops destroy path
> 
> This patch aims to fix a double-put race condition in the HID BPF struct_ops
> destroy and unreg paths. It serializes the remove/NULL decision under
> hdev->bpf.prog_list_lock to ensure exactly one path releases the device
> reference.
> 
> > diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c
> > index 702c22fae136a..049df06477610 100644
> > --- a/drivers/hid/bpf/hid_bpf_struct_ops.c
> > +++ b/drivers/hid/bpf/hid_bpf_struct_ops.c
> > @@ -250,6 +250,11 @@ static void hid_bpf_unreg(void *kdata, struct bpf_link *link)
> >  
> >  	mutex_lock(&hdev->bpf.prog_list_lock);
> 
> [Severity: High]
> This is a pre-existing issue, but is it possible for hdev to be completely
> freed between being read from ops->hdev and used here in mutex_lock()?
> 
> If hid_bpf_unreg() reads hdev = ops->hdev and is then preempted, could
> __hid_bpf_ops_destroy_device() execute concurrently, clear ops->hdev, and
> drop the registration's device reference via hid_put_device(hdev)?
> 
> If that was the final reference, the device could be freed, leading to a
> use-after-free when the unreg thread resumes and attempts to lock
> &hdev->bpf.prog_list_lock.

This is actually tackled by the commit description.

> 
> > +	if (!ops->hdev) {
> > +		mutex_unlock(&hdev->bpf.prog_list_lock);
> > +		return;
> > +	}
> > +
> 
> [Severity: High]
> This is a pre-existing issue, but does this early return safely handle list
> cleanup?
> 
> In __hid_bpf_ops_destroy_device(), e->hdev = NULL is set, but the node is
> intentionally left linked in hdev->bpf.prog_list.
> 
> If hid_bpf_unreg() runs, sees !ops->hdev under the lock, and returns early
> here, it entirely bypasses list_del_rcu(&ops->list) and
> synchronize_srcu(&hdev->bpf.srcu).
> 
> Since the BPF framework will subsequently free the ops map memory, wouldn't
> this leave a freed object in hdev->bpf.prog_list?
> 
> If a driver .remove callback then triggers hid_bpf_hw_request() (for example,
> to turn off LEDs), it would traverse the list under SRCU and access the freed
> ops object.

Sashiko is wrong here: when we call hid_bpf_destroy(), there is a guard
that is set (.destroyed) which prevents further iterations over the RCU
lists. So the next .remove() call triggered from the device_del() will
not iterate over the freed data.

Cheers,
Benjamin