[PATCH 0/4] ALSA: input validation for control element writes

[PATCH 0/4] ALSA: input validation for control element writes

[Takashi Iwai]

Hi,

this is a patch set to add a new feature to ALSA control interface,
a validation for the write to control elements.  So far we rely fully
on the driver implementation to deal with the user inputs with invalid
values (e.g. a value outside the defined range).  This patch set
allows ALSA core to verify the input values beforehand and returns the
error immediately if the validation fails.

The feature is enabled with a new Kconfig for now, as it brings a
slight performance overhead, although this could be turned on as
default in most cases, IMO.

The patch set contains a few preliminary cleanup patches.  The
essential change is only the last one.


Takashi

===

Takashi Iwai (4):
  ASoC: topology: Drop superfluous check of CONFIG_SND_CTL_VALIDATION
  ALSA: control: Rename CONFIG_SND_CTL_VALIDATION to
    CONFIG_SND_CTL_DEBUG
  ALSA: control: Drop superfluous ifdef CONFIG_SND_CTL_DEBUG
  ALSA: control: Add input validation

 include/sound/control.h  |  2 +-
 sound/core/Kconfig       | 27 ++++++++++---
 sound/core/control.c     | 87 +++++++++++++++++++++++-----------------
 sound/soc/soc-topology.c |  2 +-
 4 files changed, 73 insertions(+), 45 deletions(-)

-- 
2.35.3

[PATCH 1/4] ASoC: topology: Drop superfluous check of CONFIG_SND_CTL_VALIDATION

[Takashi Iwai]

The compiler must be clever enough to optimize out for the no-op when
CONFIG_SND_CTL_VALIDATION is disabled.  Let's drop the superfluous
check.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/soc-topology.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 3f9d314fba16..b101db85446f 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -535,7 +535,7 @@ static int soc_tplg_kcontrol_bind_io(struct snd_soc_tplg_ctl_hdr *hdr,
 		 * return an -EINVAL error and prevent the card from
 		 * being configured.
 		 */
-		if (IS_ENABLED(CONFIG_SND_CTL_VALIDATION) && sbe->max > 512)
+		if (sbe->max > 512)
 			k->access |= SNDRV_CTL_ELEM_ACCESS_SKIP_CHECK;
 
 		ext_ops = tplg->bytes_ext_ops;
-- 
2.35.3

[PATCH 2/4] ALSA: control: Rename CONFIG_SND_CTL_VALIDATION to CONFIG_SND_CTL_DEBUG

[Takashi Iwai]

The purpose of CONFIG_SND_CTL_VALIDATION is rather to enable the
debugging feature for the control API.  The validation is only a part
of it.  Let's rename it to be more explicit and intuitive.

While we're at it, let's advertise, give more comment to recommend
this feature for development in the kconfig help text.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/control.h |  2 +-
 sound/core/Kconfig      | 17 +++++++++++------
 sound/core/control.c    |  4 ++--
 3 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/include/sound/control.h b/include/sound/control.h
index 985c51a8fb74..fcd3cce673ec 100644
--- a/include/sound/control.h
+++ b/include/sound/control.h
@@ -23,7 +23,7 @@ typedef int (snd_kcontrol_tlv_rw_t)(struct snd_kcontrol *kcontrol,
 				    unsigned int __user *tlv);
 
 /* internal flag for skipping validations */
-#ifdef CONFIG_SND_CTL_VALIDATION
+#ifdef CONFIG_SND_CTL_DEBUG
 #define SNDRV_CTL_ELEM_ACCESS_SKIP_CHECK	(1 << 24)
 #define snd_ctl_skip_validation(info) \
 	((info)->access & SNDRV_CTL_ELEM_ACCESS_SKIP_CHECK)
diff --git a/sound/core/Kconfig b/sound/core/Kconfig
index dd7b40734723..5bd8c93931b2 100644
--- a/sound/core/Kconfig
+++ b/sound/core/Kconfig
@@ -178,14 +178,19 @@ config SND_PCM_XRUN_DEBUG
 	  sound clicking when system is loaded, it may help to determine
 	  the process or driver which causes the scheduling gaps.
 
-config SND_CTL_VALIDATION
-	bool "Perform sanity-checks for each control element access"
+config SND_CTL_DEBUG
+	bool "Enable debugging feature for control API"
 	depends on SND_DEBUG
 	help
-	  Say Y to enable the additional validation of each control element
-	  access, including sanity-checks like whether the values returned
-	  from the driver are in the proper ranges or the check of the invalid
-	  access at out-of-array areas.
+	  Say Y to enable the debugging feature for ALSA control API.
+	  It performs the additional sanity-checks for each control element
+	  read access, such as whether the values returned from the driver
+	  are in the proper ranges or the check of the invalid access at
+	  out-of-array areas.  The error is printed when the driver gives
+	  such unexpected values.
+	  When you develop a driver that deals with control elements, it's
+	  strongly recommended to try this one once and verify whether you see
+	  any relevant errors or not.
 
 config SND_JACK_INJECTION_DEBUG
 	bool "Sound jack injection interface via debugfs"
diff --git a/sound/core/control.c b/sound/core/control.c
index a25c0d64d104..21741dc653ed 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -855,7 +855,7 @@ static const unsigned int value_sizes[] = {
 	[SNDRV_CTL_ELEM_TYPE_INTEGER64] = sizeof(long long),
 };
 
-#ifdef CONFIG_SND_CTL_VALIDATION
+#ifdef CONFIG_SND_CTL_DEBUG
 /* fill the remaining snd_ctl_elem_value data with the given pattern */
 static void fill_remaining_elem_value(struct snd_ctl_elem_value *control,
 				      struct snd_ctl_elem_info *info,
@@ -1077,7 +1077,7 @@ static int snd_ctl_elem_read(struct snd_card *card,
 
 	snd_ctl_build_ioff(&control->id, kctl, index_offset);
 
-#ifdef CONFIG_SND_CTL_VALIDATION
+#ifdef CONFIG_SND_CTL_DEBUG
 	/* info is needed only for validation */
 	memset(&info, 0, sizeof(info));
 	info.id = control->id;
-- 
2.35.3

[PATCH 3/4] ALSA: control: Drop superfluous ifdef CONFIG_SND_CTL_DEBUG

[Takashi Iwai]

Compilers should be smart enough to optimize out the dead functions,
so we don't need to define ugly dummy functions with ifdef.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/control.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 21741dc653ed..339d420fb9f7 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -855,7 +855,6 @@ static const unsigned int value_sizes[] = {
 	[SNDRV_CTL_ELEM_TYPE_INTEGER64] = sizeof(long long),
 };
 
-#ifdef CONFIG_SND_CTL_DEBUG
 /* fill the remaining snd_ctl_elem_value data with the given pattern */
 static void fill_remaining_elem_value(struct snd_ctl_elem_value *control,
 				      struct snd_ctl_elem_info *info,
@@ -967,21 +966,6 @@ static int sanity_check_elem_value(struct snd_card *card,
 
 	return ret;
 }
-#else
-static inline void fill_remaining_elem_value(struct snd_ctl_elem_value *control,
-					     struct snd_ctl_elem_info *info,
-					     u32 pattern)
-{
-}
-
-static inline int sanity_check_elem_value(struct snd_card *card,
-					  struct snd_ctl_elem_value *control,
-					  struct snd_ctl_elem_info *info,
-					  u32 pattern)
-{
-	return 0;
-}
-#endif
 
 static int __snd_ctl_elem_info(struct snd_card *card,
 			       struct snd_kcontrol *kctl,
-- 
2.35.3

[PATCH 4/4] ALSA: control: Add input validation

[Takashi Iwai]

This patch adds a new feature to enable the validation of input data
to control elements in the ALSA core side.  When
CONFIG_SND_CTL_INPUT_VALIDATION is set, ALSA core verifies whether the
each input value via control API is in the defined ranges, also checks
whether it's aligned to the defined steps.  If an invalid value is
detected, ALSA core returns -EINVAL error immediately without passing
further to the driver's callback.  So this is a kind of hardening for
(badly written) drivers that have no proper error checks, at the cost
of a slight performance overhead.

Technically seen, this reuses a part of the existing validation code
for CONFIG_SND_CTL_DEBUG case with a slight modification to suppress
error prints for the input validation.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/Kconfig   | 10 +++++++
 sound/core/control.c | 69 +++++++++++++++++++++++++++++++-------------
 2 files changed, 59 insertions(+), 20 deletions(-)

diff --git a/sound/core/Kconfig b/sound/core/Kconfig
index 5bd8c93931b2..11d44dfda7ce 100644
--- a/sound/core/Kconfig
+++ b/sound/core/Kconfig
@@ -178,6 +178,16 @@ config SND_PCM_XRUN_DEBUG
 	  sound clicking when system is loaded, it may help to determine
 	  the process or driver which causes the scheduling gaps.
 
+config SND_CTL_INPUT_VALIDATION
+	bool "Validate input data to control API"
+	help
+	  Say Y to enable the additional validation for the input data to
+	  each control element, including the value range checks.
+	  An error is returned from ALSA core for invalid inputs without
+	  passing to the driver.  This is a kind of hardening for drivers
+	  that have no proper error checks, at the cost of a slight
+	  performance overhead.
+
 config SND_CTL_DEBUG
 	bool "Enable debugging feature for control API"
 	depends on SND_DEBUG
diff --git a/sound/core/control.c b/sound/core/control.c
index 339d420fb9f7..b5bbdabca353 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -871,7 +871,7 @@ static void fill_remaining_elem_value(struct snd_ctl_elem_value *control,
 static int sanity_check_int_value(struct snd_card *card,
 				  const struct snd_ctl_elem_value *control,
 				  const struct snd_ctl_elem_info *info,
-				  int i)
+				  int i, bool print_error)
 {
 	long long lval, lmin, lmax, lstep;
 	u64 rem;
@@ -905,21 +905,23 @@ static int sanity_check_int_value(struct snd_card *card,
 	}
 
 	if (lval < lmin || lval > lmax) {
-		dev_err(card->dev,
-			"control %i:%i:%i:%s:%i: value out of range %lld (%lld/%lld) at count %i\n",
-			control->id.iface, control->id.device,
-			control->id.subdevice, control->id.name,
-			control->id.index, lval, lmin, lmax, i);
+		if (print_error)
+			dev_err(card->dev,
+				"control %i:%i:%i:%s:%i: value out of range %lld (%lld/%lld) at count %i\n",
+				control->id.iface, control->id.device,
+				control->id.subdevice, control->id.name,
+				control->id.index, lval, lmin, lmax, i);
 		return -EINVAL;
 	}
 	if (lstep) {
 		div64_u64_rem(lval, lstep, &rem);
 		if (rem) {
-			dev_err(card->dev,
-				"control %i:%i:%i:%s:%i: unaligned value %lld (step %lld) at count %i\n",
-				control->id.iface, control->id.device,
-				control->id.subdevice, control->id.name,
-				control->id.index, lval, lstep, i);
+			if (print_error)
+				dev_err(card->dev,
+					"control %i:%i:%i:%s:%i: unaligned value %lld (step %lld) at count %i\n",
+					control->id.iface, control->id.device,
+					control->id.subdevice, control->id.name,
+					control->id.index, lval, lstep, i);
 			return -EINVAL;
 		}
 	}
@@ -927,15 +929,13 @@ static int sanity_check_int_value(struct snd_card *card,
 	return 0;
 }
 
-/* perform sanity checks to the given snd_ctl_elem_value object */
-static int sanity_check_elem_value(struct snd_card *card,
-				   const struct snd_ctl_elem_value *control,
-				   const struct snd_ctl_elem_info *info,
-				   u32 pattern)
+/* check whether the all input values are valid for the given elem value */
+static int sanity_check_input_values(struct snd_card *card,
+				     const struct snd_ctl_elem_value *control,
+				     const struct snd_ctl_elem_info *info,
+				     bool print_error)
 {
-	size_t offset;
-	int i, ret = 0;
-	u32 *p;
+	int i, ret;
 
 	switch (info->type) {
 	case SNDRV_CTL_ELEM_TYPE_BOOLEAN:
@@ -943,7 +943,8 @@ static int sanity_check_elem_value(struct snd_card *card,
 	case SNDRV_CTL_ELEM_TYPE_INTEGER64:
 	case SNDRV_CTL_ELEM_TYPE_ENUMERATED:
 		for (i = 0; i < info->count; i++) {
-			ret = sanity_check_int_value(card, control, info, i);
+			ret = sanity_check_int_value(card, control, info, i,
+						     print_error);
 			if (ret < 0)
 				return ret;
 		}
@@ -952,6 +953,23 @@ static int sanity_check_elem_value(struct snd_card *card,
 		break;
 	}
 
+	return 0;
+}
+
+/* perform sanity checks to the given snd_ctl_elem_value object */
+static int sanity_check_elem_value(struct snd_card *card,
+				   const struct snd_ctl_elem_value *control,
+				   const struct snd_ctl_elem_info *info,
+				   u32 pattern)
+{
+	size_t offset;
+	int ret;
+	u32 *p;
+
+	ret = sanity_check_input_values(card, control, info, true);
+	if (ret < 0)
+		return ret;
+
 	/* check whether the remaining area kept untouched */
 	offset = value_sizes[info->type] * info->count;
 	offset = DIV_ROUND_UP(offset, sizeof(u32));
@@ -1138,6 +1156,17 @@ 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) {
+		struct snd_ctl_elem_info info;
+
+		memset(&info, 0, sizeof(info));
+		info.id = control->id;
+		result = __snd_ctl_elem_info(card, kctl, &info, NULL);
+		if (!result)
+			result = sanity_check_input_values(card, control, &info,
+							   false);
+	}
 	if (!result)
 		result = kctl->put(kctl, control);
 	snd_power_unref(card);
-- 
2.35.3