[PATCH] ASoC: Intel: avs: validate configuration TLV bounds

[PATCH] ASoC: Intel: avs: validate configuration TLV bounds

[Pengpeng Hou]

Firmware and hardware configuration replies are parsed as a sequence of
TLVs. The walkers only check that the complete reply is non-empty before
casting the current offset to struct avs_tlv, reading its header, and
consuming type-specific values. A short header or a declared value that
extends past the reply can therefore be consumed as part of the current
item.

Validate the fixed TLV header and the declared value span before dispatch.
Also require the value space used by the selected type: firmware version
copies need the complete version structure, scalar configuration values
need one u32, and I2S capabilities need their two-word prefix plus all
declared controller addresses.

Fixes: 3322e1688953 ("ASoC: Intel: avs: Add basefw runtime-parameter requests")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 sound/soc/intel/avs/messages.c | 114 +++++++++++++++++++++++++--------
 1 file changed, 88 insertions(+), 26 deletions(-)

diff --git a/sound/soc/intel/avs/messages.c b/sound/soc/intel/avs/messages.c
index a5ba27983091..c8adbb0de20c 100644
--- a/sound/soc/intel/avs/messages.c
+++ b/sound/soc/intel/avs/messages.c
@@ -388,9 +388,33 @@ int avs_ipc_set_d0ix(struct avs_dev *adev, bool enable_pg, bool streaming)
 	return avs_dsp_send_pm_msg(adev, &request, NULL, false, "set d0ix");
 }
 
+static int avs_tlv_validate(const struct avs_tlv *tlv, size_t remaining,
+			    size_t value_size)
+{
+	if (remaining < sizeof(*tlv) ||
+	    tlv->length > remaining - sizeof(*tlv) ||
+	    tlv->length < value_size)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int avs_tlv_read_u32(const struct avs_tlv *tlv, size_t remaining,
+			    u32 *value)
+{
+	int ret;
+
+	ret = avs_tlv_validate(tlv, remaining, sizeof(*tlv->value));
+	if (!ret)
+		*value = *tlv->value;
+
+	return ret;
+}
+
 int avs_ipc_get_fw_config(struct avs_dev *adev, struct avs_fw_cfg *cfg)
 {
 	struct avs_tlv *tlv;
+	u32 *value;
 	size_t payload_size;
 	size_t offset = 0;
 	u8 *payload;
@@ -409,82 +433,91 @@ int avs_ipc_get_fw_config(struct avs_dev *adev, struct avs_fw_cfg *cfg)
 
 	while (offset < payload_size) {
 		tlv = (struct avs_tlv *)(payload + offset);
+		ret = avs_tlv_validate(tlv, payload_size - offset, 0);
+		if (ret)
+			goto exit;
+
+		value = NULL;
 
 		switch (tlv->type) {
 		case AVS_FW_CFG_FW_VERSION:
+			ret = avs_tlv_validate(tlv, payload_size - offset,
+					       sizeof(cfg->fw_version));
+			if (ret)
+				goto exit;
 			memcpy(&cfg->fw_version, tlv->value, sizeof(cfg->fw_version));
 			break;
 
 		case AVS_FW_CFG_MEMORY_RECLAIMED:
-			cfg->memory_reclaimed = *tlv->value;
+			value = &cfg->memory_reclaimed;
 			break;
 
 		case AVS_FW_CFG_SLOW_CLOCK_FREQ_HZ:
-			cfg->slow_clock_freq_hz = *tlv->value;
+			value = &cfg->slow_clock_freq_hz;
 			break;
 
 		case AVS_FW_CFG_FAST_CLOCK_FREQ_HZ:
-			cfg->fast_clock_freq_hz = *tlv->value;
+			value = &cfg->fast_clock_freq_hz;
 			break;
 
 		case AVS_FW_CFG_ALH_SUPPORT_LEVEL:
-			cfg->alh_support = *tlv->value;
+			value = &cfg->alh_support;
 			break;
 
 		case AVS_FW_CFG_IPC_DL_MAILBOX_BYTES:
-			cfg->ipc_dl_mailbox_bytes = *tlv->value;
+			value = &cfg->ipc_dl_mailbox_bytes;
 			break;
 
 		case AVS_FW_CFG_IPC_UL_MAILBOX_BYTES:
-			cfg->ipc_ul_mailbox_bytes = *tlv->value;
+			value = &cfg->ipc_ul_mailbox_bytes;
 			break;
 
 		case AVS_FW_CFG_TRACE_LOG_BYTES:
-			cfg->trace_log_bytes = *tlv->value;
+			value = &cfg->trace_log_bytes;
 			break;
 
 		case AVS_FW_CFG_MAX_PPL_COUNT:
-			cfg->max_ppl_count = *tlv->value;
+			value = &cfg->max_ppl_count;
 			break;
 
 		case AVS_FW_CFG_MAX_ASTATE_COUNT:
-			cfg->max_astate_count = *tlv->value;
+			value = &cfg->max_astate_count;
 			break;
 
 		case AVS_FW_CFG_MAX_MODULE_PIN_COUNT:
-			cfg->max_module_pin_count = *tlv->value;
+			value = &cfg->max_module_pin_count;
 			break;
 
 		case AVS_FW_CFG_MODULES_COUNT:
-			cfg->modules_count = *tlv->value;
+			value = &cfg->modules_count;
 			break;
 
 		case AVS_FW_CFG_MAX_MOD_INST_COUNT:
-			cfg->max_mod_inst_count = *tlv->value;
+			value = &cfg->max_mod_inst_count;
 			break;
 
 		case AVS_FW_CFG_MAX_LL_TASKS_PER_PRI_COUNT:
-			cfg->max_ll_tasks_per_pri_count = *tlv->value;
+			value = &cfg->max_ll_tasks_per_pri_count;
 			break;
 
 		case AVS_FW_CFG_LL_PRI_COUNT:
-			cfg->ll_pri_count = *tlv->value;
+			value = &cfg->ll_pri_count;
 			break;
 
 		case AVS_FW_CFG_MAX_DP_TASKS_COUNT:
-			cfg->max_dp_tasks_count = *tlv->value;
+			value = &cfg->max_dp_tasks_count;
 			break;
 
 		case AVS_FW_CFG_MAX_LIBS_COUNT:
-			cfg->max_libs_count = *tlv->value;
+			value = &cfg->max_libs_count;
 			break;
 
 		case AVS_FW_CFG_XTAL_FREQ_HZ:
-			cfg->xtal_freq_hz = *tlv->value;
+			value = &cfg->xtal_freq_hz;
 			break;
 
 		case AVS_FW_CFG_POWER_GATING_POLICY:
-			cfg->power_gating_policy = *tlv->value;
+			value = &cfg->power_gating_policy;
 			break;
 
 		/* Known but not useful to us. */
@@ -499,9 +532,16 @@ int avs_ipc_get_fw_config(struct avs_dev *adev, struct avs_fw_cfg *cfg)
 			break;
 		}
 
+		if (value) {
+			ret = avs_tlv_read_u32(tlv, payload_size - offset, value);
+			if (ret)
+				goto exit;
+		}
+
 		offset += sizeof(*tlv) + tlv->length;
 	}
 
+exit:
 	/* No longer needed, free it as it's owned by the get_large_config() caller. */
 	kfree(payload);
 err:
@@ -551,6 +591,7 @@ int avs_ipc_set_fw_config(struct avs_dev *adev, size_t num_tlvs, ...)
 int avs_ipc_get_hw_config(struct avs_dev *adev, struct avs_hw_cfg *cfg)
 {
 	struct avs_tlv *tlv;
+	u32 *value;
 	size_t payload_size;
 	size_t size, offset = 0;
 	u8 *payload;
@@ -569,31 +610,46 @@ int avs_ipc_get_hw_config(struct avs_dev *adev, struct avs_hw_cfg *cfg)
 
 	while (offset < payload_size) {
 		tlv = (struct avs_tlv *)(payload + offset);
+		ret = avs_tlv_validate(tlv, payload_size - offset, 0);
+		if (ret)
+			goto exit;
+
+		value = NULL;
 
 		switch (tlv->type) {
 		case AVS_HW_CFG_AVS_VER:
-			cfg->avs_version = *tlv->value;
+			value = &cfg->avs_version;
 			break;
 
 		case AVS_HW_CFG_DSP_CORES:
-			cfg->dsp_cores = *tlv->value;
+			value = &cfg->dsp_cores;
 			break;
 
 		case AVS_HW_CFG_MEM_PAGE_BYTES:
-			cfg->mem_page_bytes = *tlv->value;
+			value = &cfg->mem_page_bytes;
 			break;
 
 		case AVS_HW_CFG_TOTAL_PHYS_MEM_PAGES:
-			cfg->total_phys_mem_pages = *tlv->value;
+			value = &cfg->total_phys_mem_pages;
 			break;
 
 		case AVS_HW_CFG_I2S_CAPS:
+			ret = avs_tlv_validate(tlv, payload_size - offset,
+					       2 * sizeof(*tlv->value));
+			if (ret)
+				goto exit;
 			cfg->i2s_caps.i2s_version = tlv->value[0];
 			size = tlv->value[1];
 			cfg->i2s_caps.ctrl_count = size;
 			if (!size)
 				break;
 
+			if (size > (tlv->length - 2 * sizeof(*tlv->value)) /
+				    sizeof(*tlv->value)) {
+				ret = -EINVAL;
+				goto exit;
+			}
+
 			/* Multiply to get entire array size. */
 			size *= sizeof(*cfg->i2s_caps.ctrl_base_addr);
 			cfg->i2s_caps.ctrl_base_addr = devm_kmemdup(adev->dev,
@@ -606,19 +662,19 @@ int avs_ipc_get_hw_config(struct avs_dev *adev, struct avs_hw_cfg *cfg)
 			break;
 
 		case AVS_HW_CFG_GATEWAY_COUNT:
-			cfg->gateway_count = *tlv->value;
+			value = &cfg->gateway_count;
 			break;
 
 		case AVS_HW_CFG_HP_EBB_COUNT:
-			cfg->hp_ebb_count = *tlv->value;
+			value = &cfg->hp_ebb_count;
 			break;
 
 		case AVS_HW_CFG_LP_EBB_COUNT:
-			cfg->lp_ebb_count = *tlv->value;
+			value = &cfg->lp_ebb_count;
 			break;
 
 		case AVS_HW_CFG_EBB_SIZE_BYTES:
-			cfg->ebb_size_bytes = *tlv->value;
+			value = &cfg->ebb_size_bytes;
 			break;
 
 		case AVS_HW_CFG_GPDMA_CAPS:
@@ -629,6 +685,12 @@ int avs_ipc_get_hw_config(struct avs_dev *adev, struct avs_hw_cfg *cfg)
 			break;
 		}
 
+		if (value) {
+			ret = avs_tlv_read_u32(tlv, payload_size - offset, value);
+			if (ret)
+				goto exit;
+		}
+
 		offset += sizeof(*tlv) + tlv->length;
 	}
 
-- 
2.43.0

Re: [PATCH] ASoC: Intel: avs: validate configuration TLV bounds

[Cezary Rojewski]

On 7/15/2026 10:33 AM, Pengpeng Hou wrote:
> Firmware and hardware configuration replies are parsed as a sequence of
> TLVs. The walkers only check that the complete reply is non-empty before
> casting the current offset to struct avs_tlv, reading its header, and
> consuming type-specific values. A short header or a declared value that
> extends past the reply can therefore be consumed as part of the current
> item.
> 
> Validate the fixed TLV header and the declared value span before dispatch.
> Also require the value space used by the selected type: firmware version
> copies need the complete version structure, scalar configuration values
> need one u32, and I2S capabilities need their two-word prefix plus all
> declared controller addresses.

Hello Pengpeng,

I understand where are you coming from but there is no reason for such 
defense - as per Intel audio driver architecture, firmware is the root 
of trust.  If one breaches the verification mechanism, the TLVs are the 
least of our problems.

At the same time, struct avs_tlv is no different from any TLV in the 
kernel so I'd rather see the functions you're adding here applied to the 
kernel/ or drivers/base or other common location, not hidden within 
sound/soc/intel/avs/.

TLDR: NAK

Kind regards,
Czarek

Re: [PATCH] ASoC: Intel: avs: validate configuration TLV bounds

[Pengpeng Hou]

Hi Czarek,

Understood. AVS firmware is the root of trust in this architecture, so
this driver-local TLV validation is not appropriate. I will drop the
patch.

Thanks,
Pengpeng