Re: Use-after-free in iwl-mld after hardware fails to restart.

[Ben Greear <greearb@candelatech.com>]

On 3/4/26 11:26, Ben Greear wrote:
> On 3/3/26 06:27, Ben Greear wrote:
>> On 3/2/26 23:19, Korenblit, Miriam Rachel wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Ben Greear <greearb@candelatech.com>
>>>> Sent: Monday, March 2, 2026 9:00 PM
>>>> To: linux-wireless <linux-wireless@vger.kernel.org>; Korenblit, Miriam Rachel
>>>> <miriam.rachel.korenblit@intel.com>
>>>> Subject: Use-after-free in iwl-mld after hardware fails to restart.
>>>>
>>>> Hello,
>>>>
>>>> Here's another failure case from my torture test system.
>>>>
>>>> This is from a patched 6.18.14+ kernel, but from a look at upstream Linux
>>>> kernel, the problems are there as well.
>>>>
>>>> I believe the problem is something like this:
>>>>
>>>> firmware crashes several times and cannot recover.  Maybe
>>>> Intel folks can tell why it is crashing?  I'd love to know if that
>>>> is something that can be fixed in the driver.
>>>>
>>>>   From mac80211/util.c:
>>>>
>>>> int ieee80211_reconfig(struct ieee80211_local *local)
>>>> is called, and gets to the failure case in this code (I see that second WARN in the
>>>> crash logs)
>>>> .....
>>>>     /*
>>>>      * Upon resume hardware can sometimes be goofy due to
>>>>      * various platform / driver / bus issues, so restarting
>>>>      * the device may at times not work immediately. Propagate
>>>>      * the error.
>>>>      */
>>>>     res = drv_start(local);
>>>>     if (res) {
>>>>         if (suspended)
>>>>             WARN(1, "Hardware became unavailable upon resume.
>>>> This could be a software issue prior to suspend or a hardware issue.\n");
>>>>         else
>>>>             WARN(1, "Hardware became unavailable during
>>>> restart.\n");
>>>>         ieee80211_wake_queues_by_reason(hw,
>>>> IEEE80211_MAX_QUEUE_MAP,
>>>>
>>>>     IEEE80211_QUEUE_STOP_REASON_SUSPEND,
>>>>                         false);
>>>>         ieee80211_handle_reconfig_failure(local);
>>>>         return res;
>>>>
>>>>
>>>> This method has comments about cleaning things up, but I don't see where it
>>>> actually
>>>> cleans up the driver.  And it sets SDATA_IN_DRIVER to false, so a lot of the calls
>>>> in driver-ops.h that would otherwise tell the driver to clean up skip calls to
>>>> the driver.
>>>>
>>>> static void ieee80211_handle_reconfig_failure(struct ieee80211_local *local)
>>>> {
>>>>     struct ieee80211_sub_if_data *sdata;
>>>>     struct ieee80211_chanctx *ctx;
>>>>
>>>>     lockdep_assert_wiphy(local->hw.wiphy);
>>>>
>>>>     /*
>>>>      * We get here if during resume the device can't be restarted properly.
>>>>      * We might also get here if this happens during HW reset, which is a
>>>>      * slightly different situation and we need to drop all connections in
>>>>      * the latter case.
>>>>      *
>>>>      * Ask cfg80211 to turn off all interfaces, this will result in more
>>>>      * warnings but at least we'll then get into a clean stopped state.
>>>>      */
>>>>
>>>>     local->resuming = false;
>>>>     local->suspended = false;
>>>>     local->in_reconfig = false;
>>>>     local->reconfig_failure = true;
>>>>
>>>>     ieee80211_flush_completed_scan(local, true);
>>>>
>>>>     /* scheduled scan clearly can't be running any more, but tell
>>>>      * cfg80211 and clear local state
>>>>      */
>>>>     ieee80211_sched_scan_end(local);
>>>>
>>>>     list_for_each_entry(sdata, &local->interfaces, list)
>>>>         sdata->flags &= ~IEEE80211_SDATA_IN_DRIVER;
>>>>
>>>>     /* Mark channel contexts as not being in the driver any more to avoid
>>>>      * removing them from the driver during the shutdown process...
>>>>      */
>>>>     list_for_each_entry(ctx, &local->chanctx_list, list)
>>>>         ctx->driver_present = false;
>>>> }
>>>>
>>>>
>>>> So, how is the driver supposed to be cleaned up in this scenario?
>>> Driver was cleaned up in drv_start. See iwl_mld_restart_cleanup.
>>> Could you please share the iwlmld.ko and iwlwifi.ko files?

Hello Miriam,

Should we call 'iwl_mld_restart_cleanup()' in the failure path before the goto error?

I do not see the log "mld-mac80211-start, already in-hw-restart, stopping fw cleanup"
in my dump, so it seems it did not actually do cleanup in this particular crash.

	if (mld->fw_status.in_hw_restart) {
		IWL_ERR(mld, "mld-mac80211-start, already in-hw-restart, stopping fw cleanup\n");
		iwl_mld_stop_fw(mld);
		iwl_mld_restart_cleanup(mld);
	}

	if (!in_d3 || ret) {
		ret = iwl_mld_start_fw(mld);
		if (ret) {
			IWL_ERR(mld, "Failed start-fw. ret=%d\n", ret);
			goto error;
		}
	}

...

And note this in the error path in same method:

error:
	/* If we failed to restart the hw, there is nothing useful
	 * we can do but indicate we are no longer in restart.
	 */
	mld->fw_status.in_hw_restart = false;

Maybe that keeps that cleanup code above from hitting in double failure
states?

[snipped rest of the logs to keep the email from being huge]

Thanks,
Ben