[alsa-devel] [PATCH 4/5] ALSA: usb-audio: deallocate memory objects in error path
Takashi Iwai
tiwai at suse.de
Wed Feb 22 08:44:13 CET 2017
On Wed, 22 Feb 2017 02:42:39 +0100,
Takashi Sakamoto wrote:
>
> On Feb 22 2017 07:59, Takashi Sakamoto wrote:
> > On Feb 22 2017 07:45, Takashi Sakamoto wrote:
> >> On Fe 21 2017 11:32, Takashi Sakamoto wrote:
> >>> But it's wrong. The allocated memory objects are used for private data
> >>> for each control element set. I didn't care of it...
> >>>
> >>> Here, what I should fix is just for error path. I'll post revised
> >>> version of this patch this night.
> >>
> >> I realize that this is not concern because the code includes assignment
> >> deallocate function to each control element set. Thus, in error path,
> >> the allocated private data is going to free in below callgrach.
> >>
> >> (sound/usb/mixer_us16x08.c)
> >> add_new_ctl(elem)
> >> kctl->private_free = freeer;
> >> (sound/usb/mixer.c)
> >> ->snd_usb_mixer_add_control(kctl)
> >> (sound/core/control.c)
> >> ->snd_ctl_add(kctl)
> >> ->snd_ctl_free_one(kctl)
> >> (in error path)
> >> ->kcontrol->private_free(kctl);
> >> (sound/usb/mixer_us16x08.c)
> >> = freeer()
> >
> > Oops. On the other hand, the private data for unregistered control
> > element set is not deallocated automatically in error path of the other
> > control element set. It should be fixed...
>
> Mmm. There might be an issue of double free corruption, I think.
>
> >> (sound/core/control.c)
> >> ->snd_ctl_add(kctl)
> >> (in error path)
> >> ->snd_ctl_free_one(kctl)
> >> ->kcontrol->private_free(kctl);
> >> (sound/usb/mixer_us16x08.c)
> >> = freeer()
>
> This 'freeer()' is set in beginning of the graph.
>
> >> (sound/usb/mixer_us16x08.c)
> >> add_new_ctl(elem)
> >> kctl->private_free = freeer;
>
> The memory objects are not allocated for each of the control element
> set. On the other hand, the calls of the 'freeer()' are corresponding
> to each control element set. When the ratio between
> .private_data/.private_free and control element set is 1:N,
> .private_free can cause double free corruption at freeing control
> character device in kernel side.
>
> In current code, there're two cases; NULL or
> 'snd_usb_mixer_elem_free()' are assigned as the 'freeer()'. But this
> is not enough. Actually, the allocated memory object for the
> 'comp_store' variable is going to be deallocated by two control
> element sets; for 'channel_controls[0]' and 'channel_controls[0]' the
> allocated memory object for the 'eq_sotre' variable is going to be
> deallocated for several control element sets; the 'eq_controls[0, 1,
> 3, 6, 9]'.
>
> As long as I understand, current code has the above issue and can
> bring kernel corruption. If I missing something, please inform it to
> me.
Yeah, there are quite a few memory leaks and double frees.
The patch below is a bit of drastic surgery.
Takashi
-- 8< --
From: Takashi Iwai <tiwai at suse.de>
Subject: [PATCH] ALSA: usb-audio: Fix memory leak and corruption in
mixer_us16x08.c
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.
returns in the middle due to an error. For fixing this, we shuffle
the allocation code so that it's called just before its reference.
Fixes: d2bb390a2081 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk")
Reported-by: Takashi Sakamoto <o-takashi at sakamocchi.jp>
Signed-off-by: Takashi Iwai <tiwai at suse.de>
---
sound/usb/mixer_us16x08.c | 92 ++++++++++++++++++++---------------------------
sound/usb/mixer_us16x08.h | 1 -
2 files changed, 39 insertions(+), 54 deletions(-)
diff --git a/sound/usb/mixer_us16x08.c b/sound/usb/mixer_us16x08.c
index 73a0b9afdd70..f7289541fbce 100644
--- a/sound/usb/mixer_us16x08.c
+++ b/sound/usb/mixer_us16x08.c
@@ -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;
@@ -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));
@@ -1109,7 +1123,6 @@ 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,
@@ -1117,7 +1130,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
.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,
@@ -1125,7 +1137,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "EQ Low Frequence",
- .freeer = NULL
},
{ /* EQ mid low gain */
.kcontrol_new = &snd_us16x08_eq_gain_ctl,
@@ -1133,7 +1144,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
.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,
@@ -1141,7 +1151,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
.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,
@@ -1149,7 +1158,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "EQ MidQLow Q",
- .freeer = NULL
},
{ /* EQ mid high gain */
.kcontrol_new = &snd_us16x08_eq_gain_ctl,
@@ -1157,7 +1165,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
.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,
@@ -1165,7 +1172,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
.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,
@@ -1173,7 +1179,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "EQ MidHigh Q",
- .freeer = NULL
},
{ /* EQ high gain */
.kcontrol_new = &snd_us16x08_eq_gain_ctl,
@@ -1181,7 +1186,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
.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,
@@ -1189,7 +1193,6 @@ static struct snd_us16x08_control_params eq_controls[] = {
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "EQ High Frequence",
- .freeer = NULL
},
};
@@ -1201,7 +1204,6 @@ 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,
@@ -1209,7 +1211,6 @@ static struct snd_us16x08_control_params comp_controls[] = {
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "Compressor Threshold Volume",
- .freeer = NULL
},
{ /* Comp ratio */
.kcontrol_new = &snd_us16x08_comp_ratio_ctl,
@@ -1217,7 +1218,6 @@ static struct snd_us16x08_control_params comp_controls[] = {
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "Compressor Ratio",
- .freeer = NULL
},
{ /* Comp attack */
.kcontrol_new = &snd_us16x08_comp_attack_ctl,
@@ -1225,7 +1225,6 @@ static struct snd_us16x08_control_params comp_controls[] = {
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "Compressor Attack",
- .freeer = NULL
},
{ /* Comp release */
.kcontrol_new = &snd_us16x08_comp_release_ctl,
@@ -1233,7 +1232,6 @@ static struct snd_us16x08_control_params comp_controls[] = {
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "Compressor Release",
- .freeer = NULL
},
{ /* Comp gain */
.kcontrol_new = &snd_us16x08_comp_gain_ctl,
@@ -1241,7 +1239,6 @@ static struct snd_us16x08_control_params comp_controls[] = {
.type = USB_MIXER_U8,
.num_channels = 16,
.name = "Compressor Volume",
- .freeer = NULL
},
};
@@ -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 */
@@ -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 */
@@ -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 */
@@ -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
},
};
@@ -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 */
@@ -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 */
@@ -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 */
@@ -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
},
@@ -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",
@@ -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)
@@ -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;
@@ -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++) {
@@ -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++) {
@@ -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;
}
@@ -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;
}
diff --git a/sound/usb/mixer_us16x08.h b/sound/usb/mixer_us16x08.h
index 64f89b5eca2d..a6312fb0f962 100644
--- a/sound/usb/mixer_us16x08.h
+++ b/sound/usb/mixer_us16x08.h
@@ -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;
};
--
2.11.1
More information about the Alsa-devel
mailing list