RE: [PATCH v7 03/14] ASoC: Intel: catpt: Add IPC message handlers

["Rojewski, Cezary" <cezary.rojewski@intel.com>]

On 2020-09-21 8:41 PM, Andy Shevchenko wrote:> On Mon, Sep 21, 2020 at 06:13:59PM +0000, Rojewski, Cezary wrote:
>> On 2020-09-21 2:59 PM, Andy Shevchenko wrote:
>>> On Mon, Sep 21, 2020 at 01:54:13PM +0200, Cezary Rojewski wrote:
...

>>>> +	for (i = j = 0; i < FW_INFO_SIZE_MAX; i++)
>>>> +		if (cdev->ipc.config.fw_info[i] == ' ')
>>>> +			if (++j == 4)
>>>> +				break;
>>>
>>>> +	for (j = ++i; j < FW_INFO_SIZE_MAX && j - i < 20; j++) {
>>>
>>> This should have static_assert() at the place where you define both constants
>>> (2nd is mentioned above 20).
>>>
>>>> +		if (cdev->ipc.config.fw_info[j] == ' ')
>>>> +			break;
>>>> +		*(pos + j - i) = cdev->ipc.config.fw_info[j];
>>>> +	}
>>>> +	pos += 20;
>>>
>>> These two for-loops should have some comment to explain what's going on.
>>>
>>
>> Actually, after poking my FW friends again I realized that it's just
>> dumping 20chars from "hash" segment of fw_info (struct catpt_fw_ready,
>> field: fw_info[]).
>>
>> So, this could be replaced by:
>>
> 
>> 	/* navigate to fifth info segment (fw hash) */
>> 	for (i = j = 0; i < FW_INFO_SIZE_MAX; i++)
>> 		/* info segments are separated by space each */
>> 		if (cdev->ipc.config.fw_info[i] == ' ')
>> 			if (++j == 4)
>> 				break;
> 
> ...and this is repeating strnchr() / strnchrnul().
> 

Indeed, will incorporate into above.

> With the questions "what if...":
>   - nul in the middle of this?
>   - less than 4 spaces found?
> 

While this should never happen (means user is somehow not making use of
officially released firmware binary), coredumps are useful only if you
have access to debug tools. In cases you'd mentioned, invalid hash would
have been dumped to coredump and crash reader simply wouldn't have been
able to navigate to actual build for it. The rest of the coredump is still
vital though.

memcpy() could be gated behind an 'if' for safety if needed:

	info = cdev->ipc.config.fw_info;
	eof = info + FW_INFO_SIZE_MAX;
	/* navigate to fifth info segment (fw hash) */
	for (i = 0; i < 4 && info < eof; i++, info++)
		/* info segments are separated by space each */
		if ((info = strnchr(info, eof - info, ' ')) == NULL)
			break;

	if (i == 4 && info < eof)
		memcpy(pos, info, min(eof - info, CATPT_DUMP_HASH_SIZE));

Didn't compile this, some typecheck fixes might be in order and so on.

>> 	memcpy(pos, &cdev->ipc.config.fw_info[++i], CATPT_DUMP_HASH_SIZE);
>> 	pos += CATPT_DUMP_HASH_SIZE;
>>
>>
>> Existing for-loops were based on internal solution. Half of the code
>> isn't needed afterall..
>