Re: [PATCH wireless v2 1/3] wifi: iwlwifi: pcie: don't infer CSME presence from a failed read

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing cancellation of `me_recheck_wk` delayed work in the probe error path leads to a use-after-free when the timer fires.
--

commit ff64adc37d4f911609320ae5c3255e7d89218807
Author: Navon John Lukose <navonjohnlukose@gmail.com>

wifi: iwlwifi: pcie: don't infer CSME presence from a failed read

This commit modifies iwl_pcie_check_me_status() to avoid inferring CSME
presence from hardware error values on failed reads. It skips evaluating
these bad values so the state can be preserved and rechecked later.

Fixes: 41fff83fe6cd ("wifi: iwlwifi: pcie: check for WiAMT/CSME presence")

> diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c
> index 28b276c659526..c6a771e899041 100644
> --- a/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c
> +++ b/drivers/net/wireless/intel/iwlwifi/pcie/gen1_2/trans.c

[ ... ]

> @@ -4212,15 +4213,19 @@ static void iwl_pcie_check_me_status(struct iwl_trans *trans)
>  		return;
>  
>  	val = iwl_read_prph(trans, CNVI_SCU_REG_FOR_ECO_1);
> -	if (val & CNVI_SCU_REG_FOR_ECO_1_WIAMT_KNOWN) {
> +	/* iwl_read_prph() returns 0x5a5a5a5a if it never reached the NIC, and
> +	 * that value has WIAMT_KNOWN set and WIAMT_PRESENT clear
> +	 */
> +	if (val != ~0U && !iwl_trans_is_hw_error_value(val) &&
> +	    (val & CNVI_SCU_REG_FOR_ECO_1_WIAMT_KNOWN)) {
>  		trans_pcie->me_present =
>  			!!(val & CNVI_SCU_REG_FOR_ECO_1_WIAMT_PRESENT);
>  		return;
>  	}
>  
>  	val = iwl_read32(trans, CSR_HW_IF_CONFIG_REG);
> -	if (val & (CSR_HW_IF_CONFIG_REG_ME_OWN |
> -		   CSR_HW_IF_CONFIG_REG_IAMT_UP)) {
> +	if (val != ~0U && (val & (CSR_HW_IF_CONFIG_REG_ME_OWN |
> +				  CSR_HW_IF_CONFIG_REG_IAMT_UP))) {
>  		trans_pcie->me_present = 1;
>  		return;
>  	}

[Severity: High]
This is a pre-existing issue, but does this change make a use-after-free
consistently triggerable on a dead NIC?

Since this patch bypasses the early returns for hardware error values,
iwl_pcie_check_me_status() will now fall through and unconditionally schedule
the delayed work:

    schedule_delayed_work(&trans_pcie->me_recheck_wk, HZ);

If the probe subsequently fails (for instance, if iwl_drv_start() fails
against the unresponsive hardware), the probe function goes to the
out_free_trans error path:

iwl_pci_gen1_2_probe() {
    ...
out_free_trans:
    iwl_trans_pcie_free(iwl_trans);
    return ret;
}

But iwl_trans_pcie_free() doesn't appear to call cancel_delayed_work_sync()
for me_recheck_wk before freeing the trans structure.

If the delayed work timer fires a second later, won't it dereference the
freed trans_pcie pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831130332.323549-1-navonjohnlukose@gmail.com?part=1