[PATCH usb-next] usb: gadget: f_ncm: prevent double-free of OS descriptor table

[PATCH usb-next] usb: gadget: f_ncm: prevent double-free of OS descriptor table

[Xinsheng Zhu]

ncm_bind() allocates an OS descriptor table only when
cdev->use_os_string is enabled. However, ncm_unbind() frees the
table and resets f->os_desc_n without clearing f->os_desc_table,
leaving a dangling pointer.

When a ConfigFS gadget is unbound, the function object can be reused
for a subsequent bind. The OS descriptor support can be toggled
through the ConfigFS os_desc/use attribute, which determines
cdev->use_os_string on the next bind. If this flag changes from true
to false between binds, ncm_bind() skips the allocation and leaves
f->os_desc_table pointing to the previously freed memory.

A subsequent unbind may therefore free the stale pointer again,
resulting in a double-free.

Set f->os_desc_table to NULL after freeing it to prevent the stale
pointer from being reused across bind/unbind cycles.

Signed-off-by: Xinsheng Zhu <xinsheng.zhu.ny@gmail.com>
---
 drivers/usb/gadget/function/f_ncm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
index 500c9c19c78b..685152ec9a5d 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -1741,6 +1741,7 @@ static void ncm_unbind(struct usb_configuration *c, struct usb_function *f)
 	hrtimer_cancel(&ncm->task_timer);
 
 	kfree(f->os_desc_table);
+	f->os_desc_table = NULL;
 	f->os_desc_n = 0;
 
 	ncm_string_defs[0].id = 0;

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.54.0 (Apple Git-157)

Re: [PATCH usb-next] usb: gadget: f_ncm: prevent double-free of OS descriptor table

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] usb: gadget: f_ncm: missing Fixes trailer for ncm_unbind()
  double-free fix
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] usb: gadget: f_ncm: memory leak in ncm_unbind() [1]
--

commit 1d3d70b425a5f8ec0afdd69a5b560a6e1867217a
Author: Xinsheng Zhu <xinsheng.zhu.ny@gmail.com>

> usb: gadget: f_ncm: prevent double-free of OS descriptor table
>
> ncm_bind() allocates an OS descriptor table only when
> cdev->use_os_string is enabled. However, ncm_unbind() frees the
> table and resets f->os_desc_n without clearing f->os_desc_table,
> leaving a dangling pointer.
>
> When a ConfigFS gadget is unbound, the function object can be reused
> for a subsequent bind. The OS descriptor support can be toggled
> through the ConfigFS os_desc/use attribute, which determines
> cdev->use_os_string on the next bind. If this flag changes from true
> to false between binds, ncm_bind() skips the allocation and leaves
> f->os_desc_table pointing to the previously freed memory.
>
> A subsequent unbind may therefore free the stale pointer again,
> resulting in a double-free.
>
> Set f->os_desc_table to NULL after freeing it to prevent the stale
> pointer from being reused across bind/unbind cycles.
>
> Signed-off-by: Xinsheng Zhu <xinsheng.zhu.ny@gmail.com>

[Severity: Low]
This is a bug fix for OS descriptor handling in ncm_unbind(). Should the
commit message include:

  Fixes: 793409292382 ("usb: gadget: f_ncm: Add OS descriptor support")

> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
> index 500c9c19c78b4..685152ec9a5d0 100644
> --- a/drivers/usb/gadget/function/f_ncm.c
> +++ b/drivers/usb/gadget/function/f_ncm.c
> @@ -1741,6 +1741,7 @@ static void ncm_unbind(struct usb_configuration *c, struct usb_function *f)
>  	hrtimer_cancel(&ncm->task_timer);
>  
>  	kfree(f->os_desc_table);
> +	f->os_desc_table = NULL;
>  	f->os_desc_n = 0;

[Severity: Medium]
This problem wasn't introduced by this patch, but does ncm_unbind() leak
ncm->skb_tx_data and ncm->skb_tx_ndp [1]?

ncm_wrap_ntb() allocates skb_tx_data and skb_tx_ndp to aggregate outgoing
packets and arms task_timer to flush them. If the gadget is unbound before
task_timer fires or the NTB fills up, hrtimer_cancel(&ncm->task_timer) stops
the pending flush without freeing those skbs, and ncm_free() later frees ncm
directly.

[1] https://sashiko.dev/#/bug/linux-c8af1819-7ebd-4e08-89f6-265e1b0462fd

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109053440.49156-1-xinsheng.zhu.ny@gmail.com?part=1