Re: [PATCH] ASoC: SOF: topology: validate vendor array size before parsing

[Cássio Gabriel Monteiro Pires]

On 6/10/26 13:03, Péter Ujfalusi wrote:
>> diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
>> index 8fc7726aec29..bb6b981e55d1 100644
>> --- a/sound/soc/sof/topology.c
>> +++ b/sound/soc/sof/topology.c
>> @@ -740,10 +740,13 @@ static int sof_parse_token_sets(struct snd_soc_component *scomp,
>>  	int ret;
>>  
>>  	while (array_size > 0 && total < count * token_instance_num) {
>> +		if (array_size < (int)sizeof(*array))
>> +			return -EINVAL;
>> +
>>  		asize = le32_to_cpu(array->size);
>>  
>>  		/* validate asize */
>> -		if (asize < sizeof(*array)) {
>> +		if (asize < (int)sizeof(*array)) {
>>  			dev_err(scomp->dev, "error: invalid array size 0x%x\n",
>>  				asize);
>>  			return -EINVAL;
> 
> I think this only partially right, I would cover a bit more:
> 
> diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
> index 898b94f88706..b0d37ec2bc5e 100644
> --- a/sound/soc/sof/topology.c
> +++ b/sound/soc/sof/topology.c
> @@ -12,6 +12,7 @@
>  #include <linux/device.h>
>  #include <linux/errno.h>
>  #include <linux/firmware.h>
> +#include <linux/overflow.h>
>  #include <linux/workqueue.h>
>  #include <sound/tlv.h>
>  #include <uapi/sound/sof/tokens.h>
> @@ -738,27 +739,43 @@ static int sof_parse_token_sets(struct snd_soc_component *scomp,
>  	size_t offset = 0;
>  	int found = 0;
>  	int total = 0;
> +	int max_tokens;
>  	int asize;
>  	int ret;
>  
> -	while (array_size > 0 && total < count * token_instance_num) {
> +	if (check_mul_overflow(count, token_instance_num, &max_tokens)) {
> +		dev_err(scomp->dev, "%s: token count overflow %d * %d\n",
> +			__func__, count, token_instance_num);
> +		return -EINVAL;
> +	}
> +
> +	while (array_size > 0 && total < max_tokens) {
> +		if (array_size < (int)sizeof(*array)) {
> +			dev_err(scomp->dev,
> +				"%s: invalid remaining array size %d\n",
> +				__func__, array_size);
> +			return -EINVAL;
> +		}
> +
>  		asize = le32_to_cpu(array->size);
>  
>  		/* validate asize */
> -		if (asize < sizeof(*array)) {
> -			dev_err(scomp->dev, "error: invalid array size 0x%x\n",
> -				asize);
> +		if (asize < (int)sizeof(*array)) {
> +			dev_err(scomp->dev, "%s: vendor array too small %d\n",
> +				__func__, asize);
>  			return -EINVAL;
>  		}
>  
>  		/* make sure there is enough data before parsing */
> -		array_size -= asize;
> -		if (array_size < 0) {
> -			dev_err(scomp->dev, "error: invalid array size 0x%x\n",
> -				asize);
> +		if (asize > array_size) {
> +			dev_err(scomp->dev,
> +				"%s: vendor array size %d exceeds remaining data\n",
> +				__func__, asize);
>  			return -EINVAL;
>  		}
>  
> +		array_size -= asize;
> +
>  		/* call correct parser depending on type */
>  		switch (le32_to_cpu(array->type)) {
>  		case SND_SOC_TPLG_TUPLE_TYPE_UUID:
> 

Thank you, this is way more complete.
I will respin a v2.

-- 
Thanks,
Cássio

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature