usb: gadget: f_ncm: Fix use-after-free in ncm_wrap_ntb()
Subsystems (2)
PATCH 1/1 usb: gadget: f_ncm: Fix use-after-free in ncm_wrap_ntb()
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 240Clear both pointers after freeing so the next transmit re-enters the allocation branch on a clean state instead of dereferencing freed skbs.
--- drivers/usb/gadget/function/f_ncm.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/usb/gadget/function/f_ncm.c +6 -2
@@ -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 ; }
Signature · 1 line
2.43.0
[email protected]
AI reviewThank 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…
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 <[email protected]>
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.
drivers/usb/gadget/function/f_ncm.c static struct sk_buff *ncm_wrap_ntb(struct gether *port, · line 1134
Quoted 8 lines
drivers/usb/gadget/function/f_ncm.c +0
-0@@ -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?
Quoted 13 lines
drivers/usb/gadget/function/f_ncm.c +6
-2-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 ; }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Hi,
Both findings are real bugs, but both predate this patch; the use-after-free fix itself is unaffected.
[High] ncm->netdev NULL dereference in the err: path: agreed, the window exists -- ncm_disable() clears ncm->netdev locklessly, and ncm_tx_timeout() already treats it as possibly NULL (READ_ONCE + check). It additionally needs an alloc_skb() failure inside that window, so the race is narrow. Pre-existing; a fix (guard the stats update in the err: path, or move the clear after gether_disconnect()) can be sent separately and does not block this patch.
[Medium] skb2 leak in the err: path: agreed, also pre-existing. Since this patch already rewrites that block, I can fold
if (skb2) dev_kfree_skb_any(skb2);
into a v2, or it can go as a separate follow-up -- whichever Greg prefers.
Thanks,
Boyan
Hi Greg,
I wanted to follow up on this fix. The use-after-free addressed by v1 is independent of the two pre-existing issues raised by Sashiko.
For the skb2 leak in the same err: path, would you prefer that I fold
if (skb2) dev_kfree_skb_any(skb2);
into a v2 of this patch, or keep it as a separate follow-up? My plan for the independent ncm->netdev NULL-dereference race is to handle it separately.
Is the use-after-free fix otherwise acceptable, or are any other changes needed?
Thanks,
Boyan