netdev - Re: [PATCH] net: usbnet: Fix potential NULL pointer dereference

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <80af8b7a-c543-4386-bb0c-a356189581a0@suse.com>
Date: Mon, 6 Nov 2023 11:18:39 +0100
From: Oliver Neukum <oneukum@...e.com>
To: Ren Mingshuai <renmingshuai@...wei.com>, kuba@...nel.org,
 Bjørn Mork <bjorn@...k.no>
Cc: caowangbao@...wei.com, davem@...emloft.net, khlebnikov@...nvz.org,
 liaichun@...wei.com, linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
 oneukum@...e.com, yanan@...wei.com
Subject: Re: [PATCH] net: usbnet: Fix potential NULL pointer dereference



On 02.11.23 10:06, Ren Mingshuai wrote:
>>>> 23ba07991dad said SKB can be NULL without describing the triggering
>>>> scenario. Always Check it before dereference to void potential NULL
>>>> pointer dereference.
>>> I've tried to find out the scenarios where SKB is NULL, but failed.
>>> It seems impossible for SKB to be NULL. If SKB can be NULL, please
>>> tell me the reason and I'd be very grateful.
>>
>> What do you mean? Grepping the function name shows call sites with NULL getting passed as skb.
> 
> Yes And I just learned that during the cdc_ncm_driver.probe, it is possible to pass a NULL SKB to usbnet_start_xmit().

Hi,

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.

	Regards
		Oliver