Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Message-ID: <80af8b7a-c543-4386-bb0c-a356189581a0@suse.com>
Date:   Mon, 6 Nov 2023 11:18:39 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] net: usbnet: Fix potential NULL pointer dereference
To:     Ren Mingshuai <renmingshuai@huawei.com>, kuba@kernel.org,
        =?UTF-8?Q?Bj=C3=B8rn_Mork?= <bjorn@mork.no>
Cc:     caowangbao@huawei.com, davem@davemloft.net, khlebnikov@openvz.org,
        liaichun@huawei.com, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, oneukum@suse.com, yanan@huawei.com
References: <20231101213832.77bd657b@kernel.org>
 <20231102090630.938759-1-renmingshuai@huawei.com>
Content-Language: en-US
From:   Oliver Neukum <oneukum@suse.com>
In-Reply-To: <20231102090630.938759-1-renmingshuai@huawei.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?a1h0ampCNTFoQ3hnbENnbkZVS3BkZ2VZRmRhUUhVUWIyWGd3YjdUNE4yWjFI?=
 =?utf-8?B?azNaV2JzSDFJbzRRVSsvQklGS3hBVjdhekhXQ203U3FRRVp0RFJ0ZkhRQTBO?=
 =?utf-8?B?MEV3d0dxZ2thZlVMcVNCTm52SnhlU2hHUXc0cmdVbGQ3S1p3S1ArYkYxeHNR?=
 =?utf-8?B?am9pcDg1UmFrRjVvbGduZWRUWnYvYnFlVGJZd1NYbnpoNDZDZXJPSmRDK0lq?=
 =?utf-8?B?SENPdDZHR3JuR1RHTVlEYnVsYU1YV0E5SmdJQ0Q2NkgrVUdpMEhWQ0ZrWkRj?=
 =?utf-8?B?Y3hESVphbmxUTUFtdDZ6aXpDOGZDQ2J0KzAwTGZleFVqL1UyRWxTNmFzMTJC?=
 =?utf-8?B?ZkR0SjdiclJuTWJsdGFPSkpzbU5ldjR3ZlBhRmRIendoZSsrWm1lTDduU2ox?=
 =?utf-8?B?Yk9UUEZ3ZVJZWFJrM0lYcWNRWGNMWXhkdUhoT0Jlc2FWVEF2VlNpVmIxbGRI?=
 =?utf-8?B?Z0FwQkdHVWJyK0dNdElpem9zNUVwclZPdWFrS1pLbkZLQ3pxemtDVlZ0WUcx?=
 =?utf-8?B?WWJPaXZ3WWdnMzZiMVQ3V3VEekZHMzBza01JallrT2Z0YlJjSE8vTWlvS21y?=
 =?utf-8?B?czdwb3UydEdkOEw3RWpsUHE3TjhNbEY2dll2N2RxOTgvUTVKYVE5dTFjVUF6?=
 =?utf-8?B?ZStzQ0J3SSt6SzllSFo5d3kwK3dTMHNpOXNQaHI2ZEFhK0ZhS3piaDRRQmRX?=
 =?utf-8?B?Mk1CZ3dCUjNGVFFXc2ZqaTlYdHc2WTFBazd0MUtLQVhxejJTZzMyc3JvL2VT?=
 =?utf-8?B?STRTMkpXaFdVYzZGLzJ6QXowWHZZaC9WWnNqVkdETnl1THY0SDJjNmdTY0Fo?=
 =?utf-8?B?OFdGYVdTVXlRbk00VFV0eXU4a2tkeVpURU0xWVByeVQvT09hc09iU29mV1dJ?=
 =?utf-8?B?alYwZEVFS3dDN2FtcG1BL05YckdJNHFveE5BNXB2THhJUStBNUV4MWR3dXJu?=
 =?utf-8?B?K0x4VFJjLzZZNm1JajhnT0JjV1BsNjZoYURVckllOElnVGtham5ESXd5OU1t?=
 =?utf-8?B?V3lwYWkyWnZ1VmpVQnFGei9rOGFWUVY2Z1IzWFdxbEc3YmVYVlUvWHFqZ2Jm?=
 =?utf-8?B?ZmxUSVFlSUlRRzVWYVJoSTVKZDVMSXJJZ2pVdW5RbFllOGNJblBPS3h5TDRH?=
 =?utf-8?B?SHNxQlAxTjRLbUcwR05RZW5wM0pvV0xqWEdZZTU4MXNZSUZJcjhPTFpHcy9K?=
 =?utf-8?B?V2tGcjRZWGhxeXc0cEM3RlNrS2tRWVdhS0pqNjZMb1NwSHFhQWRDdytRa2tM?=
 =?utf-8?B?OEFHYU5Gc3kxc2JheVJ3dGQ3YmdBSjVqbmUvTEltbWhsc21FZW5TRzhKQkRT?=
 =?utf-8?B?UEF5ZDd3VDZhd3RXZTBLQnBOM1U5YmM1RXpNZVJ3TDUvcUk4STFRODVWVi9s?=
 =?utf-8?B?aWdsSkQ5TmJWbjBpV251UklGV2paSy9vRzBaYlhoU3lQREw1bFM2eHFlUGNh?=
 =?utf-8?B?VldKR0lwWlRLRDhFUFJpRGxkS2FyWGNMV0xWNHBDa05xKzBOcU1MeUNQRC9H?=
 =?utf-8?B?eHAzaUUxZ3krd0xBWjhrY2QrcFVCZXQ5a2FoQnQ3QkUvT2JSRThZN3IxWmxy?=
 =?utf-8?B?Rmpjd2dwRTgyQW90cCs1OUV1OHl0VW03QnlVWVJsbEMvcHdpYnh6a3ZHZXdx?=
 =?utf-8?B?eWlxNFdaUGxHUm56di9ScDBQRWVJOWlKV1JIUmxBbHRNTW1WT1FwK0FzcDMx?=
 =?utf-8?B?OXlBbm5ZZXNRdlBHb0xFcHFYL1ZZQlFVNk5ZSXo4eUdxRHk2ZTNRbDNnMHQy?=
 =?utf-8?B?aDRka2JvTVh5U1g5cytkZlJWa1FJaFNwSDRNbElwcFg4Ni93ejNFUzMwSTJ5?=
 =?utf-8?B?WEpYK1YwK25RbzA0UHQ1OFNIQ2Qyazk2ZHNhWXRVNEMwaE5CUktHWkZuNEpa?=
 =?utf-8?B?TG5wK3pNVEFFNmhEbmI3TGVwaGNPdGs3bitIQnVsRHZNSEV0eGpYblY0S2lV?=
 =?utf-8?B?MnFkQk9CcGNSNzBkVjA4QUxoTjlwSWFkN1IwaElPM0tPU0s2Y0JDUS8rbGRJ?=
 =?utf-8?B?aGJQRjJPTG5CWGJvMG5qVkRJU1JMZlNkK0RhNWR0Vnd4NlNwZzN6NGtVRmNz?=
 =?utf-8?B?b2hJV3FNaWVoZW9VYzBiVzhWWURXRlFQSE03ZkgraU5zWGZuZDZzUUNWN3d1?=
 =?utf-8?B?YlZ0SVJiRG5hZTlRbDJUNmZlaTNjN05oUGlIMC81Z1BRU3JZc3ZoaDA1aW1G?=
 =?utf-8?Q?BaLGhyj6JS30/FFBDVLVhZKqV9tLj4nH1i2T+k87iyav?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9ba64dc4-08ea-4a9b-a684-08dbdeb1c17f
X-MS-Exchange-CrossTenant-AuthSource: AM0PR04MB6467.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Nov 2023 10:18:45.9976
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: V+JOEIZtavlPiUsYMqOEf75PtuY+78O5sDCAtxyQhmvpXErBAuNntNF5uR1DXl+wP4yvMBjs+W948m7r4jWVbg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PA4PR04MB9343
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Mon, 06 Nov 2023 02:19:03 -0800 (PST)


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