Re: [PATCH ath-next 04/16] wifi: ath11k: Add support for loading m3 mbn firmware

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing IS_ERR check on devm_memremap() leads to kernel panic
- [Medium] Permanent memory mapping of firmware region risks secure memory violation
--

commit 3ba907fe6d594d6c69e92925ac78d06c3ce54aaf
Author: George Moussalem <george.moussalem@outlook.com>

wifi: ath11k: Add support for loading m3 mbn firmware

This commit adds support for loading the m3 firmware in MBN (MDT) format
for ath11k WiFi chips. It introduces a function to allocate a memory region,
request the firmware, and load MDT segments.

> diff --git a/drivers/net/wireless/ath/ath11k/ahb.c b/drivers/net/wireless/ath/ath11k/ahb.c
> index 86723a2eb3d26..9fe2e112dfb81 100644
> --- a/drivers/net/wireless/ath/ath11k/ahb.c
> +++ b/drivers/net/wireless/ath/ath11k/ahb.c
[ ... ]
> @@ -962,6 +964,58 @@ static int ath11k_ahb_boot_root_pd(struct ath11k_base *ab)
>  	return 0;
>  }
>  
> +static int ath11k_ahb_load_m3_firmware(struct ath11k_base *ab)
> +{
[ ... ]
> +	mem_phys = res.start;
> +	mem_size = resource_size(&res);
> +	mem_region = devm_memremap(ab->dev, mem_phys, mem_size, MEMREMAP_WC);

[Severity: High]
Does this code miss an error check for devm_memremap()?

When devm_memremap() fails in ath11k_ahb_load_m3_firmware() within
drivers/net/wireless/ath/ath11k/ahb.c, it returns an ERR_PTR (such as
ERR_PTR(-ENOMEM)) rather than NULL. The return value is directly assigned
to mem_region without an IS_ERR() check.

[Severity: Medium]
Could this permanent memory mapping risk a secure memory violation?

Because devm_memremap() is used instead of a temporary memremap() and
memunmap() pair, the mapping is kept active for the lifetime of the device.
This unnecessarily consumes vmalloc space, as mem_region is never used again
after the firmware is loaded.

Additionally, on Qualcomm platforms, leaving a kernel mapping open to a memory
region that may be protected by TrustZone after boot risks fatal secure memory
violations (hypervisor panic) due to speculative processor accesses.

[ ... ]
> +	if (!m3_fw->size || m3_fw->size > mem_size) {
> +		ath11k_err(ab, "Invalid firmware size\n");
> +		release_firmware(m3_fw);
> +		return -EINVAL;
> +	}
> +
> +	ret = qcom_mdt_load_no_init(ab->dev, m3_fw, m3_fw_name, mem_region,
> +				    mem_phys, mem_size, &mem_phys);

[Severity: High]
Can this lead to a kernel panic if mem_region is an error pointer?

The mem_region pointer is passed to qcom_mdt_load_no_init(), which only
checks for !mem_region. Since an ERR_PTR is not NULL, the loader proceeds to
dereference the error pointer, computing an address offset and calling memcpy().
This results in a kernel panic due to an invalid memory write.

> +	if (ret)
> +		ath11k_err(ab, "Failed to load MDT segments: %d\n", ret);
> +
> +	release_firmware(m3_fw);
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-ipq5018-qcn6122-mpd-v1-0-86729521a75c@outlook.com?part=4