Re: [PATCH 5/5] ALSA: firewire-tascam: change device probing processing

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

On Oct 12 2015 21:42, Stefan Richter wrote:
>> -static int check_name(struct snd_tscm *tscm)
>> +static int identify_model(struct snd_tscm *tscm)
>>  {
>>  	struct fw_device *fw_dev = fw_parent_device(tscm->unit);
>> -	char vendor[8];
>> +	const u32 *config_rom = fw_dev->config_rom;
>>  	char model[8];
>> -	__u32 data;
>> -
>> -	/* Retrieve model name. */
>> -	data = be32_to_cpu(fw_dev->config_rom[28]);
>> -	memcpy(model, &data, 4);
>> -	data = be32_to_cpu(fw_dev->config_rom[29]);
>> -	memcpy(model + 4, &data, 4);
>> -	model[7] = '\0';
>> -
>> -	/* Retrieve vendor name. */
>> -	data = be32_to_cpu(fw_dev->config_rom[23]);
>> -	memcpy(vendor, &data, 4);
>> -	data = be32_to_cpu(fw_dev->config_rom[24]);
>> -	memcpy(vendor + 4, &data, 4);
>> -	vendor[7] = '\0';
>> +	unsigned int i;
>> +	u8 c;
>> +
>> +	if (fw_dev->config_rom_length < 30) {
>> +		dev_err(&tscm->unit->device,
>> +			"Configuration ROM is too short.\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	/* Pick up model name from certain addresses. */
>> +	for (i = 0; i < 8; i++) {
>> +		c = config_rom[28 + i / 4] >> (24 - 8 * (i % 4));
>> +		if (c == '\0')
>> +			break;
>> +		model[i] = c;
>> +	}
>> +	model[i] = '\0';
> 
> You could get a buffer overrun here.  Perhaps only go to i < 7:

Indeed, thanks.

> 	for (i = 0; i < 7; i++) {
> 		[...]
> 	}
> 	model[i] = '\0';
> 
>> +	for (i = 0; i < ARRAY_SIZE(model_specs); i++) {
>> +		if (strcmp(model, model_specs[i].name) == 0) {
>> +			tscm->spec = &model_specs[i];
>> +			break;
>> +		}
>> +	}
>> +	if (tscm->spec == NULL)
>> +		return -ENODEV;
>>  
>>  	strcpy(tscm->card->driver, "FW-TASCAM");
>>  	strcpy(tscm->card->shortname, model);
>>  	strcpy(tscm->card->mixername, model);
>>  	snprintf(tscm->card->longname, sizeof(tscm->card->longname),
>> -		 "%s %s, GUID %08x%08x at %s, S%d", vendor, model,
>> +		 "TASCAM %s, GUID %08x%08x at %s, S%d", model,
>>  		 cpu_to_be32(fw_dev->config_rom[3]),
>>  		 cpu_to_be32(fw_dev->config_rom[4]),
>>  		 dev_name(&tscm->unit->device), 100 << fw_dev->max_speed);
> 
> Should be
> 		fw_dev->config_rom[3],
> 		fw_dev->config_rom[4],
> 
> since snprintf wants CPU-endian values.

Firewire-digi00x also includes the same bug.

I found some endianness bug in the other modules. I'll fixed these bugs
in the same series of patches later.


Thanks

Takashi Sakamoto