Re: [PATCH] usb: gadget: f_hid: only clear write_pending from the owning request

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Fri, Aug 14, 2026 at 05:46:16PM +0900, Hyeontae Lee wrote:
> f_hidg_write() sets write_pending, caches hidg->req into a stack local,
> and drops write_spinlock across copy_from_user(). hidg_disable() frees
> hidg->req whenever write_pending is clear, so that flag is the only thing
> keeping the writer's cached pointer alive.
> 
> f_hidg_req_complete() clears it for whichever request completes, not for
> the one the writer owns. A write() that has queued a request leaves it
> queued indefinitely if the host never issues an IN token. When the host
> then disables the configuration, usb_ep_disable() gives that old request
> back, its completion clears write_pending, and hidg_disable() goes on to
> free hidg->req - which a different write() is holding at that moment.
> 
> The writer resumes, finds hidg->req NULL, and frees its cached pointer a
> second time:
> 
>   BUG: KASAN: slab-use-after-free in f_hidg_write+0x7b7/0x920
>   Read of size 8 at addr ffff888104fc9018 by task s4-3/163
> 
>   Allocated by task 0:
>    alloc_ep_req+0x20/0x1b0
>    hidg_set_alt+0x1ed/0xbd0
>    composite_setup+0x1072/0x8690
>    configfs_composite_setup+0xcd/0x110
>    dummy_timer+0x1a68/0x31d0
> 
>   Freed by task 0:
>    kfree+0x121/0x380
>    hidg_disable+0x559/0x7a0
>    reset_config+0x9d/0x200
>    composite_setup+0x32d4/0x8690
>    configfs_composite_setup+0xcd/0x110
>    dummy_timer+0x1a68/0x31d0
> 
>   The buggy address belongs to the object at ffff888104fc9000
>    which belongs to the cache kmalloc-128 of size 128
> 
>   BUG: KASAN: double-free in f_hidg_write+0x267/0x920
> 
> Record which request owns the flag and let only that request's completion
> clear it. An older queued request being given back then leaves
> write_pending set, hidg_disable() declines to free, and the writer frees
> its own request on the existing path.
> 
> Fixes: 25cd9721c2b1 ("usb: gadget: f_hid: fix: Don't access hidg->req without spinlock held")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hyeontae Lee <wonju345@naver.com>
> ---
> Notes for reviewers:
> 
> The clearer was identified by instrumenting the three sites that clear the
> flag: of 4944 frees at hidg_disable(), all 4944 followed a clear from
> f_hidg_req_complete(), and 4440 freed a request a writer was holding.
> 
> 749494b6bdbb introduced the stack local but left the uses on hidg->req, so
> check and use still agreed; 25cd9721c2b1 moved the uses to the local and
> left the guard on the field, which is why that one is tagged.
> 
> Every dereference of the freed pointer is inside f_hidg_write(), so this is
> not remotely triggerable on its own - a local process must be writing to
> /dev/hidgN at the time.
> 
> hidg_disable() now declines to free a request a writer owns; f_hid's request
> lifetime under repeated SET_INTERFACE has pre-existing gaps this does not
> address.
> 
> Tested on v7.2-rc7-12 (f5bbbfec59b4) under dummy_hcd, report_length=8: over
> 45 s of identical workload, double frees of the same address went from 52 to
> 0 and KASAN reports from 208 to 0.

Did you forget to add an Assisted-by: tag?


> ---
>  drivers/usb/gadget/function/f_hid.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_hid.c b/drivers/usb/gadget/function/f_hid.c
> index 3c6b43d06a6d1..d65169c9645b1 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -89,6 +89,7 @@ struct f_hidg {
>  	/* send report */
>  	spinlock_t			write_spinlock;
>  	bool				write_pending;
> +	struct usb_request		*write_req;
>  	wait_queue_head_t		write_queue;
>  	struct usb_request		*req;
>  
> @@ -443,7 +444,16 @@ static void f_hidg_req_complete(struct usb_ep *ep, struct usb_request *req)
>  	}
>  
>  	spin_lock_irqsave(&hidg->write_spinlock, flags);
> -	hidg->write_pending = 0;
> +	/*
> +	 * Only the completion of the request this writer owns may clear the
> +	 * flag. usb_ep_disable() gives back whatever is still queued from an
> +	 * earlier write(), and letting that clear write_pending lets
> +	 * hidg_disable() go on to free a request a writer is still holding.

LLMs love to add comments when it's not needed, be judicious please.

thanks,

greg k-h