From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from fldsmtpe03.verizon.com ([140.108.26.142]:32876 "EHLO fldsmtpe03.verizon.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932288AbdDDTfB (ORCPT ); Tue, 4 Apr 2017 15:35:01 -0400 From: alexander.levin@verizon.com To: "gregkh@linuxfoundation.org" CC: "stable@vger.kernel.org" Subject: [PATCH for 4.9 88/98] ALSA: usb-audio: Fix memory leak and corruption in mixer_us16x08.c Date: Tue, 4 Apr 2017 19:32:35 +0000 Message-ID: <20170404193158.19041-89-alexander.levin@verizon.com> References: <20170404193158.19041-1-alexander.levin@verizon.com> In-Reply-To: <20170404193158.19041-1-alexander.levin@verizon.com> Content-Language: en-US Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Sender: stable-owner@vger.kernel.org List-ID: From: Takashi Iwai [ Upstream commit e2810d76c5f3b0152fa0f7c40170e123b33e058c ] 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: d2bb390a2081 ("ALSA: usb-audio: Tascam US-16x08 DSP mixer quirk") Reported-by: Takashi Sakamoto Reviewed-by: Takashi Sakamoto Signed-off-by: Takashi Iwai Signed-off-by: Sasha Levin --- 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 301939b..7f78127 100644 --- a/sound/usb/mixer_us16x08.c +++ b/sound/usb/mixer_us16x08.c @@ -1053,11 +1053,22 @@ struct snd_us16x08_meter_store *snd_us16x08_create_= meter_store(void) =20 } =20 +/* 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 =3D kctl->private_data; + + if (elem) + kfree(elem->private_data); + kfree(elem); + kctl->private_data =3D 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 *m= ixer, return -ENOMEM; } =20 - kctl->private_free =3D freeer; + if (do_private_free) + kctl->private_free =3D elem_private_free; + else + kctl->private_free =3D snd_usb_mixer_elem_free; =20 strlcpy(kctl->id.name, name, sizeof(kctl->id.name)); =20 @@ -1109,7 +1123,6 @@ static struct snd_us16x08_control_params eq_controls[= ] =3D { .type =3D USB_MIXER_BOOLEAN, .num_channels =3D 16, .name =3D "EQ Switch", - .freeer =3D snd_usb_mixer_elem_free }, { /* EQ low gain */ .kcontrol_new =3D &snd_us16x08_eq_gain_ctl, @@ -1117,7 +1130,6 @@ static struct snd_us16x08_control_params eq_controls[= ] =3D { .type =3D USB_MIXER_U8, .num_channels =3D 16, .name =3D "EQ Low Volume", - .freeer =3D snd_usb_mixer_elem_free }, { /* EQ low freq */ .kcontrol_new =3D &snd_us16x08_eq_low_freq_ctl, @@ -1125,7 +1137,6 @@ static struct snd_us16x08_control_params eq_controls[= ] =3D { .type =3D USB_MIXER_U8, .num_channels =3D 16, .name =3D "EQ Low Frequence", - .freeer =3D NULL }, { /* EQ mid low gain */ .kcontrol_new =3D &snd_us16x08_eq_gain_ctl, @@ -1133,7 +1144,6 @@ static struct snd_us16x08_control_params eq_controls[= ] =3D { .type =3D USB_MIXER_U8, .num_channels =3D 16, .name =3D "EQ MidLow Volume", - .freeer =3D snd_usb_mixer_elem_free }, { /* EQ mid low freq */ .kcontrol_new =3D &snd_us16x08_eq_mid_freq_ctl, @@ -1141,7 +1151,6 @@ static struct snd_us16x08_control_params eq_controls[= ] =3D { .type =3D USB_MIXER_U8, .num_channels =3D 16, .name =3D "EQ MidLow Frequence", - .freeer =3D NULL }, { /* EQ mid low Q */ .kcontrol_new =3D &snd_us16x08_eq_mid_width_ctl, @@ -1149,7 +1158,6 @@ static struct snd_us16x08_control_params eq_controls[= ] =3D { .type =3D USB_MIXER_U8, .num_channels =3D 16, .name =3D "EQ MidQLow Q", - .freeer =3D NULL }, { /* EQ mid high gain */ .kcontrol_new =3D &snd_us16x08_eq_gain_ctl, @@ -1157,7 +1165,6 @@ static struct snd_us16x08_control_params eq_controls[= ] =3D { .type =3D USB_MIXER_U8, .num_channels =3D 16, .name =3D "EQ MidHigh Volume", - .freeer =3D snd_usb_mixer_elem_free }, { /* EQ mid high freq */ .kcontrol_new =3D &snd_us16x08_eq_mid_freq_ctl, @@ -1165,7 +1172,6 @@ static struct snd_us16x08_control_params eq_controls[= ] =3D { .type =3D USB_MIXER_U8, .num_channels =3D 16, .name =3D "EQ MidHigh Frequence", - .freeer =3D NULL }, { /* EQ mid high Q */ .kcontrol_new =3D &snd_us16x08_eq_mid_width_ctl, @@ -1173,7 +1179,6 @@ static struct snd_us16x08_control_params eq_controls[= ] =3D { .type =3D USB_MIXER_U8, .num_channels =3D 16, .name =3D "EQ MidHigh Q", - .freeer =3D NULL }, { /* EQ high gain */ .kcontrol_new =3D &snd_us16x08_eq_gain_ctl, @@ -1181,7 +1186,6 @@ static struct snd_us16x08_control_params eq_controls[= ] =3D { .type =3D USB_MIXER_U8, .num_channels =3D 16, .name =3D "EQ High Volume", - .freeer =3D snd_usb_mixer_elem_free }, { /* EQ low freq */ .kcontrol_new =3D &snd_us16x08_eq_high_freq_ctl, @@ -1189,7 +1193,6 @@ static struct snd_us16x08_control_params eq_controls[= ] =3D { .type =3D USB_MIXER_U8, .num_channels =3D 16, .name =3D "EQ High Frequence", - .freeer =3D NULL }, }; =20 @@ -1201,7 +1204,6 @@ static struct snd_us16x08_control_params comp_control= s[] =3D { .type =3D USB_MIXER_BOOLEAN, .num_channels =3D 16, .name =3D "Compressor Switch", - .freeer =3D snd_usb_mixer_elem_free }, { /* Comp threshold */ .kcontrol_new =3D &snd_us16x08_comp_threshold_ctl, @@ -1209,7 +1211,6 @@ static struct snd_us16x08_control_params comp_control= s[] =3D { .type =3D USB_MIXER_U8, .num_channels =3D 16, .name =3D "Compressor Threshold Volume", - .freeer =3D NULL }, { /* Comp ratio */ .kcontrol_new =3D &snd_us16x08_comp_ratio_ctl, @@ -1217,7 +1218,6 @@ static struct snd_us16x08_control_params comp_control= s[] =3D { .type =3D USB_MIXER_U8, .num_channels =3D 16, .name =3D "Compressor Ratio", - .freeer =3D NULL }, { /* Comp attack */ .kcontrol_new =3D &snd_us16x08_comp_attack_ctl, @@ -1225,7 +1225,6 @@ static struct snd_us16x08_control_params comp_control= s[] =3D { .type =3D USB_MIXER_U8, .num_channels =3D 16, .name =3D "Compressor Attack", - .freeer =3D NULL }, { /* Comp release */ .kcontrol_new =3D &snd_us16x08_comp_release_ctl, @@ -1233,7 +1232,6 @@ static struct snd_us16x08_control_params comp_control= s[] =3D { .type =3D USB_MIXER_U8, .num_channels =3D 16, .name =3D "Compressor Release", - .freeer =3D NULL }, { /* Comp gain */ .kcontrol_new =3D &snd_us16x08_comp_gain_ctl, @@ -1241,7 +1239,6 @@ static struct snd_us16x08_control_params comp_control= s[] =3D { .type =3D USB_MIXER_U8, .num_channels =3D 16, .name =3D "Compressor Volume", - .freeer =3D NULL }, }; =20 @@ -1253,7 +1250,6 @@ static struct snd_us16x08_control_params channel_cont= rols[] =3D { .type =3D USB_MIXER_BOOLEAN, .num_channels =3D 16, .name =3D "Phase Switch", - .freeer =3D snd_usb_mixer_elem_free, .default_val =3D 0 }, { /* Fader */ @@ -1262,7 +1258,6 @@ static struct snd_us16x08_control_params channel_cont= rols[] =3D { .type =3D USB_MIXER_U8, .num_channels =3D 16, .name =3D "Line Volume", - .freeer =3D NULL, .default_val =3D 127 }, { /* Mute */ @@ -1271,7 +1266,6 @@ static struct snd_us16x08_control_params channel_cont= rols[] =3D { .type =3D USB_MIXER_BOOLEAN, .num_channels =3D 16, .name =3D "Mute Switch", - .freeer =3D NULL, .default_val =3D 0 }, { /* Pan */ @@ -1280,7 +1274,6 @@ static struct snd_us16x08_control_params channel_cont= rols[] =3D { .type =3D USB_MIXER_U16, .num_channels =3D 16, .name =3D "Pan Left-Right Volume", - .freeer =3D NULL, .default_val =3D 127 }, }; @@ -1293,7 +1286,6 @@ static struct snd_us16x08_control_params master_contr= ols[] =3D { .type =3D USB_MIXER_U8, .num_channels =3D 16, .name =3D "Master Volume", - .freeer =3D NULL, .default_val =3D 127 }, { /* Bypass */ @@ -1302,7 +1294,6 @@ static struct snd_us16x08_control_params master_contr= ols[] =3D { .type =3D USB_MIXER_BOOLEAN, .num_channels =3D 16, .name =3D "DSP Bypass Switch", - .freeer =3D NULL, .default_val =3D 0 }, { /* Buss out */ @@ -1311,7 +1302,6 @@ static struct snd_us16x08_control_params master_contr= ols[] =3D { .type =3D USB_MIXER_BOOLEAN, .num_channels =3D 16, .name =3D "Buss Out Switch", - .freeer =3D NULL, .default_val =3D 0 }, { /* Master mute */ @@ -1320,7 +1310,6 @@ static struct snd_us16x08_control_params master_contr= ols[] =3D { .type =3D USB_MIXER_BOOLEAN, .num_channels =3D 16, .name =3D "Master Mute Switch", - .freeer =3D NULL, .default_val =3D 0 }, =20 @@ -1338,30 +1327,10 @@ int snd_us16x08_controls_create(struct usb_mixer_in= terface *mixer) /* just check for non-MIDI interface */ if (mixer->hostif->desc.bInterfaceNumber =3D=3D 3) { =20 - /* create compressor mixer elements */ - comp_store =3D snd_us16x08_create_comp_store(); - if (comp_store =3D=3D NULL) - return -ENOMEM; - - /* create eq store */ - eq_store =3D snd_us16x08_create_eq_store(); - if (eq_store =3D=3D NULL) { - kfree(comp_store); - return -ENOMEM; - } - - /* create meters store */ - meter_store =3D snd_us16x08_create_meter_store(); - if (meter_store =3D=3D NULL) { - kfree(comp_store); - kfree(eq_store); - return -ENOMEM; - } - /* add routing control */ err =3D 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_int= erface *mixer) elem->cache_val[i] =3D i < 2 ? i : i + 2; elem->cached =3D 0xff; =20 + /* create compressor mixer elements */ + comp_store =3D snd_us16x08_create_comp_store(); + if (!comp_store) + return -ENOMEM; + /* add master controls */ for (i =3D 0; i < sizeof(master_controls) @@ -1385,7 +1359,8 @@ int snd_us16x08_controls_create(struct usb_mixer_inte= rface *mixer) master_controls[i].num_channels, master_controls[i].name, comp_store, - master_controls[i].freeer, &elem); + i =3D=3D 0, /* release comp_store only once */ + &elem); if (err < 0) return err; elem->cache_val[0] =3D master_controls[i].default_val; @@ -1405,7 +1380,7 @@ int snd_us16x08_controls_create(struct usb_mixer_inte= rface *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 =3D 0; j < SND_US16X08_MAX_CHANNELS; j++) { @@ -1415,6 +1390,11 @@ int snd_us16x08_controls_create(struct usb_mixer_int= erface *mixer) elem->cached =3D 0xffff; } =20 + /* create eq store */ + eq_store =3D snd_us16x08_create_eq_store(); + if (!eq_store) + return -ENOMEM; + /* add EQ controls */ for (i =3D 0; i < sizeof(eq_controls) / sizeof(control_params); i++) { @@ -1426,7 +1406,8 @@ int snd_us16x08_controls_create(struct usb_mixer_inte= rface *mixer) eq_controls[i].num_channels, eq_controls[i].name, eq_store, - eq_controls[i].freeer, NULL); + i =3D=3D 0, /* release eq_store only once */ + NULL); if (err < 0) return err; } @@ -1444,18 +1425,23 @@ int snd_us16x08_controls_create(struct usb_mixer_in= terface *mixer) comp_controls[i].num_channels, comp_controls[i].name, comp_store, - comp_controls[i].freeer, NULL); + false, NULL); if (err < 0) return err; } =20 + /* create meters store */ + meter_store =3D 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 =3D comp_store; err =3D 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 64f89b5..a6312fb 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; }; =20 --=20 2.9.3