[PATCH] usb: gadget: f_ncm: Fix use-after-free in ncm_wrap_ntb()

[PATCH] usb: gadget: f_ncm: Fix use-after-free in ncm_wrap_ntb()

[Boyan Liu]

The err: path of ncm_wrap_ntb() frees ncm->skb_tx_data and
ncm->skb_tx_ndp but never clears the pointers. The rest of the
file treats NULL as "no aggregation in progress", and the other
release site, package_for_tx(), clears them after freeing.

When the NTH skb allocation succeeds but the NDP skb allocation
fails -- both GFP_ATOMIC, so this happens under memory pressure
-- the err: path frees both skbs and leaves stale pointers.
The next transmit passes the guard on the freed skb_tx_data,
reads ->len from the freed skb, and package_for_tx() may then
write the NTB header through the freed skb's ->data:

    BUG: KASAN: slab-use-after-free in ncm_wrap_ntb (drivers/usb/gadget/function/f_ncm.c:1044)
    Read of size 4 at addr ffff888108582930 by task kworker/1:2/1623
    Call Trace:
     ncm_wrap_ntb (drivers/usb/gadget/function/f_ncm.c:1044)
     eth_start_xmit (drivers/usb/gadget/function/u_ether.c:543)
     __dev_queue_xmit (net/core/dev.c:4884)
    Freed by task 4999:
     napi_skb_cache_put (net/core/skbuff.c:1471)	/* deferred dev_kfree_skb_any() from err: */
     net_tx_action (net/core/dev.c:5854)
    The buggy address belongs to the object at ffff8881085828c0
     which belongs to the cache skbuff_head_cache of size 240

Clear both pointers after freeing so the next transmit
re-enters the allocation branch on a clean state instead of
dereferencing freed skbs.

Fixes: 6d3865f9d41f ("usb: gadget: NCM: Add transmit multi-frame.")
Cc: stable@vger.kernel.org
Reported-by: co+02480a85e1a0d4f5@bugs.sh
Signed-off-by: Boyan Liu <yymhvert@gmail.com>
---
 drivers/usb/gadget/function/f_ncm.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
index 500c9c19c78b..be1c2fa36350 100644
--- a/drivers/usb/gadget/function/f_ncm.c
+++ b/drivers/usb/gadget/function/f_ncm.c
@@ -1132,10 +1132,14 @@ static struct sk_buff *ncm_wrap_ntb(struct gether *port,
 
 	if (skb)
 		dev_kfree_skb_any(skb);
-	if (ncm->skb_tx_data)
+	if (ncm->skb_tx_data) {
 		dev_kfree_skb_any(ncm->skb_tx_data);
-	if (ncm->skb_tx_ndp)
+		ncm->skb_tx_data = NULL;
+	}
+	if (ncm->skb_tx_ndp) {
 		dev_kfree_skb_any(ncm->skb_tx_ndp);
+		ncm->skb_tx_ndp = NULL;
+	}
 
 	return NULL;
 }
-- 
2.43.0

Re: [PATCH] usb: gadget: f_ncm: Fix use-after-free in ncm_wrap_ntb()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: f_ncm: NULL pointer dereference in ncm_wrap_ntb()
- [Medium] usb: gadget: f_ncm: skb2 is leaked on subsequent allocation failure
--

commit 8ffb0eb4a1490f0bf7e457d7a7e0bc30811f90a1
Author: Boyan Liu <yymhvert@gmail.com>

usb: gadget: f_ncm: Fix use-after-free in ncm_wrap_ntb()

This commit addresses a use-after-free by clearing the ncm->skb_tx_data and
ncm->skb_tx_ndp pointers after freeing them in the error path of
ncm_wrap_ntb(). This prevents subsequent transmits from dereferencing the
freed skbs when allocation fails.

> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
> index 500c9c19c78b4..be1c2fa36350b 100644
> --- a/drivers/usb/gadget/function/f_ncm.c
> +++ b/drivers/usb/gadget/function/f_ncm.c
> @@ -1132,10 +1132,14 @@ static struct sk_buff *ncm_wrap_ntb(struct gether *port,
>  
>  	if (skb)
>  		dev_kfree_skb_any(skb);

[Severity: High]
Is it possible for ncm->netdev to be NULL when accessed in this error path?

If ncm_disable() or ncm_set_alt() executes concurrently with
eth_start_xmit(), they clear ncm->netdev locklessly before calling
gether_disconnect():

drivers/usb/gadget/function/f_ncm.c:ncm_disable() {
	if (ncm->netdev) {
		ncm->netdev = NULL;
		gether_disconnect(&ncm->port);
	}
}

Because eth_start_xmit() relies on dev->lock to verify dev->port_usb, it can
proceed to call ncm_wrap_ntb() if it acquires the lock before
gether_disconnect() disables the TX path.

If alloc_skb() then fails under memory pressure inside ncm_wrap_ntb(), the
code jumps to the err label just above this modified block:

	err:
		ncm->netdev->stats.tx_dropped++;

Will this unconditional dereference of ncm->netdev lead to a NULL pointer
dereference?

[Severity: Medium]
Is skb2 leaked in this error handling path?

Earlier in ncm_wrap_ntb(), when an existing batch is full, the function
packages the pending frames into skb2:

	if (ncm->skb_tx_data && (... > max_size)) {
		skb2 = package_for_tx(ncm);
		if (!skb2)
			goto err;
	}

Immediately after, it attempts to allocate a new buffer for the incoming skb:

	if (!ncm->skb_tx_data) {
		...
		ncm->skb_tx_data = alloc_skb(max_size, GFP_ATOMIC);
		if (!ncm->skb_tx_data)
			goto err;

If this alloc_skb() fails, the code jumps to the err label and runs through
the cleanup path modified here. While the incoming skb and the pending
ncm->skb_tx_data and ncm->skb_tx_ndp are freed, skb2 is never handled.

Since the function returns NULL without freeing skb2, is the newly
aggregated buffer leaked?

> -	if (ncm->skb_tx_data)
> +	if (ncm->skb_tx_data) {
>  		dev_kfree_skb_any(ncm->skb_tx_data);
> -	if (ncm->skb_tx_ndp)
> +		ncm->skb_tx_data = NULL;
> +	}
> +	if (ncm->skb_tx_ndp) {
>  		dev_kfree_skb_any(ncm->skb_tx_ndp);
> +		ncm->skb_tx_ndp = NULL;
> +	}
>  
>  	return NULL;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015024508.3179-1-yymhvert@gmail.com?part=1