Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <0ed0f964-f8c8-4776-a2ae-ca25071bd0cd@suse.com>
Date:   Mon, 6 Nov 2023 13:53:12 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] net: usbnet: Fix potential NULL pointer dereference
Content-Language: en-US
To:     =?UTF-8?Q?Bj=C3=B8rn_Mork?= <bjorn@mork.no>,
        Oliver Neukum <oneukum@suse.com>
Cc:     Ren Mingshuai <renmingshuai@huawei.com>, kuba@kernel.org,
        caowangbao@huawei.com, davem@davemloft.net, khlebnikov@openvz.org,
        liaichun@huawei.com, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, yanan@huawei.com
References: <20231101213832.77bd657b@kernel.org>
 <20231102090630.938759-1-renmingshuai@huawei.com>
 <80af8b7a-c543-4386-bb0c-a356189581a0@suse.com>
 <871qd3up56.fsf@miraculix.mork.no>
From:   Oliver Neukum <oneukum@suse.com>
In-Reply-To: <871qd3up56.fsf@miraculix.mork.no>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TmYrc1U0QVd1alIrWnBEc2E5YUg0ZVlLZ3JWd29yUFJQM3NNOEN5ODdDWmNX?=
 =?utf-8?B?VXg4b2QxQjI5MDUydWR3SjVNYjd0SFpOQ0tjK0RXYjdDaWd6WU4zRzVKcFlS?=
 =?utf-8?B?TlVUWjE1S3dXcEdST0hLdGNGcTdKd3h0aWVxMm1oNGdBVFFkbHFUSUMxWjl3?=
 =?utf-8?B?TzNKV2VJZlNrRC9WcnN4Z2kwSTNMNExPSzdROEZrSFl3d3RieisrM2tBWEtQ?=
 =?utf-8?B?N3ZDeW1pQjl3UU1sUE56ZWthQmJjZktUMkx5Q0l6djFiKzhVdDhpN0cwVjhj?=
 =?utf-8?B?TktldnRGN0wrODhaSzVwWEtKV3JnTW8vY3ZqdHI2N3RSRUdUeHRmMjh2dVVr?=
 =?utf-8?B?a0VlQzZ0d0VZSFFuZStWckdOZk1pTkk0T3h2Zkw3ZDM3eE5VZG55SFVqQUds?=
 =?utf-8?B?eTBKdzFyRGhaWUlJdGJTNGtSMjl4MjMyVjVFOUxCZC9HbnlWVjU5UytHNGh1?=
 =?utf-8?B?NGRoSU5HQW9MbUphZ25CZHFjUC8rNTlRZld2amxTS0VTYUdSTEhCU0Nja3JN?=
 =?utf-8?B?OW9sMjVBbk96bHc0TmVYZ1dxaGQyZVNvSVBORjArR08rRmRsaUoxQmF6eDVV?=
 =?utf-8?B?ZzB0aUJHekIwZnhjMW8xM0RJanUyWGdYNmRYNXNlNUdOdVBHR0JLRWxiYng5?=
 =?utf-8?B?Y3JTcm1WTXBTM3dDcDZWV2dHNWdZOWlXa093S2FSYjdwR2U1c3dTbkFXUUVE?=
 =?utf-8?B?NnlIT21QQ1ZDTGFQVm9uQzg0M3JjUHZwL0FGcEt0Q1RHamhiSXNENldzczBo?=
 =?utf-8?B?ZEJCN1FVV29yTlVybVBySzBWeWpzaDJ1SGtBeGVOdDlQSzVUa013SDA0OUFh?=
 =?utf-8?B?T1FRWVNiUFhHcy83Yjc1QmpMdXFXZ002N2FweFlkdlJTS2I4bXp3eUx1R1BN?=
 =?utf-8?B?VnVrVUtyYndyeU45akVaVVFVSHV5VHp5V2ZLMGVoYXdoMnZXR2VMb1FmRk5r?=
 =?utf-8?B?UzZOTEU5eWZES2FnZDd0UDBta1kvNU5PaEFUSitkZlRRbDZzdVhSQ2tDcTRD?=
 =?utf-8?B?MG02bmFWWHFGZUZpM1BINXoveTMrZTQxeUZUR3ZGUmcxMWsvVXBiZjROZkh0?=
 =?utf-8?B?U2ZBOHZtY3JEeXI1eFI0SVlHcmplaG0xM2FvajMzSzk5TTNLU0JJN1oyNXFs?=
 =?utf-8?B?N2lkMHhIQzVLN1l3Y2ZBUHRFbUZGaWpRZHRwQTN5a1p5YW5ZSFo2R2lFYmFQ?=
 =?utf-8?B?WXRDbFE1YVI1WmxHYzBVbEtqL2lUNEs3YW1CVUdNb1BDSFYwMlBpU1ZCdkMv?=
 =?utf-8?B?Ni9iVFZvNGxaTU1hUlNDaFNYTm1GRGQ4OUhEVHgrMWtjQmtKenNRSDVmVGcz?=
 =?utf-8?B?Unp4RW9RbFgwdGt3V1pYYytabzRFbTVWblJaOGdxbnNuS3E4UFJrT0hhd29H?=
 =?utf-8?B?VlBPRDdLbzBLUDVGdm9SR3l5RHJseFErTHFsRFlOVjFnM25iYzB1OUJ2cmNz?=
 =?utf-8?B?ckFJMjJVdEhzc2pFM1U3YlJaYlVIVkphY1Z1Z1RFYTJuRWVJbnV5ak5VUDQ3?=
 =?utf-8?B?WHF2SnRNNmhpSXZWRC9SWTZWZi9qWDk2TjZZUzRLbFNFNHQ5VWdnVXRadmtX?=
 =?utf-8?B?cElPTFRlSmswb05pWFZ2RWhaMEtEOWRXUzJNWjdaNmtnMmI4bWFES295YzJi?=
 =?utf-8?B?cGNOL1VDMDFWcUxyb0FEMTJiTGNodmFiSEF4L2VuTVNnb3RJYzdEQUhycm5n?=
 =?utf-8?B?bUtOUVFCb1d0ZTB4WW0vNFdQbkY3Q3JGRnNLdHFrV1RtMWg0U21UZVlSOCs1?=
 =?utf-8?B?MkZHMkxxOTBONHJzYXdyOFJJRTd0d1NWalJ6cXQxUElPYVBOd0N1eE1LbG9h?=
 =?utf-8?B?UGJUS1NGb2ppYStUOTl0b3Z5WHZiQ1o5cXJVbk9vN2p5UlQvQ2J4NFppWVgx?=
 =?utf-8?B?c0RkajROU3drTjM2SFNEczJCeEUzT3pEaHJuYlZ6UER6VnBYeEZHOGF6NXJo?=
 =?utf-8?B?VmhYZ1FIUXF5bjdoNjZtOHdXV2dmaS91eW05c0J3SU1RckQyemJFbkl3Ynlj?=
 =?utf-8?B?WmhQOGxhWS9pOEpqQ0hqcHdlV3VjSDFJQlNzOFBUbW1yemNXZmJZeWJGMWFY?=
 =?utf-8?B?aTRQNXJJYUl0MHVncW1XRm51VXJIeWFQWC92NTM4QUpCY0ZTSEFNUUNsUll2?=
 =?utf-8?B?YlZzclNzcS9VZEhQSkdhaXJSL2FpZDJ5QWZNaTdBQVZuL0ROVS9UU2U3Q2c5?=
 =?utf-8?Q?XyQxDNuP7FscdXDrBBo+9Q1nv1nyAj9En3Pzm5yTyqqg?=
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e02559ed-b4d4-4f8c-51ff-08dbdec7574a
X-MS-Exchange-CrossTenant-AuthSource: AM0PR04MB6467.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Nov 2023 12:53:19.5789
 (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: yCnCYdVsW53Us9LvFyIkaYgw5T182uNso7p4BpWVOjopIMpk41HahGhJ6rYG2hY34O7tdk3Xq9SjDxtmG2nmAA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DU2PR04MB8823
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 lipwig.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 (lipwig.vger.email [0.0.0.0]); Mon, 06 Nov 2023 04:53:39 -0800 (PST)

On 06.11.23 11:55, Bjørn Mork wrote:

> 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

That seems to be a correct assumption, but one that is far from obvious.
Could you add a big, fat comment?

> 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.

I understand what you mean, but I need a generic answer. Can you call
ndo_start_xmit() with skb == NULL?

	Regards
		Oliver