Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <563e8d01-beba-bff2-54e0-7629c462add0@quicinc.com>
Date:   Thu, 27 Jul 2023 10:12:27 -0700
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 3/3] wifi: ath11k: add firmware-2.bin support
Content-Language: en-US
To:     Kalle Valo <kvalo@kernel.org>, <mhi@lists.linux.dev>
CC:     <ath11k@lists.infradead.org>, <linux-wireless@vger.kernel.org>
References: <20230727100430.3603551-1-kvalo@kernel.org>
 <20230727100430.3603551-4-kvalo@kernel.org>
From:   Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20230727100430.3603551-4-kvalo@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/27/2023 3:04 AM, Kalle Valo wrote:
> From: Anilkumar Kolli <quic_akolli@quicinc.com>
> 
> Firmware IE containers can dynamically provide various information
> what firmware supports. Also it can embed more than one image so
> updating firmware is easy, user just needs to update one file in
> /lib/firmware/.
> 
> The firmware API 2 or higher will use the IE container format, the
> current API 1 will not use the new format but it still is supported
> for some time. Firmware API 2 files are named as firmware-2.bin
> (which contains both amss.bin and m3.bin images) and API 1 files are
> amss.bin and m3.bin.
> 
> Currently ath11k PCI driver provides firmware binary (amss.bin) path to
> MHI driver, MHI driver reads firmware from filesystem and boots it. Add
> provision to read firmware files from ath11k driver and provide the amss.bin
> firmware data and size to MHI using a pointer.
> 
> Currently enum ath11k_fw_features is empty, the patches adding features will
> add the flags.
> 
> With AHB devices there's no amss.bin or m3.bin, so no changes in how AHB
> firmware files are used. But AHB devices can use future additions to the meta
> data, for example in enum ath11k_fw_features.
> 
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-03125-QCAHSPSWPL_V1_V2_SILICONZ_LITE-3.6510.9
> 
> Co-developed-by: P Praneesh <quic_ppranees@quicinc.com>
> Signed-off-by: P Praneesh <quic_ppranees@quicinc.com>
> Signed-off-by: Anilkumar Kolli <quic_akolli@quicinc.com>
> Co-developed-by: Kalle Valo <quic_kvalo@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> ---
>   drivers/net/wireless/ath/ath11k/Makefile |   3 +-
>   drivers/net/wireless/ath/ath11k/core.c   |   8 ++
>   drivers/net/wireless/ath/ath11k/core.h   |  15 +++
>   drivers/net/wireless/ath/ath11k/fw.c     | 157 +++++++++++++++++++++++
>   drivers/net/wireless/ath/ath11k/fw.h     |  27 ++++
>   drivers/net/wireless/ath/ath11k/mhi.c    |  18 ++-
>   drivers/net/wireless/ath/ath11k/qmi.c    |  36 ++++--
>   7 files changed, 247 insertions(+), 17 deletions(-)
>   create mode 100644 drivers/net/wireless/ath/ath11k/fw.c
>   create mode 100644 drivers/net/wireless/ath/ath11k/fw.h
...
> 
> diff --git a/drivers/net/wireless/ath/ath11k/fw.c b/drivers/net/wireless/ath/ath11k/fw.c
> new file mode 100644
> index 000000000000..5423c0be63fa
> --- /dev/null
> +++ b/drivers/net/wireless/ath/ath11k/fw.c
> @@ -0,0 +1,157 @@
> +// SPDX-License-Identifier: BSD-3-Clause-Clear
> +/*
> + * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.

should the year be updated?
same question applies to the .h file

> + */
> +
> +#include "core.h"
> +
> +#include "debug.h"
> +
> +static int ath11k_fw_request_firmware_api_n(struct ath11k_base *ab,
> +					    const char *name)
> +{
> +	size_t magic_len, len, ie_len;
> +	int ie_id, i, index, bit, ret;
> +	struct ath11k_fw_ie *hdr;
> +	const u8 *data;
> +	__le32 *timestamp;
> +
> +	ab->fw.fw = ath11k_core_firmware_request(ab, name);
> +	if (IS_ERR(ab->fw.fw)) {
> +		ret = PTR_ERR(ab->fw.fw);
> +		ath11k_dbg(ab, ATH11K_DBG_BOOT, "failed to load %s: %d\n", name, ret);
> +		ab->fw.fw = NULL;
> +		return ret;
> +	}
> +
> +	data = ab->fw.fw->data;
> +	len = ab->fw.fw->size;
> +
> +	/* magic also includes the null byte, check that as well */
> +	magic_len = strlen(ATH11K_FIRMWARE_MAGIC) + 1;
> +
> +	if (len < magic_len) {
> +		ath11k_err(ab, "firmware image too small to contain magic: %zu\n",
> +			   len);
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	if (memcmp(data, ATH11K_FIRMWARE_MAGIC, magic_len) != 0) {
> +		ath11k_err(ab, "Invalid firmware magic\n");
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	/* jump over the padding */
> +	magic_len = ALIGN(magic_len, 4);
> +
> +	len -= magic_len;
> +	data += magic_len;
> +
> +	/* loop elements */
> +	while (len > sizeof(struct ath11k_fw_ie)) {
> +		hdr = (struct ath11k_fw_ie *)data;
> +
> +		ie_id = le32_to_cpu(hdr->id);
> +		ie_len = le32_to_cpu(hdr->len);
> +
> +		len -= sizeof(*hdr);
> +		data += sizeof(*hdr);
> +
> +		if (len < ie_len) {
> +			ath11k_err(ab, "Invalid length for FW IE %d (%zu < %zu)\n",
> +				   ie_id, len, ie_len);
> +			ret = -EINVAL;
> +			goto err;
> +		}
> +
> +		switch (ie_id) {
> +		case ATH11K_FW_IE_TIMESTAMP:
> +			if (ie_len != sizeof(u32))
> +				break;
> +
> +			timestamp = (__le32 *)data;
> +
> +			ath11k_dbg(ab, ATH11K_DBG_BOOT, "found fw timestamp %d\n",
> +				   le32_to_cpup(timestamp));
> +			break;
> +		case ATH11K_FW_IE_FEATURES:
> +			ath11k_dbg(ab, ATH11K_DBG_BOOT,
> +				   "found firmware features ie (%zd B)\n",
> +				   ie_len);
> +
> +			for (i = 0; i < ATH11K_FW_FEATURE_COUNT; i++) {
> +				index = i / 8;
> +				bit = i % 8;
> +
> +				if (index == ie_len)
> +					break;
> +
> +				if (data[index] & (1 << bit))
> +					__set_bit(i, ab->fw.fw_features);
> +			}
> +
> +			ath11k_dbg_dump(ab, ATH11K_DBG_BOOT, "features", "",
> +					ab->fw.fw_features,
> +					sizeof(ab->fw.fw_features));
> +			break;
> +		case ATH11K_FW_IE_AMSS_IMAGE:
> +			ath11k_dbg(ab, ATH11K_DBG_BOOT,
> +				   "found fw image ie (%zd B)\n",
> +				   ie_len);
> +
> +			ab->fw.amss_data = data;
> +			ab->fw.amss_len = ie_len;
> +			break;
> +		case ATH11K_FW_IE_M3_IMAGE:
> +			ath11k_dbg(ab, ATH11K_DBG_BOOT,
> +				   "found m3 image ie (%zd B)\n",
> +				   ie_len);
> +
> +			ab->fw.m3_data = data;
> +			ab->fw.m3_len = ie_len;
> +			break;
> +		default:
> +			ath11k_warn(ab, "Unknown FW IE: %u\n", ie_id);
> +			break;
> +		}
> +
> +		/* jump over the padding */
> +		ie_len = ALIGN(ie_len, 4);
> +
> +		len -= ie_len;
> +		data += ie_len;

is this always safe?

can we have a case where the original ie_len was <= len but the aligned 
ie_len is > len, and hence this will lead to an integer underflow of len 
(becoming a large unsigned value) and we'll continue looping with a 
buffer overread?

the same question applies to the code where the magic is checked & skipped

> +	};
> +
> +	return 0;
> +
> +err:
> +	release_firmware(ab->fw.fw);
> +	ab->fw.fw = NULL;
> +	return ret;
> +}
> +