ALSA: hda/cs_dsp_ctl: Use private_free for control cleanup

Use the control private_free callback to free the associated data
block. This ensures that the memory won't leak, whatever way the
control gets destroyed.

The original implementation didn't actually remove the ALSA
controls in hda_cs_dsp_control_remove(). It only freed the internal
tracking structure. This meant it was possible to remove/unload the
amp driver while leaving its ALSA controls still present in the
soundcard. Obviously attempting to access them could cause segfaults
or at least dereferencing stale pointers.

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
Fixes: 3233b978af23 ("ALSA: hda: hda_cs_dsp_ctl: Add Library to support CS_DSP ALSA controls")
Link: https://lore.kernel.org/r/20240508095627.44476-1-rf@opensource.cirrus.com
Signed-off-by: Takashi Iwai <tiwai@suse.de>

authored by

Richard Fitzgerald and committed by

Takashi Iwai 2 years ago 172811e3 b7df4cc3

+31 -16

1 changed file

expand all

sound

pci

hda

hda_cs_dsp_ctl.c

+31 -16

sound/pci/hda/hda_cs_dsp_ctl.c

··· 8 8 9 9 #include <linux/module.h> 10 10 #include <sound/soc.h> 11 + #include <linux/cleanup.h> 11 12 #include <linux/firmware/cirrus/cs_dsp.h> 12 13 #include <linux/firmware/cirrus/wmfw.h> 13 14 #include "hda_cs_dsp_ctl.h" ··· 98 97 return out; 99 98 } 100 99 101 - static void hda_cs_dsp_add_kcontrol(struct hda_cs_dsp_coeff_ctl *ctl, const char *name) 100 + static void hda_cs_dsp_free_kcontrol(struct snd_kcontrol *kctl) 102 101 { 102 + struct hda_cs_dsp_coeff_ctl *ctl = (struct hda_cs_dsp_coeff_ctl *)snd_kcontrol_chip(kctl); 103 103 struct cs_dsp_coeff_ctl *cs_ctl = ctl->cs_ctl; 104 + 105 + /* NULL priv to prevent a double-free in hda_cs_dsp_control_remove() */ 106 + cs_ctl->priv = NULL; 107 + kfree(ctl); 108 + } 109 + 110 + static void hda_cs_dsp_add_kcontrol(struct cs_dsp_coeff_ctl *cs_ctl, 111 + const struct hda_cs_dsp_ctl_info *info, 112 + const char *name) 113 + { 104 114 struct snd_kcontrol_new kcontrol = {0}; 105 115 struct snd_kcontrol *kctl; 116 + struct hda_cs_dsp_coeff_ctl *ctl __free(kfree) = NULL; 106 117 int ret = 0; 107 118 108 119 if (cs_ctl->len > ADSP_MAX_STD_CTRL_SIZE) { ··· 123 110 return; 124 111 } 125 112 113 + ctl = kzalloc(sizeof(*ctl), GFP_KERNEL); 114 + if (!ctl) 115 + return; 116 + 117 + ctl->cs_ctl = cs_ctl; 118 + ctl->card = info->card; 119 + 126 120 kcontrol.name = name; 127 121 kcontrol.info = hda_cs_dsp_coeff_info; 128 122 kcontrol.iface = SNDRV_CTL_ELEM_IFACE_MIXER; ··· 137 117 kcontrol.get = hda_cs_dsp_coeff_get; 138 118 kcontrol.put = hda_cs_dsp_coeff_put; 139 119 140 - /* Save ctl inside private_data, ctl is owned by cs_dsp, 141 - * and will be freed when cs_dsp removes the control */ 142 120 kctl = snd_ctl_new1(&kcontrol, (void *)ctl); 143 121 if (!kctl) 144 122 return; 145 123 146 - ret = snd_ctl_add(ctl->card, kctl); 124 + kctl->private_free = hda_cs_dsp_free_kcontrol; 125 + ctl->kctl = kctl; 126 + 127 + /* snd_ctl_add() calls our private_free on error, which will kfree(ctl) */ 128 + cs_ctl->priv = no_free_ptr(ctl); 129 + ret = snd_ctl_add(info->card, kctl); 147 130 if (ret) { 148 131 dev_err(cs_ctl->dsp->dev, "Failed to add KControl %s = %d\n", kcontrol.name, ret); 149 132 return; 150 133 } 151 134 152 135 dev_dbg(cs_ctl->dsp->dev, "Added KControl: %s\n", kcontrol.name); 153 - ctl->kctl = kctl; 154 136 } 155 137 156 138 static void hda_cs_dsp_control_add(struct cs_dsp_coeff_ctl *cs_ctl, ··· 160 138 { 161 139 struct cs_dsp *cs_dsp = cs_ctl->dsp; 162 140 char name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN]; 163 - struct hda_cs_dsp_coeff_ctl *ctl; 164 141 const char *region_name; 165 142 int ret; 166 143 ··· 184 163 " %.*s", cs_ctl->subname_len - skip, cs_ctl->subname + skip); 185 164 } 186 165 187 - ctl = kzalloc(sizeof(*ctl), GFP_KERNEL); 188 - if (!ctl) 189 - return; 190 - 191 - ctl->cs_ctl = cs_ctl; 192 - ctl->card = info->card; 193 - cs_ctl->priv = ctl; 194 - 195 - hda_cs_dsp_add_kcontrol(ctl, name); 166 + hda_cs_dsp_add_kcontrol(cs_ctl, info, name); 196 167 } 197 168 198 169 void hda_cs_dsp_add_controls(struct cs_dsp *dsp, const struct hda_cs_dsp_ctl_info *info) ··· 216 203 { 217 204 struct hda_cs_dsp_coeff_ctl *ctl = cs_ctl->priv; 218 205 219 - kfree(ctl); 206 + /* ctl and kctl may already have been removed by ALSA private_free */ 207 + if (ctl && ctl->kctl) 208 + snd_ctl_remove(ctl->card, ctl->kctl); 220 209 } 221 210 EXPORT_SYMBOL_NS_GPL(hda_cs_dsp_control_remove, SND_HDA_CS_DSP_CONTROLS); 222 211

Configure Feed

Configure Feed