Messages in this thread | ![/](/images/icornerl.gif) | | From | Bjørn Mork <> | Subject | Re: [PATCH] net: usbnet: Fix potential NULL pointer dereference | Date | Mon, 06 Nov 2023 11:55:49 +0100 |
| |
Oliver Neukum <oneukum@suse.com> writes:
> yes it looks like NCM does funky things, but what does that mean? > > ndp_to_end_store() > > /* flush pending data before changing flag */ > netif_tx_lock_bh(dev->net); > usbnet_start_xmit(NULL, dev->net); > spin_lock_bh(&ctx->mtx); > if (enable) > > expects some odd semantics from it. The proposed patch simply > increases the drop counter, which is by itself questionable, as > we drop nothing. > > But it definitely does no IO, so we flush nothing. > That is, we clearly have bug(s) but the patch only papers over > them. > And frankly, the basic question needs to be answered: > Are you allowed to call ndo_start_xmit() with a NULL skb? > > My understanding until now was that you must not.
Yuck. I see that I'm to blame for that code, so I've tried to figure out what the idea behind it could possibly have been.
I believe that code is based on the (safe?) assumption that the struct usbnet driver_info->tx_fixup points to cdc_ncm_tx_fixup(). And cdc_ncm_tx_fixup does lots of weird stuff, including special handling of NULL skb. It might return a valid skb for further processing by usbnet_start_xmit(). If it doesn't, then we jump straight to "not_drop", like we do when cdc_ncm_tx_fixup decides to eat the passed skb.
But "funky" is i precise description of all this... If someone feels like it, then all that open coded skb queing inside cdc_ncm should be completely rewritten.
Bjørn
| ![\](/images/icornerr.gif) |