linux-kernel - Re: [PATCH v4 4/7] ASoC: codecs: wcd937x: add basic controls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f766e8fc-64e7-4579-ac5a-4afcdae067cc@sirena.org.uk>
Date: Thu, 16 May 2024 12:58:40 +0100
From: Mark Brown <broonie@...nel.org>
To: Mohammad Rafi Shaik <quic_mohs@...cinc.com>
Cc: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
	Banajit Goswami <bgoswami@...cinc.com>,
	Liam Girdwood <lgirdwood@...il.com>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
	alsa-devel@...a-project.org, linux-arm-msm@...r.kernel.org,
	linux-sound@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, quic_rohkumar@...cinc.com,
	quic_pkumpatl@...cinc.com
Subject: Re: [PATCH v4 4/7] ASoC: codecs: wcd937x: add basic controls

On Thu, May 16, 2024 at 10:17:58AM +0530, Mohammad Rafi Shaik wrote:

> +static int wcd937x_rx_hph_mode_put(struct snd_kcontrol *kcontrol,
> +				   struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component =
> +				snd_soc_kcontrol_component(kcontrol);
> +	struct wcd937x_priv *wcd937x = snd_soc_component_get_drvdata(component);
> +	u32 mode_val;
> +
> +	mode_val = ucontrol->value.enumerated.item[0];
> +	if (!mode_val) {
> +		dev_warn(component->dev, "Invalid HPH Mode, default to class_AB\n");
> +		mode_val = CLS_AB;

This should be silent (or return an error) otherwise people can DoS the
logs by just spamming in invalid values.

> +	}
> +
> +	wcd937x->hph_mode = mode_val;

I would expect there's more validation needed here, this will blindly
assign any non-zero mode.  Please run the mixer-test selftests on a card
with this device in it and show the results on future submissions, this
will detect this and other issues for you.

Several of the other controls look like they're also missing validation.

> +static int wcd937x_set_swr_port(struct snd_kcontrol *kcontrol,
> +				struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct soc_mixer_control *mixer = (struct soc_mixer_control *)kcontrol->private_value;
> +	struct snd_soc_component *comp = snd_soc_kcontrol_component(kcontrol);
> +	struct wcd937x_priv *wcd937x = snd_soc_component_get_drvdata(comp);
> +	struct wcd937x_sdw_priv *wcd;
> +	int dai_id = mixer->shift;
> +	int ch_idx = mixer->reg;
> +	int portidx;
> +	bool enable;
> +
> +	wcd = wcd937x->sdw_priv[dai_id];
> +
> +	portidx = wcd->ch_info[ch_idx].port_num;
> +
> +	enable = !!ucontrol->value.integer.value[0];
> +
> +	wcd->port_enable[portidx] = enable;
> +	wcd937x_connect_port(wcd, portidx, ch_idx, enable);
> +
> +	return 1;
> +}

This unconditionally reports that the value changed so will generate
spurious events.
> +
> +static const char * const rx_hph_mode_mux_text[] = {
> +	"CLS_H_INVALID", "CLS_H_HIFI", "CLS_H_LP", "CLS_AB", "CLS_H_LOHIFI",
> +	"CLS_H_ULP", "CLS_AB_HIFI",
> +};

It would be more idiomatic to write these in a more human readable form.

> +static const char * const wcd937x_ear_pa_gain_text[] = {
> +	"G_6_DB", "G_4P5_DB", "G_3_DB", "G_1P5_DB", "G_0_DB",
> +	"G_M1P5_DB", "G_M3_DB", "G_M4P5_DB",
> +	"G_M6_DB", "G_7P5_DB", "G_M9_DB",
> +	"G_M10P5_DB", "G_M12_DB", "G_M13P5_DB",
> +	"G_M15_DB", "G_M16P5_DB", "G_M18_DB",
> +};

Why is this an enum and not TLV information?

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)