[5/5] ALSA: firewire-tascam: change device probing processing
diff mbox

Message ID 1444644625-30189-6-git-send-email-o-takashi@sakamocchi.jp
State New
Headers show

Commit Message

Takashi Sakamoto Oct. 12, 2015, 10:10 a.m. UTC
Currently, this driver picks up model name with be32_to_cpu() macro
to align characters. This is wrong operation because the result is
different depending on CPU endiannness.

Additionally, vendor released several versions of firmware for this
series. It's not better to assign model-dependent information to
device entry according to the version field.

This commit fixes these bugs. The name of model is picked up correctly
and used to identify model-dependent information.

Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
Fixes: c0949b278515('ALSA: firewire-tascam: add skeleton for TASCAM FireWire series')
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/tascam/tascam.c | 78 +++++++++++++++++-------------------------
 1 file changed, 32 insertions(+), 46 deletions(-)

Comments

Stefan Richter Oct. 12, 2015, 12:42 p.m. UTC | #1
On Oct 12 Takashi Sakamoto wrote:
[...]
> Fixes: c0949b278515('ALSA: firewire-tascam: add skeleton for TASCAM FireWire series')

I have some trivial comments, and one off-by-one index error in case
of unexpected Config ROM contents.

[...]
> --- a/sound/firewire/tascam/tascam.c
> +++ b/sound/firewire/tascam/tascam.c
> @@ -24,16 +24,6 @@ static struct snd_tscm_spec model_specs[] = {
>  		.is_controller = true,
>  	},
>  	{
> -		.name = "FW-1804",
> -		.has_adat = true,
> -		.has_spdif = true,
> -		.pcm_capture_analog_channels = 8,
> -		.pcm_playback_analog_channels = 2,
> -		.midi_capture_ports = 2,
> -		.midi_playback_ports = 4,
> -		.is_controller = false,
> -	},
> -	{
>  		.name = "FW-1082",
>  		.has_adat = false,
>  		.has_spdif = true,
> @@ -43,34 +33,46 @@ static struct snd_tscm_spec model_specs[] = {
>  		.midi_playback_ports = 2,
>  		.is_controller = true,
>  	},
> +	/* FW-1804 mey be supported. */

mey -> may  (already fixed in tiwai/sound.git)

>  };
>  
> -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:

	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.

> @@ -108,13 +110,12 @@ static int snd_tscm_probe(struct fw_unit *unit,
>  	tscm = card->private_data;
>  	tscm->card = card;
>  	tscm->unit = fw_unit_get(unit);
> -	tscm->spec = (const struct snd_tscm_spec *)entry->driver_data;
>  
>  	mutex_init(&tscm->mutex);
>  	spin_lock_init(&tscm->lock);
>  	init_waitqueue_head(&tscm->hwdep_wait);
>  
> -	err = check_name(tscm);
> +	err = identify_model(tscm);
>  	if (err < 0)
>  		goto error;
>  
> @@ -172,27 +173,12 @@ static void snd_tscm_remove(struct fw_unit *unit)
>  }
>  
>  static const struct ieee1394_device_id snd_tscm_id_table[] = {
> -	/* FW-1082 */
> -	{
> -		.match_flags = IEEE1394_MATCH_VENDOR_ID |
> -			       IEEE1394_MATCH_SPECIFIER_ID |
> -			       IEEE1394_MATCH_VERSION,
> -		.vendor_id = 0x00022e,
> -		.specifier_id = 0x00022e,
> -		.version = 0x800003,
> -		.driver_data = (kernel_ulong_t)&model_specs[2],
> -	},
> -	/* FW-1884 */
>  	{
>  		.match_flags = IEEE1394_MATCH_VENDOR_ID |
> -			       IEEE1394_MATCH_SPECIFIER_ID |
> -			       IEEE1394_MATCH_VERSION,
> +			       IEEE1394_MATCH_SPECIFIER_ID,
>  		.vendor_id = 0x00022e,
>  		.specifier_id = 0x00022e,
> -		.version = 0x800000,
> -		.driver_data = (kernel_ulong_t)&model_specs[0],
>  	},
> -	/* FW-1804 mey be supported if IDs are clear. */
>  	/* FE-08 requires reverse-engineering because it just has faders. */
>  	{}
>  };
Takashi Sakamoto Oct. 13, 2015, 2:12 p.m. UTC | #2
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

Patch
diff mbox

diff --git a/sound/firewire/tascam/tascam.c b/sound/firewire/tascam/tascam.c
index dc07e3e..3baf1b2 100644
--- a/sound/firewire/tascam/tascam.c
+++ b/sound/firewire/tascam/tascam.c
@@ -24,16 +24,6 @@  static struct snd_tscm_spec model_specs[] = {
 		.is_controller = true,
 	},
 	{
-		.name = "FW-1804",
-		.has_adat = true,
-		.has_spdif = true,
-		.pcm_capture_analog_channels = 8,
-		.pcm_playback_analog_channels = 2,
-		.midi_capture_ports = 2,
-		.midi_playback_ports = 4,
-		.is_controller = false,
-	},
-	{
 		.name = "FW-1082",
 		.has_adat = false,
 		.has_spdif = true,
@@ -43,34 +33,46 @@  static struct snd_tscm_spec model_specs[] = {
 		.midi_playback_ports = 2,
 		.is_controller = true,
 	},
+	/* FW-1804 mey be supported. */
 };
 
-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';
+
+	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);
@@ -108,13 +110,12 @@  static int snd_tscm_probe(struct fw_unit *unit,
 	tscm = card->private_data;
 	tscm->card = card;
 	tscm->unit = fw_unit_get(unit);
-	tscm->spec = (const struct snd_tscm_spec *)entry->driver_data;
 
 	mutex_init(&tscm->mutex);
 	spin_lock_init(&tscm->lock);
 	init_waitqueue_head(&tscm->hwdep_wait);
 
-	err = check_name(tscm);
+	err = identify_model(tscm);
 	if (err < 0)
 		goto error;
 
@@ -172,27 +173,12 @@  static void snd_tscm_remove(struct fw_unit *unit)
 }
 
 static const struct ieee1394_device_id snd_tscm_id_table[] = {
-	/* FW-1082 */
-	{
-		.match_flags = IEEE1394_MATCH_VENDOR_ID |
-			       IEEE1394_MATCH_SPECIFIER_ID |
-			       IEEE1394_MATCH_VERSION,
-		.vendor_id = 0x00022e,
-		.specifier_id = 0x00022e,
-		.version = 0x800003,
-		.driver_data = (kernel_ulong_t)&model_specs[2],
-	},
-	/* FW-1884 */
 	{
 		.match_flags = IEEE1394_MATCH_VENDOR_ID |
-			       IEEE1394_MATCH_SPECIFIER_ID |
-			       IEEE1394_MATCH_VERSION,
+			       IEEE1394_MATCH_SPECIFIER_ID,
 		.vendor_id = 0x00022e,
 		.specifier_id = 0x00022e,
-		.version = 0x800000,
-		.driver_data = (kernel_ulong_t)&model_specs[0],
 	},
-	/* FW-1804 mey be supported if IDs are clear. */
 	/* FE-08 requires reverse-engineering because it just has faders. */
 	{}
 };