[PATCH 0/5] ALSA: some driver fixes for control input validations

[PATCH 0/5] ALSA: some driver fixes for control input validations

[Takashi Iwai]

Hi,

this is a patch set as a follow up of the thread about the errors
reported by kselftest mixer-test.  It changes HD-audio and vmaster
control behavior to return -EINVAL for invalid input values.

There is a change in kselftest itself to skip the write tests for
volatile controls, too.  It's for the channel map controls that can't
hold the stable values.


Takashi

===

Takashi Iwai (5):
  ALSA: vmaster: Return error for invalid input values
  ALSA: hda: Return -EINVAL for invalid volume/switch inputs
  ALSA: control: Apply sanity check of input values for user elements
  kselftest/alsa: mixer-test: Skip write tests for volatile controls
  ALSA: chmap: Mark Channel Map controls as volatile

 sound/core/control.c                      |  3 ++-
 sound/core/pcm_lib.c                      |  1 +
 sound/core/vmaster.c                      |  8 ++++++++
 sound/pci/hda/hda_codec.c                 | 23 ++++++++++++++++++-----
 tools/testing/selftests/alsa/mixer-test.c | 21 +++++++++++++++++++++
 5 files changed, 50 insertions(+), 6 deletions(-)

-- 
2.43.0

[PATCH 1/5] ALSA: vmaster: Return error for invalid input values

[Takashi Iwai]

So far the vmaster code has been tolerant about the input values and
accepts any values by correcting internally.  But now our own selftest
starts complaining about this behavior, so let's be picky and change
the behavior to return -EINVAL for invalid input values instead.

Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Closes: https://lore.kernel.org/r/1d44be36-9bb9-4d82-8953-5ae2a4f09405@molgen.mpg.de
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/vmaster.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/sound/core/vmaster.c b/sound/core/vmaster.c
index 04a57f7be6ea..c657659b236c 100644
--- a/sound/core/vmaster.c
+++ b/sound/core/vmaster.c
@@ -198,6 +198,12 @@ static int follower_put(struct snd_kcontrol *kcontrol,
 	err = follower_init(follower);
 	if (err < 0)
 		return err;
+	for (ch = 0; ch < follower->info.count; ch++) {
+		if (ucontrol->value.integer.value[ch] < follower->info.min_val ||
+		    ucontrol->value.integer.value[ch] > follower->info.max_val)
+			return -EINVAL;
+	}
+
 	for (ch = 0; ch < follower->info.count; ch++) {
 		if (follower->vals[ch] != ucontrol->value.integer.value[ch]) {
 			changed = 1;
@@ -365,6 +371,8 @@ static int master_put(struct snd_kcontrol *kcontrol,
 	new_val = ucontrol->value.integer.value[0];
 	if (new_val == old_val)
 		return 0;
+	if (new_val < master->info.min_val || new_val > master->info.max_val)
+		return -EINVAL;
 
 	err = sync_followers(master, old_val, new_val);
 	if (err < 0)
-- 
2.43.0

[PATCH 2/5] ALSA: hda: Return -EINVAL for invalid volume/switch inputs

[Takashi Iwai]

So far the HD-audio driver has been tolerant about the input values
and accepts any values by correcting the amp volume and switch values
internally.  But now our own selftest starts complaining about this
behavior, so let's be picky and change the behavior to return -EINVAL
for invalid input values instead.

Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Closes: https://lore.kernel.org/r/1d44be36-9bb9-4d82-8953-5ae2a4f09405@molgen.mpg.de
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_codec.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 325e8f0b99a8..3dd1bda0c5c6 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -1496,7 +1496,7 @@ update_amp_value(struct hda_codec *codec, hda_nid_t nid,
 	/* ofs = 0: raw max value */
 	maxval = get_amp_max_value(codec, nid, dir, 0);
 	if (val > maxval)
-		val = maxval;
+		return -EINVAL;
 	return snd_hda_codec_amp_update(codec, nid, ch, dir, idx,
 					HDA_AMP_VOLMASK, val);
 }
@@ -1547,13 +1547,21 @@ int snd_hda_mixer_amp_volume_put(struct snd_kcontrol *kcontrol,
 	unsigned int ofs = get_amp_offset(kcontrol);
 	long *valp = ucontrol->value.integer.value;
 	int change = 0;
+	int err;
 
 	if (chs & 1) {
-		change = update_amp_value(codec, nid, 0, dir, idx, ofs, *valp);
+		err = update_amp_value(codec, nid, 0, dir, idx, ofs, *valp);
+		if (err < 0)
+			return err;
+		change |= err;
 		valp++;
 	}
-	if (chs & 2)
-		change |= update_amp_value(codec, nid, 1, dir, idx, ofs, *valp);
+	if (chs & 2) {
+		err = update_amp_value(codec, nid, 1, dir, idx, ofs, *valp);
+		if (err < 0)
+			return err;
+		change |= err;
+	}
 	return change;
 }
 EXPORT_SYMBOL_GPL(snd_hda_mixer_amp_volume_put);
@@ -2149,15 +2157,20 @@ int snd_hda_mixer_amp_switch_put(struct snd_kcontrol *kcontrol,
 	int change = 0;
 
 	if (chs & 1) {
+		if (*valp < 0 || *valp > 1)
+			return -EINVAL;
 		change = snd_hda_codec_amp_update(codec, nid, 0, dir, idx,
 						  HDA_AMP_MUTE,
 						  *valp ? 0 : HDA_AMP_MUTE);
 		valp++;
 	}
-	if (chs & 2)
+	if (chs & 2) {
+		if (*valp < 0 || *valp > 1)
+			return -EINVAL;
 		change |= snd_hda_codec_amp_update(codec, nid, 1, dir, idx,
 						   HDA_AMP_MUTE,
 						   *valp ? 0 : HDA_AMP_MUTE);
+	}
 	hda_call_check_power_status(codec, nid);
 	return change;
 }
-- 
2.43.0

[PATCH 3/5] ALSA: control: Apply sanity check of input values for user elements

[Takashi Iwai]

Although we have already a mechanism for sanity checks of input values
for control writes, it's not applied unless the kconfig
CONFIG_SND_CTL_INPUT_VALIDATION is set due to the performance reason.
Nevertheless, it still makes sense to apply the check for user
elements despite of its cost, as that's the only way to filter out the
invalid values; the user controls are handled solely in ALSA core
code, and there is no corresponding driver, after all.

This patch enables the input value validation for user control
elements no matter whether CONFIG_SND_CTL_INPUT_VALIDATION is set or
not.  The kselftest will be happier with this change, as the incorrect
values will be bailed out now with errors.

For other normal controls, the check is applied still only when
CONFIG_SND_CTL_INPUT_VALIDATION is set.

Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Closes: https://lore.kernel.org/r/1d44be36-9bb9-4d82-8953-5ae2a4f09405@molgen.mpg.de
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/control.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index fb0c60044f7b..50890983d7e2 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1317,7 +1317,8 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file,
 	snd_ctl_build_ioff(&control->id, kctl, index_offset);
 	result = snd_power_ref_and_wait(card);
 	/* validate input values */
-	if (IS_ENABLED(CONFIG_SND_CTL_INPUT_VALIDATION) && !result) {
+	if ((IS_ENABLED(CONFIG_SND_CTL_INPUT_VALIDATION) ||
+	     (vd->access & SNDRV_CTL_ELEM_ACCESS_USER)) && !result) {
 		struct snd_ctl_elem_info info;
 
 		memset(&info, 0, sizeof(info));
-- 
2.43.0

[PATCH 4/5] kselftest/alsa: mixer-test: Skip write tests for volatile controls

[Takashi Iwai]

The control elements with volatile flag don't guarantee that the
written values are actually saved for the next reads, hence they
aren't suitable for the standard mixer tests.  Skip the write tests
for those volatile controls for avoiding confusion.

Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Closes: https://lore.kernel.org/r/1d44be36-9bb9-4d82-8953-5ae2a4f09405@molgen.mpg.de
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 tools/testing/selftests/alsa/mixer-test.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c
index 1c04e5f638a0..3c9a45fb5a29 100644
--- a/tools/testing/selftests/alsa/mixer-test.c
+++ b/tools/testing/selftests/alsa/mixer-test.c
@@ -702,6 +702,13 @@ static void test_ctl_write_default(struct ctl_data *ctl)
 		return;
 	}
 
+	if (snd_ctl_elem_info_is_volatile(ctl->info)) {
+		ksft_print_msg("%s is volatile\n", ctl->name);
+		ksft_test_result_skip("write_default.%d.%d\n",
+				      ctl->card->card, ctl->elem);
+		return;
+	}
+
 	err = write_and_verify(ctl, ctl->def_val, NULL);
 
 	ksft_test_result(err >= 0, "write_default.%d.%d\n",
@@ -827,6 +834,13 @@ static void test_ctl_write_valid(struct ctl_data *ctl)
 		return;
 	}
 
+	if (snd_ctl_elem_info_is_volatile(ctl->info)) {
+		ksft_print_msg("%s is volatile\n", ctl->name);
+		ksft_test_result_skip("write_valid.%d.%d\n",
+				      ctl->card->card, ctl->elem);
+		return;
+	}
+
 	switch (snd_ctl_elem_info_get_type(ctl->info)) {
 	case SND_CTL_ELEM_TYPE_BOOLEAN:
 		pass = test_ctl_write_valid_boolean(ctl);
@@ -1039,6 +1053,13 @@ static void test_ctl_write_invalid(struct ctl_data *ctl)
 		return;
 	}
 
+	if (!snd_ctl_elem_info_is_volatile(ctl->info)) {
+		ksft_print_msg("%s is volatile\n", ctl->name);
+		ksft_test_result_skip("write_invalid.%d.%d\n",
+				      ctl->card->card, ctl->elem);
+		return;
+	}
+
 	switch (snd_ctl_elem_info_get_type(ctl->info)) {
 	case SND_CTL_ELEM_TYPE_BOOLEAN:
 		pass = test_ctl_write_invalid_boolean(ctl);
-- 
2.43.0

[PATCH 5/5] ALSA: chmap: Mark Channel Map controls as volatile

[Takashi Iwai]

The values returned from Playback Channel Map and Capture Channel Map
controls may vary dynamically depending on the corresponding PCM
stream.  Mark those as volatile to indicate the values are unstable
and not suitable for testing.

Note that we may change the driver to return -EINVAL, but this would
bring other side effects, such as "alsactl restore" would start
receiving unexpected errors.  So we still keep returning 0 for those
invalid inputs.

Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Closes: https://lore.kernel.org/r/1d44be36-9bb9-4d82-8953-5ae2a4f09405@molgen.mpg.de
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/pcm_lib.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 6f73b3c2c205..071c67cbc479 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -2556,6 +2556,7 @@ int snd_pcm_add_chmap_ctls(struct snd_pcm *pcm, int stream,
 	struct snd_kcontrol_new knew = {
 		.iface = SNDRV_CTL_ELEM_IFACE_PCM,
 		.access = SNDRV_CTL_ELEM_ACCESS_READ |
+			SNDRV_CTL_ELEM_ACCESS_VOLATILE |
 			SNDRV_CTL_ELEM_ACCESS_TLV_READ |
 			SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK,
 		.info = pcm_chmap_ctl_info,
-- 
2.43.0

Re: [PATCH 3/5] ALSA: control: Apply sanity check of input values for user elements

[Jaroslav Kysela]

On 14. 06. 24 14:47, Takashi Iwai wrote:
> Although we have already a mechanism for sanity checks of input values
> for control writes, it's not applied unless the kconfig
> CONFIG_SND_CTL_INPUT_VALIDATION is set due to the performance reason.
> Nevertheless, it still makes sense to apply the check for user
> elements despite of its cost, as that's the only way to filter out the
> invalid values; the user controls are handled solely in ALSA core
> code, and there is no corresponding driver, after all.
> 
> This patch enables the input value validation for user control
> elements no matter whether CONFIG_SND_CTL_INPUT_VALIDATION is set or
> not.  The kselftest will be happier with this change, as the incorrect
> values will be bailed out now with errors.
> 
> For other normal controls, the check is applied still only when
> CONFIG_SND_CTL_INPUT_VALIDATION is set.
> 
> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Closes: https://lore.kernel.org/r/1d44be36-9bb9-4d82-8953-5ae2a4f09405@molgen.mpg.de
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   sound/core/control.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index fb0c60044f7b..50890983d7e2 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -1317,7 +1317,8 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file,
>   	snd_ctl_build_ioff(&control->id, kctl, index_offset);
>   	result = snd_power_ref_and_wait(card);
>   	/* validate input values */
> -	if (IS_ENABLED(CONFIG_SND_CTL_INPUT_VALIDATION) && !result) {
> +	if ((IS_ENABLED(CONFIG_SND_CTL_INPUT_VALIDATION) ||
> +	     (vd->access & SNDRV_CTL_ELEM_ACCESS_USER)) && !result) {
>   		struct snd_ctl_elem_info info;

The info structure for user elements may be used directly without this 
intermediate variable for the validation. But it may be a possible future 
improvement.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH 0/5] ALSA: some driver fixes for control input validations

[Jaroslav Kysela]

On 14. 06. 24 14:47, Takashi Iwai wrote:
> Hi,
> 
> this is a patch set as a follow up of the thread about the errors
> reported by kselftest mixer-test.  It changes HD-audio and vmaster
> control behavior to return -EINVAL for invalid input values.
> 
> There is a change in kselftest itself to skip the write tests for
> volatile controls, too.  It's for the channel map controls that can't
> hold the stable values.

Thanks.

Reviewed-by: Jaroslav Kysela <perex@perex.cz>

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH 3/5] ALSA: control: Apply sanity check of input values for user elements

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 846 bytes --]

On Fri, Jun 14, 2024 at 02:47:25PM +0200, Takashi Iwai wrote:
> Although we have already a mechanism for sanity checks of input values
> for control writes, it's not applied unless the kconfig
> CONFIG_SND_CTL_INPUT_VALIDATION is set due to the performance reason.
> Nevertheless, it still makes sense to apply the check for user
> elements despite of its cost, as that's the only way to filter out the
> invalid values; the user controls are handled solely in ALSA core
> code, and there is no corresponding driver, after all.
> 
> This patch enables the input value validation for user control
> elements no matter whether CONFIG_SND_CTL_INPUT_VALIDATION is set or
> not.  The kselftest will be happier with this change, as the incorrect
> values will be bailed out now with errors.

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/5] kselftest/alsa: mixer-test: Skip write tests for volatile controls

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 812 bytes --]

On Fri, Jun 14, 2024 at 02:47:26PM +0200, Takashi Iwai wrote:

> The control elements with volatile flag don't guarantee that the
> written values are actually saved for the next reads, hence they
> aren't suitable for the standard mixer tests.  Skip the write tests
> for those volatile controls for avoiding confusion.

We should still verify that you can actually write I think...

> +	if (!snd_ctl_elem_info_is_volatile(ctl->info)) {
> +		ksft_print_msg("%s is volatile\n", ctl->name);
> +		ksft_test_result_skip("write_invalid.%d.%d\n",
> +				      ctl->card->card, ctl->elem);
> +		return;
> +	}

...and that you don't read back invalid values after a write like this
for example.  I think any change for this should be in the validation of
the read but we should still try the writes we think we can do.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 4/5] kselftest/alsa: mixer-test: Skip write tests for volatile controls

[Takashi Iwai]

On Fri, 14 Jun 2024 16:29:03 +0200,
Mark Brown wrote:
> 
> On Fri, Jun 14, 2024 at 02:47:26PM +0200, Takashi Iwai wrote:
> 
> > The control elements with volatile flag don't guarantee that the
> > written values are actually saved for the next reads, hence they
> > aren't suitable for the standard mixer tests.  Skip the write tests
> > for those volatile controls for avoiding confusion.
> 
> We should still verify that you can actually write I think...
> 
> > +	if (!snd_ctl_elem_info_is_volatile(ctl->info)) {
> > +		ksft_print_msg("%s is volatile\n", ctl->name);
> > +		ksft_test_result_skip("write_invalid.%d.%d\n",
> > +				      ctl->card->card, ctl->elem);
> > +		return;
> > +	}
> 
> ...and that you don't read back invalid values after a write like this
> for example.  I think any change for this should be in the validation of
> the read but we should still try the writes we think we can do.

OK, makes sense.  Will respin this one.


thanks,

Takashi