Skip to content

Commit

Permalink
ALSA: usb-audio: Fix memory leak and corruption in mixer_us16x08.c
Browse files Browse the repository at this point in the history 
There are a few places leaking memory and doing double-free in
mixer_us16x08.c.

The driver allocates a usb_mixer_elem_info object at each
add_new_ctl() call.  This has to be freed via kctl->private_free, but
currently this is done properly only for some controls.

Also, the driver allocates three external objects (comp_store,
eq_store, meter_store), and these are referred in elem->private_data
(it's not kctl->private_data).  And these have to be released, but
there are none doing it.  Moreover, these extra objects have to be
released only once.  Thus the release should be done only by the first
kctl element that refers to it.

For fixing these, we call either snd_usb_mixer_elem_free() (only for
kctl->private_data) or elem_private_free() (for both
kctl->private_data and elem->private_data) via kctl->private_free
appropriately.

Last but not least, snd_us16x08_controls_create() may return in the
middle without releasing the allocated *_store objects due to an
error.  For fixing this, we shuffle the allocation code so that it's
called just before its reference.

Fixes: d2bb390 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk")
Reported-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
  • Loading branch information
@tiwai
tiwai committed Feb 22, 2017
1 parent 89b593c commit e2810d7
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 54 deletions.
92 changes: 39 additions & 53 deletions sound/usb/mixer_us16x08.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1053,11 +1053,22 @@ static struct snd_us16x08_meter_store *snd_us16x08_create_meter_store(void)

}

/* release elem->private_free as well; called only once for each *_store */
static void elem_private_free(struct snd_kcontrol *kctl)
{
struct usb_mixer_elem_info *elem = kctl->private_data;

if (elem)
kfree(elem->private_data);
kfree(elem);
kctl->private_data = NULL;
}

static int add_new_ctl(struct usb_mixer_interface *mixer,
const struct snd_kcontrol_new *ncontrol,
int index, int val_type, int channels,
const char *name, const void *opt,
void (*freeer)(struct snd_kcontrol *kctl),
bool do_private_free,
struct usb_mixer_elem_info **elem_ret)
{
struct snd_kcontrol *kctl;
Expand Down Expand Up @@ -1085,7 +1096,10 @@ static int add_new_ctl(struct usb_mixer_interface *mixer,
return -ENOMEM;
}

kctl->private_free = freeer;
if (do_private_free)
kctl->private_free = elem_private_free;
else
kctl->private_free = snd_usb_mixer_elem_free;

strlcpy(kctl->id.name, name, sizeof(kctl->id.name));

Expand All @@ -1109,87 +1123,76 @@ static struct snd_us16x08_control_params eq_controls[] = {
.type = USB_MIXER_BOOLEAN,
.num_channels = 16,
.name = "EQ Switch",
.freeer = snd_usb_mixer_elem_free
},
{ /* EQ low gain */
.kcontrol_new = &snd_us16x08_eq_gain_ctl,
.control_id = SND_US16X08_ID_EQLOWLEVEL,
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "EQ Low Volume",
.freeer = snd_usb_mixer_elem_free
},
{ /* EQ low freq */
.kcontrol_new = &snd_us16x08_eq_low_freq_ctl,
.control_id = SND_US16X08_ID_EQLOWFREQ,
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "EQ Low Frequence",
.freeer = NULL
},
{ /* EQ mid low gain */
.kcontrol_new = &snd_us16x08_eq_gain_ctl,
.control_id = SND_US16X08_ID_EQLOWMIDLEVEL,
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "EQ MidLow Volume",
.freeer = snd_usb_mixer_elem_free
},
{ /* EQ mid low freq */
.kcontrol_new = &snd_us16x08_eq_mid_freq_ctl,
.control_id = SND_US16X08_ID_EQLOWMIDFREQ,
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "EQ MidLow Frequence",
.freeer = NULL
},
{ /* EQ mid low Q */
.kcontrol_new = &snd_us16x08_eq_mid_width_ctl,
.control_id = SND_US16X08_ID_EQLOWMIDWIDTH,
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "EQ MidQLow Q",
.freeer = NULL
},
{ /* EQ mid high gain */
.kcontrol_new = &snd_us16x08_eq_gain_ctl,
.control_id = SND_US16X08_ID_EQHIGHMIDLEVEL,
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "EQ MidHigh Volume",
.freeer = snd_usb_mixer_elem_free
},
{ /* EQ mid high freq */
.kcontrol_new = &snd_us16x08_eq_mid_freq_ctl,
.control_id = SND_US16X08_ID_EQHIGHMIDFREQ,
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "EQ MidHigh Frequence",
.freeer = NULL
},
{ /* EQ mid high Q */
.kcontrol_new = &snd_us16x08_eq_mid_width_ctl,
.control_id = SND_US16X08_ID_EQHIGHMIDWIDTH,
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "EQ MidHigh Q",
.freeer = NULL
},
{ /* EQ high gain */
.kcontrol_new = &snd_us16x08_eq_gain_ctl,
.control_id = SND_US16X08_ID_EQHIGHLEVEL,
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "EQ High Volume",
.freeer = snd_usb_mixer_elem_free
},
{ /* EQ low freq */
.kcontrol_new = &snd_us16x08_eq_high_freq_ctl,
.control_id = SND_US16X08_ID_EQHIGHFREQ,
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "EQ High Frequence",
.freeer = NULL
},
};

Expand All @@ -1201,47 +1204,41 @@ static struct snd_us16x08_control_params comp_controls[] = {
.type = USB_MIXER_BOOLEAN,
.num_channels = 16,
.name = "Compressor Switch",
.freeer = snd_usb_mixer_elem_free
},
{ /* Comp threshold */
.kcontrol_new = &snd_us16x08_comp_threshold_ctl,
.control_id = SND_US16X08_ID_COMP_THRESHOLD,
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "Compressor Threshold Volume",
.freeer = NULL
},
{ /* Comp ratio */
.kcontrol_new = &snd_us16x08_comp_ratio_ctl,
.control_id = SND_US16X08_ID_COMP_RATIO,
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "Compressor Ratio",
.freeer = NULL
},
{ /* Comp attack */
.kcontrol_new = &snd_us16x08_comp_attack_ctl,
.control_id = SND_US16X08_ID_COMP_ATTACK,
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "Compressor Attack",
.freeer = NULL
},
{ /* Comp release */
.kcontrol_new = &snd_us16x08_comp_release_ctl,
.control_id = SND_US16X08_ID_COMP_RELEASE,
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "Compressor Release",
.freeer = NULL
},
{ /* Comp gain */
.kcontrol_new = &snd_us16x08_comp_gain_ctl,
.control_id = SND_US16X08_ID_COMP_GAIN,
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "Compressor Volume",
.freeer = NULL
},
};

Expand All @@ -1253,7 +1250,6 @@ static struct snd_us16x08_control_params channel_controls[] = {
.type = USB_MIXER_BOOLEAN,
.num_channels = 16,
.name = "Phase Switch",
.freeer = snd_usb_mixer_elem_free,
.default_val = 0
},
{ /* Fader */
Expand All @@ -1262,7 +1258,6 @@ static struct snd_us16x08_control_params channel_controls[] = {
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "Line Volume",
.freeer = NULL,
.default_val = 127
},
{ /* Mute */
Expand All @@ -1271,7 +1266,6 @@ static struct snd_us16x08_control_params channel_controls[] = {
.type = USB_MIXER_BOOLEAN,
.num_channels = 16,
.name = "Mute Switch",
.freeer = NULL,
.default_val = 0
},
{ /* Pan */
Expand All @@ -1280,7 +1274,6 @@ static struct snd_us16x08_control_params channel_controls[] = {
.type = USB_MIXER_U16,
.num_channels = 16,
.name = "Pan Left-Right Volume",
.freeer = NULL,
.default_val = 127
},
};
Expand All @@ -1293,7 +1286,6 @@ static struct snd_us16x08_control_params master_controls[] = {
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "Master Volume",
.freeer = NULL,
.default_val = 127
},
{ /* Bypass */
Expand All @@ -1302,7 +1294,6 @@ static struct snd_us16x08_control_params master_controls[] = {
.type = USB_MIXER_BOOLEAN,
.num_channels = 16,
.name = "DSP Bypass Switch",
.freeer = NULL,
.default_val = 0
},
{ /* Buss out */
Expand All @@ -1311,7 +1302,6 @@ static struct snd_us16x08_control_params master_controls[] = {
.type = USB_MIXER_BOOLEAN,
.num_channels = 16,
.name = "Buss Out Switch",
.freeer = NULL,
.default_val = 0
},
{ /* Master mute */
Expand All @@ -1320,7 +1310,6 @@ static struct snd_us16x08_control_params master_controls[] = {
.type = USB_MIXER_BOOLEAN,
.num_channels = 16,
.name = "Master Mute Switch",
.freeer = NULL,
.default_val = 0
},

Expand All @@ -1338,30 +1327,10 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
/* just check for non-MIDI interface */
if (mixer->hostif->desc.bInterfaceNumber == 3) {

/* create compressor mixer elements */
comp_store = snd_us16x08_create_comp_store();
if (comp_store == NULL)
return -ENOMEM;

/* create eq store */
eq_store = snd_us16x08_create_eq_store();
if (eq_store == NULL) {
kfree(comp_store);
return -ENOMEM;
}

/* create meters store */
meter_store = snd_us16x08_create_meter_store();
if (meter_store == NULL) {
kfree(comp_store);
kfree(eq_store);
return -ENOMEM;
}

/* add routing control */
err = add_new_ctl(mixer, &snd_us16x08_route_ctl,
SND_US16X08_ID_ROUTE, USB_MIXER_U8, 8, "Line Out Route",
NULL, NULL, &elem);
NULL, false, &elem);
if (err < 0) {
usb_audio_dbg(mixer->chip,
"Failed to create route control, err:%d\n",
Expand All @@ -1372,6 +1341,11 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
elem->cache_val[i] = i < 2 ? i : i + 2;
elem->cached = 0xff;

/* create compressor mixer elements */
comp_store = snd_us16x08_create_comp_store();
if (!comp_store)
return -ENOMEM;

/* add master controls */
for (i = 0;
i < sizeof(master_controls)
Expand All @@ -1385,7 +1359,8 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
master_controls[i].num_channels,
master_controls[i].name,
comp_store,
master_controls[i].freeer, &elem);
i == 0, /* release comp_store only once */
&elem);
if (err < 0)
return err;
elem->cache_val[0] = master_controls[i].default_val;
Expand All @@ -1405,7 +1380,7 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
channel_controls[i].num_channels,
channel_controls[i].name,
comp_store,
channel_controls[i].freeer, &elem);
false, &elem);
if (err < 0)
return err;
for (j = 0; j < SND_US16X08_MAX_CHANNELS; j++) {
Expand All @@ -1415,6 +1390,11 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
elem->cached = 0xffff;
}

/* create eq store */
eq_store = snd_us16x08_create_eq_store();
if (!eq_store)
return -ENOMEM;

/* add EQ controls */
for (i = 0; i < sizeof(eq_controls) /
sizeof(control_params); i++) {
Expand All @@ -1426,7 +1406,8 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
eq_controls[i].num_channels,
eq_controls[i].name,
eq_store,
eq_controls[i].freeer, NULL);
i == 0, /* release eq_store only once */
NULL);
if (err < 0)
return err;
}
Expand All @@ -1444,18 +1425,23 @@ int snd_us16x08_controls_create(struct usb_mixer_interface *mixer)
comp_controls[i].num_channels,
comp_controls[i].name,
comp_store,
comp_controls[i].freeer, NULL);
false, NULL);
if (err < 0)
return err;
}

/* create meters store */
meter_store = snd_us16x08_create_meter_store();
if (!meter_store)
return -ENOMEM;

/* meter function 'get' must access to compressor store
* so place a reference here
*/
meter_store->comp_store = comp_store;
err = add_new_ctl(mixer, &snd_us16x08_meter_ctl,
SND_US16X08_ID_METER, USB_MIXER_U16, 0, "Level Meter",
(void *) meter_store, snd_usb_mixer_elem_free, NULL);
meter_store, true, NULL);
if (err < 0)
return err;
}
Expand Down
1 change: 0 additions & 1 deletion sound/usb/mixer_us16x08.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ struct snd_us16x08_control_params {
int type;
int num_channels;
const char *name;
void (*freeer)(struct snd_kcontrol *kctl);
int default_val;
};

Expand Down

0 comments on commit e2810d7

Please sign in to comment.