netdev - Re: [Intel-wired-lan] [PATCH iwl-net] idpf: fix adapter NULL pointer dereference on reboot

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: <9d84cc79-2990-49dd-a528-3d6d182ea2e9@intel.com>
Date: Mon, 10 Mar 2025 21:42:28 -0700
From: "Tantilov, Emil S" <emil.s.tantilov@...el.com>
To: Simon Horman <horms@...nel.org>
CC: <willemb@...gle.com>, <pabeni@...hat.com>, <netdev@...r.kernel.org>,
	<yuma@...hat.com>, <Aleksandr.Loktionov@...el.com>, <edumazet@...gle.com>,
	<madhu.chittim@...el.com>, <anthony.l.nguyen@...el.com>, <kuba@...nel.org>,
	<intel-wired-lan@...ts.osuosl.org>, <decot@...gle.com>,
	<davem@...emloft.net>, Michal Swiatkowski
	<michal.swiatkowski@...ux.intel.com>
Subject: Re: [Intel-wired-lan] [PATCH iwl-net] idpf: fix adapter NULL pointer
 dereference on reboot

On 3/9/2025 11:22 PM, Simon Horman wrote:
> On Thu, Mar 06, 2025 at 04:39:56PM -0800, Emil Tantilov wrote:
>> Driver calls idpf_remove() from idpf_shutdown(), which can end up
>> calling idpf_remove() again when disabling SRIOV.
>>
>> echo 1 > /sys/class/net/<netif>/device/sriov_numvfs
>> reboot
>>
>> BUG: kernel NULL pointer dereference, address: 0000000000000020
>> ...
>> RIP: 0010:idpf_remove+0x22/0x1f0 [idpf]
>> ...
>> ? idpf_remove+0x22/0x1f0 [idpf]
>> ? idpf_remove+0x1e4/0x1f0 [idpf]
>> pci_device_remove+0x3f/0xb0
>> device_release_driver_internal+0x19f/0x200
>> pci_stop_bus_device+0x6d/0x90
>> pci_stop_and_remove_bus_device+0x12/0x20
>> pci_iov_remove_virtfn+0xbe/0x120
>> sriov_disable+0x34/0xe0
>> idpf_sriov_configure+0x58/0x140 [idpf]
>> idpf_remove+0x1b9/0x1f0 [idpf]
>> idpf_shutdown+0x12/0x30 [idpf]
>> pci_device_shutdown+0x35/0x60
>> device_shutdown+0x156/0x200
>> ...
>>
>> Replace the direct idpf_remove() call in idpf_shutdown() with
>> idpf_vc_core_deinit() and idpf_deinit_dflt_mbx(), which perform
>> the bulk of the cleanup, such as stopping the init task, freeing IRQs,
>> destroying the vports and freeing the mailbox.
> 
> Hi Emil,
> 
> I think it would be worth adding some commentary on the rest of
> the clean-up performed by idpf_remove() and why it is correct
The main reason behind the change is to avoid calling sriov_disable(), 
which ends up calling idpf_remove() again via pci_device_remove(). The 
idpf_remove() will crash in that situation as it attempts to access 
adapter pointer, which was already freed.

> to no longer do so directly from a call to idpf_remove() from
> idpf_shutdown() (IOW, it isn't clear to me :).
I assume you are asking what portion of the idpf_remove() will not be 
present in idpf_shutdown() as result? Aside from not calling 
sriov_disable(), there is a small cleanup of stale netdevs and the 
destruction of WQs, which did not seem like would be needed on shutdown. 
Then again, I was not able to find documentation on what steps are 
required for shutdown and mostly checked on how other drivers handle it 
(where there is no 1:1 overlap between shutdown and remove), and applied 
similar steps to idpf. Ideally I do not wish to do more than is needed 
for that flow.

> 
> ...