[PATCH v2 0/2] ALSA: hda - Fix incorrect TLV callback check

[PATCH v2 0/2] ALSA: hda - Fix incorrect TLV callback check

[Takashi Iwai]

Hi,

this is a revised patchset for addressing the incorrect TLV callback
check introduced by a set_fs() removal commit.  A fix for a
superfluous '-' in the code is split as another fix so that it can be
applied to older kernels without the rest.  Other than that, only the
changelog text updates since the older patch (that was embedded in the
mail thread "4.13 regression: get_kctl_0dB_offset doesn't handle all
possible callbacks").


thanks,

Takashi

===

Takashi Iwai (2):
  ALSA: hda: Remove superfluous '-' added by printk conversion
  ALSA: hda - Fix incorrect TLV callback check introduced during
    set_fs() removal

 include/sound/control.h   |  3 ++
 sound/core/vmaster.c      | 31 +++++++++++++++
 sound/pci/hda/hda_codec.c | 97 +++++++++++++++++++++++++++--------------------
 3 files changed, 89 insertions(+), 42 deletions(-)

-- 
2.14.2

[PATCH v2 1/2] ALSA: hda: Remove superfluous '-' added by printk conversion

[Takashi Iwai]

While converting the error messages to the standard macros in the
commit 4e76a8833fac ("ALSA: hda - Replace with standard printk"), a
superfluous '-' slipped in the code mistakenly.  Its influence is
almost negligible, merely shows a dB value as negative integer instead
of positive integer (or vice versa) in the rare error message.
So let's kill this embarrassing byte to show more correct value.

Fixes: 4e76a8833fac ("ALSA: hda - Replace with standard printk")
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_codec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 3db26c451837..b6cf9684c2ec 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -1824,7 +1824,7 @@ static int get_kctl_0dB_offset(struct hda_codec *codec,
 			return -1;
 		if (*step_to_check && *step_to_check != step) {
 			codec_err(codec, "Mismatching dB step for vmaster slave (%d!=%d)\n",
--				   *step_to_check, step);
+				   *step_to_check, step);
 			return -1;
 		}
 		*step_to_check = step;
-- 
2.14.2

[PATCH v2 2/2] ALSA: hda - Fix incorrect TLV callback check introduced during set_fs() removal

[Takashi Iwai]

The commit 99b5c5bb9a54 ("ALSA: hda - Remove the use of set_fs()")
converted the get_kctl_0dB_offset() call for killing set_fs() usage in
HD-audio codec code.  The conversion assumed that the TLV callback
used in HD-audio code is only snd_hda_mixer_amp() and applies the TLV
calculation locally.

Although this assumption is correct, and all slave kctls are actually
with that callback, the current code is still utterly buggy; it
doesn't hit this condition and falls back to the next check.  It's
because the function gets called after adding slave kctls to vmaster.
By assigning a slave kctl, the slave kctl object is faked inside
vmaster code, and the whole kctl ops are overridden.  Thus the
callback op points to a different value from what we've assumed.

More badly, as reported by the KERNEXEC and UDEREF features of PaX,
the code flow turns into the unexpected pitfall.  The next fallback
check is SNDRV_CTL_ELEM_ACCESS_TLV_READ access bit, and this always
hits for each kctl with TLV.  Then it evaluates the callback function
pointer wrongly as if it were a TLV array.  Although currently its
side-effect is fairly limited, this incorrect reference may lead to an
unpleasant result.

For addressing the regression, this patch introduces a new helper to
vmaster code, snd_ctl_apply_vmaster_slaves().  This works similarly
like the existing map_slaves() in hda_codec.c: it loops over the slave
list of the given master, and applies the given function to each
slave.  Then the initializer function receives the right kctl object
and we can compare the correct pointer instead of the faked one.

Also, for catching the similar breakage in future, give an error
message when the unexpected TLV callback is found and bail out
immediately.

Fixes: 99b5c5bb9a54 ("ALSA: hda - Remove the use of set_fs()")
Reported-by: PaX Team <pageexec@freemail.hu>
Cc: <stable@vger.kernel.org> # v4.13
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/control.h   |  3 ++
 sound/core/vmaster.c      | 31 +++++++++++++++
 sound/pci/hda/hda_codec.c | 97 +++++++++++++++++++++++++++--------------------
 3 files changed, 89 insertions(+), 42 deletions(-)

diff --git a/include/sound/control.h b/include/sound/control.h
index bd7246de58e7..a1f1152bc687 100644
--- a/include/sound/control.h
+++ b/include/sound/control.h
@@ -248,6 +248,9 @@ int snd_ctl_add_vmaster_hook(struct snd_kcontrol *kctl,
 			     void *private_data);
 void snd_ctl_sync_vmaster(struct snd_kcontrol *kctl, bool hook_only);
 #define snd_ctl_sync_vmaster_hook(kctl)	snd_ctl_sync_vmaster(kctl, true)
+int snd_ctl_apply_vmaster_slaves(struct snd_kcontrol *kctl,
+				 int (*func)(struct snd_kcontrol *, void *),
+				 void *arg);
 
 /*
  * Helper functions for jack-detection controls
diff --git a/sound/core/vmaster.c b/sound/core/vmaster.c
index 6c58e6f73a01..e43af18d4383 100644
--- a/sound/core/vmaster.c
+++ b/sound/core/vmaster.c
@@ -484,3 +484,34 @@ void snd_ctl_sync_vmaster(struct snd_kcontrol *kcontrol, bool hook_only)
 		master->hook(master->hook_private_data, master->val);
 }
 EXPORT_SYMBOL_GPL(snd_ctl_sync_vmaster);
+
+/**
+ * snd_ctl_apply_vmaster_slaves - Apply function to each vmaster slave
+ * @kctl: vmaster kctl element
+ * @func: function to apply
+ * @arg: optional function argument
+ *
+ * Apply the function @func to each slave kctl of the given vmaster kctl.
+ * Returns 0 if successful, or a negative error code.
+ */
+int snd_ctl_apply_vmaster_slaves(struct snd_kcontrol *kctl,
+				 int (*func)(struct snd_kcontrol *, void *),
+				 void *arg)
+{
+	struct link_master *master;
+	struct link_slave *slave;
+	int err;
+
+	master = snd_kcontrol_chip(kctl);
+	err = master_init(master);
+	if (err < 0)
+		return err;
+	list_for_each_entry(slave, &master->slaves, list) {
+		err = func(&slave->slave, arg);
+		if (err < 0)
+			return err;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_ctl_apply_vmaster_slaves);
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index b6cf9684c2ec..a0989d231fd0 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -1803,36 +1803,6 @@ static int check_slave_present(struct hda_codec *codec,
 	return 1;
 }
 
-/* guess the value corresponding to 0dB */
-static int get_kctl_0dB_offset(struct hda_codec *codec,
-			       struct snd_kcontrol *kctl, int *step_to_check)
-{
-	int _tlv[4];
-	const int *tlv = NULL;
-	int val = -1;
-
-	if ((kctl->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) &&
-	    kctl->tlv.c == snd_hda_mixer_amp_tlv) {
-		get_ctl_amp_tlv(kctl, _tlv);
-		tlv = _tlv;
-	} else if (kctl->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_READ)
-		tlv = kctl->tlv.p;
-	if (tlv && tlv[0] == SNDRV_CTL_TLVT_DB_SCALE) {
-		int step = tlv[3];
-		step &= ~TLV_DB_SCALE_MUTE;
-		if (!step)
-			return -1;
-		if (*step_to_check && *step_to_check != step) {
-			codec_err(codec, "Mismatching dB step for vmaster slave (%d!=%d)\n",
-				   *step_to_check, step);
-			return -1;
-		}
-		*step_to_check = step;
-		val = -tlv[2] / step;
-	}
-	return val;
-}
-
 /* call kctl->put with the given value(s) */
 static int put_kctl_with_value(struct snd_kcontrol *kctl, int val)
 {
@@ -1847,19 +1817,58 @@ static int put_kctl_with_value(struct snd_kcontrol *kctl, int val)
 	return 0;
 }
 
-/* initialize the slave volume with 0dB */
-static int init_slave_0dB(struct hda_codec *codec,
-			  void *data, struct snd_kcontrol *slave)
+struct slave_init_arg {
+	struct hda_codec *codec;
+	int step;
+};
+
+/* initialize the slave volume with 0dB via snd_ctl_apply_vmaster_slaves() */
+static int init_slave_0dB(struct snd_kcontrol *kctl, void *_arg)
 {
-	int offset = get_kctl_0dB_offset(codec, slave, data);
-	if (offset > 0)
-		put_kctl_with_value(slave, offset);
+	struct slave_init_arg *arg = _arg;
+	int _tlv[4];
+	const int *tlv = NULL;
+	int step;
+	int val;
+
+	if (kctl->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) {
+		if (kctl->tlv.c != snd_hda_mixer_amp_tlv) {
+			codec_err(arg->codec,
+				  "Unexpected TLV callback for slave %s:%d\n",
+				  kctl->id.name, kctl->id.index);
+			return 0; /* ignore */
+		}
+		get_ctl_amp_tlv(kctl, _tlv);
+		tlv = _tlv;
+	} else if (kctl->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_READ)
+		tlv = kctl->tlv.p;
+
+	if (!tlv || tlv[0] != SNDRV_CTL_TLVT_DB_SCALE)
+		return 0;
+
+	step = tlv[3];
+	step &= ~TLV_DB_SCALE_MUTE;
+	if (!step)
+		return 0;
+	if (arg->step && arg->step != step) {
+		codec_err(arg->codec,
+			  "Mismatching dB step for vmaster slave (%d!=%d)\n",
+			  arg->step, step);
+		return 0;
+	}
+
+	arg->step = step;
+	val = -tlv[2] / step;
+	if (val > 0) {
+		put_kctl_with_value(kctl, val);
+		return val;
+	}
+
 	return 0;
 }
 
-/* unmute the slave */
-static int init_slave_unmute(struct hda_codec *codec,
-			     void *data, struct snd_kcontrol *slave)
+/* unmute the slave via snd_ctl_apply_vmaster_slaves() */
+static int init_slave_unmute(struct snd_kcontrol *slave, void *_arg)
 {
 	return put_kctl_with_value(slave, 1);
 }
@@ -1919,9 +1928,13 @@ int __snd_hda_add_vmaster(struct hda_codec *codec, char *name,
 	/* init with master mute & zero volume */
 	put_kctl_with_value(kctl, 0);
 	if (init_slave_vol) {
-		int step = 0;
-		map_slaves(codec, slaves, suffix,
-			   tlv ? init_slave_0dB : init_slave_unmute, &step);
+		struct slave_init_arg arg = {
+			.codec = codec,
+			.step = 0,
+		};
+		snd_ctl_apply_vmaster_slaves(kctl,
+					     tlv ? init_slave_0dB : init_slave_unmute,
+					     &arg);
 	}
 
 	if (ctl_ret)
-- 
2.14.2

Re: [PATCH v2 2/2] ALSA: hda - Fix incorrect TLV callback check introduced during set_fs() removal

[PaX Team]

On 17 Oct 2017 at 12:34, Takashi Iwai wrote:

> Fixes: 99b5c5bb9a54 ("ALSA: hda - Remove the use of set_fs()")
> Reported-by: PaX Team <pageexec@freemail.hu>

thanks, you can also add Tested-by: PaX Team <pageexec@freemail.hu>.