netdev - RE: [PATCH iwl-net 1/2] ice: fix double-call to ice_deinit

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] [day] [month] [year] [list]

Message-ID: <IA3PR11MB8986384371A15DDD2A279F86E55DA@IA3PR11MB8986.namprd11.prod.outlook.com>
Date: Mon, 21 Jul 2025 15:00:23 +0000
From: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
To: "Keller, Jacob E" <jacob.e.keller@...el.com>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, Intel Wired LAN
	<intel-wired-lan@...ts.osuosl.org>
CC: "Grinberg, Vitaly" <vgrinber@...hat.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
Subject: RE: [PATCH iwl-net 1/2] ice: fix double-call to ice_deinit_hw()
 during probe failure



> -----Original Message-----
> From: Keller, Jacob E <jacob.e.keller@...el.com>
> Sent: Thursday, July 17, 2025 6:57 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@...el.com>; Intel Wired LAN
> <intel-wired-lan@...ts.osuosl.org>; Loktionov, Aleksandr
> <aleksandr.loktionov@...el.com>
> Cc: Keller, Jacob E <jacob.e.keller@...el.com>; Grinberg, Vitaly
> <vgrinber@...hat.com>; netdev@...r.kernel.org
> Subject: [PATCH iwl-net 1/2] ice: fix double-call to ice_deinit_hw()
> during probe failure
> 
> The following (and similar) KFENCE bugs have recently been found
> occurring during certain error flows of the ice_probe() function:
> 
> kernel:
> ==================================================================
> kernel: BUG: KFENCE: use-after-free read in
> ice_cleanup_fltr_mgmt_struct+0x1d
> kernel: Use-after-free read at 0x00000000e72fe5ed (in kfence-#223):
> kernel:  ice_cleanup_fltr_mgmt_struct+0x1d/0x200 [ice]
> kernel:  ice_deinit_hw+0x1e/0x60 [ice]
> kernel:  ice_probe+0x245/0x2e0 [ice]
> kernel:
> kernel: kfence-#223: <..snip..>
> kernel: allocated by task 7553 on cpu 0 at 2243.527621s (198.108303s
> ago):
> kernel:  devm_kmalloc+0x57/0x120
> kernel:  ice_init_hw+0x491/0x8e0 [ice]
> kernel:  ice_probe+0x203/0x2e0 [ice]
> kernel:
> kernel: freed by task 7553 on cpu 0 at 2441.509158s (0.175707s ago):
> kernel:  ice_deinit_hw+0x1e/0x60 [ice]
> kernel:  ice_init+0x1ad/0x570 [ice]
> kernel:  ice_probe+0x22b/0x2e0 [ice]
> kernel:
> kernel:
> ==================================================================
> 
> These occur as the result of a double-call to ice_deinit_hw(). This
> double call happens if ice_init() fails at any point after calling
> ice_init_dev().
> 
> Upon errors, ice_init() calls ice_deinit_dev(), which is supposed to
> be the inverse of ice_init_dev(). However, currently ice_init_dev()
> does not call ice_init_hw(). Instead, ice_init_hw() is called by
> ice_probe(). Thus,
> ice_probe() itself calls ice_deinit_hw() as part of its error cleanup
> logic.
> 
> This results in two calls to ice_deinit_hw() which results in straight
> forward use-after-free violations due to double calling kfree and
> other cleanup functions.
> 
> To avoid this double call, move the call to ice_init_hw() into
> ice_init_dev(), and remove the now logically unnecessary cleanup from
> ice_probe(). This is simpler than the alternative of moving
> ice_deinit_hw()
> *out* of ice_deinit_dev().
> 
> Moving the calls to ice_deinit_hw() requires validating all cleanup
> paths, and changing significantly more code. Moving the calls of
> ice_init_hw() requires only validating that the new placement is still
> prior to all HW structure accesses.
> 
> For ice_probe(), this now delays ice_init_hw() from before
> ice_adapter_get() to just after it. This is safe, as ice_adapter_get()
> does not rely on the HW structure.
> 
> For ice_devlink_reinit_up(), the ice_init_hw() is now called after
> ice_set_min_max_msix(). This is also safe as that function does not
> access the HW structure either.
> 
> This flow makes more logical sense, as ice_init_dev() is mirrored by
> ice_deinit_dev(), so it reasonably should be the caller of
> ice_init_hw().
> It also reduces one extra call to ice_init_hw() since both ice_probe()
> and
> ice_devlink_reinit_up() call ice_init_dev().
> 
> This resolves the double-free and avoids memory corruption and other
> invalid memory accesses in the event of a failed probe.
> 
> Fixes: 5b246e533d01 ("ice: split probe into smaller functions")
> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>