Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <aef0c568-e088-b897-f8ec-f22cfef124f6@suse.com>
Date:   Wed, 20 Apr 2022 11:45:49 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH] driver: usb: nullify dangling pointer in cdc_ncm_free
Content-Language: en-US
To:     Johan Hovold <johan@kernel.org>, Oliver Neukum <oneukum@suse.com>
CC:     Andy Shevchenko <andy.shevchenko@gmail.com>,
        Oleksij Rempel <linux@rempel-privat.de>,
        Dongliang Mu <dzm91@hust.edu.cn>,
        Oliver Neukum <oliver@neukum.org>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Dongliang Mu <mudongliangabcd@gmail.com>,
        syzbot+eabbf2aaa999cc507108@syzkaller.appspotmail.com,
        USB <linux-usb@vger.kernel.org>, netdev <netdev@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <20220409120901.267526-1-dzm91@hust.edu.cn>
 <YlQbqnYP/jcYinvz@hovoldconsulting.com>
 <CAHp75VeTqmdLhavZ+VbBYSFMDHr0FG4iKFGdbzE-wo5MCNikAA@mail.gmail.com>
 <d851497f-7960-b606-2f87-eb9bff89c8ac@suse.com>
 <Yl+utFmKEgILDFr5@hovoldconsulting.com>
From:   Oliver Neukum <oneukum@suse.com>
In-Reply-To: <Yl+utFmKEgILDFr5@hovoldconsulting.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 2
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?kw/jG1rHfuOBxKvmV4yi3V5Fiyvn9gOv1lIcYLDJ5tN0GvwG292PteAfxSh3?=
 =?us-ascii?Q?uLi0cYn/kPZcmd9AMVHTPKCp/TRMIbckBv/rqzNhss5FyrZXHPLsLj2ur9EN?=
 =?us-ascii?Q?MLbtMzoDKS3lqw7fpidrgynguVcHD6K7g03NbEGc3wssZpmrzziZ8stXuoIe?=
 =?us-ascii?Q?PCZSuzIH4JGDFpLlOBpo0uoNaTQ1HN+GLvIDNq7SZ/KpKXYLCIBDl6wVUbts?=
 =?us-ascii?Q?QDWEVZeOPEXJgxg3VcwDxECmdvnEGcmvOogKeJvy3mFrmiqivsbZMhTwvaZz?=
 =?us-ascii?Q?Ke44rxb7WQn/iVqTmwloRnvYZZKXQ5DvPF/CBY28Ptp8DCnw2MSEHPDw6pN5?=
 =?us-ascii?Q?795kVBatf35SFEXzCXBoAdhRA3fatFmZ8Cn5W4Eqy7OdM//0d90EP2fkdo73?=
 =?us-ascii?Q?nhzSGm5H22SkghCFL0vLkLjl+FRp9q3Rq3OlBNjYL2xg3NtVrbyzBbrcWJ9f?=
 =?us-ascii?Q?ZH5WoAN3mDF8McboH4yQMtw5ZKnCauECE3IF50tSPHwRro9LVPW4r8+TMJAH?=
 =?us-ascii?Q?/GwsUvV6e23I/Bwip/tfwDocLnmIMY2hvRzgvuVC6nXdfgqok1Exy+HxBIPf?=
 =?us-ascii?Q?NoXVqDIDUVC94TARHZ36S9EGZeFX3xR+rshNd95e1K+mStLPyGBwBtyc51Yj?=
 =?us-ascii?Q?+G3+REVPb1JUr5VTXtG4/wyr8I0oFza5mEeKd2hBEJ+gzh7kxK0ZS6dAhg2R?=
 =?us-ascii?Q?1QT5dFT2QfqSbgD1VBm1ztAp3W13Dl5j99117gjLKbiHIF+1hTpEB5nZEYO4?=
 =?us-ascii?Q?FvK4LH5EmGCH74GzMksc32a9i6Uy9CA9hS16syQcKgTlPD2ioqF5hAxZ4fyF?=
 =?us-ascii?Q?RCsxWmjAQx0X8ZlV6EQnxwh8lMeP4CRsv2mKFNOyGJyTnMONK7KEv3+Gsi1G?=
 =?us-ascii?Q?UxtVDMpNy2oQIYdNP30YMBAjCRluLQdeLrqWJVlGYZsbDm2wsMFKz7iJ68ge?=
 =?us-ascii?Q?PWjavC/Mp5QmxD/TtpxAi8vQqa8ZFJev13bsmsgkdTCY6mLJ89YYiJ395DZu?=
 =?us-ascii?Q?G1b9DQbKSCKjpENGVB70wiLowu82feeYWZpjNUQ37oXgTgOB/8J9h48lLo03?=
 =?us-ascii?Q?Ztj3Puq8afCEWRwjDKuLT5dy30MMoBgHrX6FUQzSwrxT8jEmTEzO2UrolFkH?=
 =?us-ascii?Q?Sry82v1DazLrywKPabEJC7ly4D01n0Y4hFSiHt/9KhWaqSY2qbePpIPxUffg?=
 =?us-ascii?Q?O+YqFCyBBLS8AW4pOWzoGM98ER7UnpuNCTYWZ+fsDmK8BTSH1VPnNzA899g2?=
 =?us-ascii?Q?esTATo+pJr430xJNSbI0GpfxLwgvoWuuG5ROsipcKnU1veMoc7OUYVDq0Z9P?=
 =?us-ascii?Q?hn+ZgHhbWS4xc7heyeIUPbnGiEDABxnEX1g7J0jo7DySMcKRncY1eQwLWneg?=
 =?us-ascii?Q?ybE1dMxKTcrDLPR6h7zQGjQ1fr5X/0+bCpih2PNkQpPmqmojaS14NjUaXNuJ?=
 =?us-ascii?Q?fc0VPb+aoCDSUb2IMP0WcKwXgYoGWysJsWB2EOYLxNBirx2fU7qJPbUUP7mZ?=
 =?us-ascii?Q?WzE4IvUM2g8Oj5brBFPYd2+ruU2s3fIuib2gy1qjJdHOXMSZxMuChhWSnQYE?=
 =?us-ascii?Q?+bWhNrLCxiej5dk40bWGIIXDXDoDLy0lo5brLymflJX3Do349yU2pjdJLv0X?=
 =?us-ascii?Q?OD+6lxdH1zaHhO4LR/nkbq33qO5lRajmj19kiFLvQtmZT4RWhNVVWJ8SSoK0?=
 =?us-ascii?Q?x+tw9FTvgKjwvCl/IMBaboBgyl9ZEiKcCxBYEcDLGaKmdoNlxMOl3x/INS0l?=
 =?us-ascii?Q?LSx5ZVAi+UiE+uV0Du2oEkPQiPOJxaJTIeUNvxiCyImicfqLDm8q2HRBXxht?=
X-MS-Exchange-AntiSpam-MessageData-1: snNBOyMlvP80hg==
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 98c941c1-7a3b-45a9-c2f9-08da22b28f17
X-MS-Exchange-CrossTenant-AuthSource: VI1PR0401MB2526.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Apr 2022 09:45:52.9381
 (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: 1AuBnmhtnMkMmWpGNXobFonnF6Fnq+LZUJQOB/RhsbhvvR/SCI2xdns5OJBsl1dLpTNDi90+g8IgV7VNnW2/og==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB4335
X-Spam-Status: No, score=-3.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 20.04.22 08:56, Johan Hovold wrote:
>
>> =20
>> @@ -1214,7 +1214,7 @@ static const struct driver_info hawking_uf200_info=
 =3D {
>>  static const struct driver_info ax88772_info =3D {
>>  	.description =3D "ASIX AX88772 USB 2.0 Ethernet",
>>  	.bind =3D ax88772_bind,
>> -	.unbind =3D ax88772_unbind,
>> +	.unbind =3D ax88772_disable,
> These should all be
>
> 	.disable =3D ...
Thanks, noted.
>
> but you probably need to split the callback and keep unbind as well for
> the actual clean up (freeing resources etc).
That is the driver the problematic commit was requested for.
I am looking into it.
>
>> -	if (dev->driver_info->unbind)
>> -		dev->driver_info->unbind(dev, intf);
>> +	if (dev->driver_info->disable)
>> +		dev->driver_info->disable(dev, intf);
>> =20
>>  	net =3D dev->net;
>>  	unregister_netdev (net);
>> @@ -1651,6 +1651,9 @@ void usbnet_disconnect (struct usb_interface *intf=
)
>> =20
>>  	usb_scuttle_anchored_urbs(&dev->deferred);
>> =20
>> +	if (dev->driver_info->unbind)
>> +		dev->driver_info->unbind (dev, intf);
>> +
>>  	usb_kill_urb(dev->interrupt);
> Don't you need to quiesce all I/O, including stopping the interrupt URB,
> before unbind?
If I do that, how do I prevent people from relaunching the URB between
kill and unbind? Do I need to poison it?

=C2=A0=C2=A0=C2=A0 Regards
=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 Oliver