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

[sashiko-bot@kernel.org]

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