Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 22 Jun 2022 15:34:29 +0200
Message-ID: <87wnd8n7oa.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Vitaly Rodionov <vitalyr@opensource.cirrus.com>
Cc:     Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
        Mark Brown <broonie@kernel.org>, patches@opensource.cirrus.com,
        alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
        Stefan Binding <sbinding@opensource.cirrus.com>
Subject: Re: [PATCH v7 01/14] ALSA: hda: hda_cs_dsp_ctl: Add Library to support CS_DSP ALSA controls
In-Reply-To: <20220622074653.179078-2-vitalyr@opensource.cirrus.com>
References: <20220622074653.179078-1-vitalyr@opensource.cirrus.com>
        <20220622074653.179078-2-vitalyr@opensource.cirrus.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Wed, 22 Jun 2022 09:46:40 +0200,
Vitaly Rodionov wrote:
> 
> +static int hda_cs_dsp_add_kcontrol(struct hda_cs_dsp_coeff_ctl *ctl)
> +{
> +	struct cs_dsp_coeff_ctl *cs_ctl = ctl->cs_ctl;
> +	struct snd_kcontrol_new *kcontrol;
> +	struct snd_kcontrol *kctl;
> +	int ret = 0;
> +
> +	if (cs_ctl->len > ADSP_MAX_STD_CTRL_SIZE) {
> +		dev_err(cs_ctl->dsp->dev, "Control %s: length %zu exceeds maximum %d\n", ctl->name,
> +			cs_ctl->len, ADSP_MAX_STD_CTRL_SIZE);
> +		return -EINVAL;
> +	}
> +
> +	kcontrol = kzalloc(sizeof(*kcontrol), GFP_KERNEL);
> +	if (!kcontrol)
> +		return -ENOMEM;
> +
> +	kcontrol->name = ctl->name;
> +	kcontrol->info = hda_cs_dsp_coeff_info;
> +	kcontrol->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
> +	kcontrol->private_value = (unsigned long)ctl;
> +	kcontrol->access = wmfw_convert_flags(cs_ctl->flags);
> +
> +	kcontrol->get = hda_cs_dsp_coeff_get;
> +	kcontrol->put = hda_cs_dsp_coeff_put;
> +
> +	kctl = snd_ctl_new1(kcontrol, NULL);
> +	if (!kctl) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +	ctl->kctl = kctl;
> +
> +	ret = snd_ctl_add(ctl->card, kctl);
> +	if (ret)
> +		dev_err(cs_ctl->dsp->dev, "Failed to add KControl: %s - Ret: %d\n", kcontrol->name,
> +			ret);
> +	else
> +		dev_dbg(cs_ctl->dsp->dev, "Added KControl: %s\n", kcontrol->name);

snd_ctl_add() releases the kctl automatically upon errors, hence
assigning ctl->kctl may lead to a use-after-free.  Therefore ctl->kctl
should be assigned after the success of snd_ctl_add().

> +int hda_cs_dsp_control_add(struct cs_dsp_coeff_ctl *cs_ctl, struct hda_cs_dsp_ctl_info *info)
> +{
> +	struct cs_dsp *cs_dsp = cs_ctl->dsp;
> +	char name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
> +	struct hda_cs_dsp_coeff_ctl *ctl;
> +	const char *region_name;
> +	int ret;
> +
> +	if (cs_ctl->flags & WMFW_CTL_FLAG_SYS)
> +		return 0;
> +
> +	region_name = cs_dsp_mem_region_name(cs_ctl->alg_region.type);
> +	if (!region_name) {
> +		dev_err(cs_dsp->dev, "Unknown region type: %d\n", cs_ctl->alg_region.type);
> +		return -EINVAL;
> +	}
> +
> +	ret = scnprintf(name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN, "%s %s %.12s %x", info->device_name,
> +			cs_dsp->name, hda_cs_dsp_fw_text[info->fw_type], cs_ctl->alg_region.alg);
> +
> +	if (cs_ctl->subname) {
> +		int avail = SNDRV_CTL_ELEM_ID_NAME_MAXLEN - ret - 2;
> +		int skip = 0;
> +
> +		/* Truncate the subname from the start if it is too long */
> +		if (cs_ctl->subname_len > avail)
> +			skip = cs_ctl->subname_len - avail;
> +
> +		snprintf(name + ret, SNDRV_CTL_ELEM_ID_NAME_MAXLEN - ret,
> +			 " %.*s", cs_ctl->subname_len - skip, cs_ctl->subname + skip);
> +	}
> +
> +	ctl = kzalloc(sizeof(*ctl), GFP_KERNEL);
> +	if (!ctl)
> +		return -ENOMEM;
> +	ctl->cs_ctl = cs_ctl;
> +	ctl->card = info->card;
> +
> +	ctl->name = kmemdup(name, strlen(name) + 1, GFP_KERNEL);

This is kstrdup() :)

But, we don't need to keep the name string persistently at all.
It's copied onto kcontrol id field, and the original string is no
longer needed after that point.  So you can pass the name as is to
hda_cs_dsp_add_kcontrol().


> +	if (!ctl->name) {
> +		ret = -ENOMEM;
> +		dev_err(cs_dsp->dev, "Cannot save ctl name\n");
> +		goto err_ctl;
> +	}
> +
> +	cs_ctl->priv = ctl;
> +
> +	return hda_cs_dsp_add_kcontrol(ctl);

Hm, this leaves ctl even if it returns an error, i.e. some leaks?

> +err_ctl:
> +	dev_err(cs_dsp->dev, "Error adding control: %s\n", name);
> +	kfree(ctl);
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(hda_cs_dsp_control_add, SND_HDA_CS_DSP_CONTROLS);
> +
> +void hda_cs_dsp_control_remove(struct cs_dsp_coeff_ctl *cs_ctl)
> +{
> +	struct hda_cs_dsp_coeff_ctl *ctl = cs_ctl->priv;
> +
> +	snd_ctl_remove_id(ctl->card, &ctl->kctl->id);
> +	kfree(ctl->name);
> +	kfree(ctl);
> +}
> +EXPORT_SYMBOL_NS_GPL(hda_cs_dsp_control_remove, SND_HDA_CS_DSP_CONTROLS);

Is hda_cs_dsp_control_remove() *always* called explicitly for all
added controls at the device removal / unbind?  ALSA control core also
releases the remaining controls by itself, and if the objects are
released there, it'll lead to memory leaks for ctl object.

If the snd_kcontrol may be freed by itself without
hda_cs_dsp_control_remove() call, it should have a proper private_free
callback to free the assigned ctl object (also better to reset
ctl->cs_ctl->priv field, too).


Takashi