[PATCH] ASoC: ops: Shift tested values in snd_soc_put_volsw() by +min

[PATCH] ASoC: ops: Shift tested values in snd_soc_put_volsw() by +min

[Marek Vasut]

While the $val/$val2 values passed in from userspace are always >= 0
integers, the limits of the control can be signed integers and the $min
can be non-zero and less than zero. To correctly validate $val/$val2
against platform_max, add the $min offset to val first.

Fixes: 817f7c9335ec0 ("ASoC: ops: Reject out of bounds values in snd_soc_put_volsw()")
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Mark Brown <broonie@kernel.org>
Cc: stable@vger.kernel.org
---
 sound/soc/soc-ops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index f24f7354f46fe..6389a512c4dc6 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -317,7 +317,7 @@ int snd_soc_put_volsw(struct snd_kcontrol *kcontrol,
 		mask = BIT(sign_bit + 1) - 1;
 
 	val = ucontrol->value.integer.value[0];
-	if (mc->platform_max && val > mc->platform_max)
+	if (mc->platform_max && ((int)val + min) > mc->platform_max)
 		return -EINVAL;
 	if (val > max - min)
 		return -EINVAL;
@@ -330,7 +330,7 @@ int snd_soc_put_volsw(struct snd_kcontrol *kcontrol,
 	val = val << shift;
 	if (snd_soc_volsw_is_stereo(mc)) {
 		val2 = ucontrol->value.integer.value[1];
-		if (mc->platform_max && val2 > mc->platform_max)
+		if (mc->platform_max && ((int)val2 + min) > mc->platform_max)
 			return -EINVAL;
 		if (val2 > max - min)
 			return -EINVAL;
-- 
2.34.1

Re: [PATCH] ASoC: ops: Shift tested values in snd_soc_put_volsw() by +min

[Mark Brown]

On Tue, 15 Feb 2022 14:06:45 +0100, Marek Vasut wrote:
> While the $val/$val2 values passed in from userspace are always >= 0
> integers, the limits of the control can be signed integers and the $min
> can be non-zero and less than zero. To correctly validate $val/$val2
> against platform_max, add the $min offset to val first.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-linus

Thanks!

[1/1] ASoC: ops: Shift tested values in snd_soc_put_volsw() by +min
      commit: 9bdd10d57a8807dba0003af0325191f3cec0f11c

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH] ASoC: ops: Shift tested values in snd_soc_put_volsw() by +min

[Takashi Iwai]

On Tue, 15 Feb 2022 14:06:45 +0100,
Marek Vasut wrote:
> 
> While the $val/$val2 values passed in from userspace are always >= 0
> integers, the limits of the control can be signed integers and the $min
> can be non-zero and less than zero. To correctly validate $val/$val2
> against platform_max, add the $min offset to val first.
> 
> Fixes: 817f7c9335ec0 ("ASoC: ops: Reject out of bounds values in snd_soc_put_volsw()")
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: stable@vger.kernel.org

Now I'm looking at this since I pulled Mark's PR, and noticed that
snd_soc_put_volsw_sx() may have a similar problem.  Care to cover
that, too?


But, more reading the code, I suspect whether the function does work
correctly at all...  How is the mask calculation done in that way?
  unsigned int mask = (1U << (fls(min + max) - 1)) - 1;
What's the difference of this function with snd_soc_put_volsw()?

Furthermore, the mask calculation and usage in snd_soc_put_volsw()
isn't right, either, I'm afraid; if the range is [-10, 0], max=0, then
mask will 0, which will omit all values...

I guess we need to revisit those functions (or I need more coffee).


thanks,

Takashi

Re: [PATCH] ASoC: ops: Shift tested values in snd_soc_put_volsw() by +min

[Mark Brown]

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

On Wed, Feb 23, 2022 at 03:55:54PM +0100, Takashi Iwai wrote:
> 
> But, more reading the code, I suspect whether the function does work
> correctly at all...  How is the mask calculation done in that way?
>   unsigned int mask = (1U << (fls(min + max) - 1)) - 1;
> What's the difference of this function with snd_soc_put_volsw()?

Yeah, I'm not clear either - Marek mentioned _SX when he was doing the
patch but I didn't get the bandwidth to figure out what it's doing
properly yet.  At this point I'm not clear what _SX is supposed to do,
I'm hoping it works well for the devices that use it but I don't have
any of them.

> Furthermore, the mask calculation and usage in snd_soc_put_volsw()
> isn't right, either, I'm afraid; if the range is [-10, 0], max=0, then
> mask will 0, which will omit all values...

Indeed, if anyone did that.  Fortunately I don't *think* that's an
issue.  The whole way that code handles signed bitfields by remapping
them into unsigned user visible controls is a landmine, it's not even
obvious that they handle signed bitfields in the first place.

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

Re: [PATCH] ASoC: ops: Shift tested values in snd_soc_put_volsw() by +min

[Takashi Iwai]

On Wed, 23 Feb 2022 17:32:19 +0100,
Mark Brown wrote:
> 
> On Wed, Feb 23, 2022 at 03:55:54PM +0100, Takashi Iwai wrote:
> > 
> > But, more reading the code, I suspect whether the function does work
> > correctly at all...  How is the mask calculation done in that way?
> >   unsigned int mask = (1U << (fls(min + max) - 1)) - 1;
> > What's the difference of this function with snd_soc_put_volsw()?
> 
> Yeah, I'm not clear either - Marek mentioned _SX when he was doing the
> patch but I didn't get the bandwidth to figure out what it's doing
> properly yet.  At this point I'm not clear what _SX is supposed to do,
> I'm hoping it works well for the devices that use it but I don't have
> any of them.

OK, let's hope that...

> > Furthermore, the mask calculation and usage in snd_soc_put_volsw()
> > isn't right, either, I'm afraid; if the range is [-10, 0], max=0, then
> > mask will 0, which will omit all values...
> 
> Indeed, if anyone did that.  Fortunately I don't *think* that's an
> issue.  The whole way that code handles signed bitfields by remapping
> them into unsigned user visible controls is a landmine, it's not even
> obvious that they handle signed bitfields in the first place.

Thanks, then it seems OK as is for now.  I guess the signed bit should
be detected by the helper instead of hard-coding, but it's no urgent
issue.


Takashi

Re: [PATCH] ASoC: ops: Shift tested values in snd_soc_put_volsw() by +min

[Marek Vasut]

On 2/23/22 17:32, Mark Brown wrote:
> On Wed, Feb 23, 2022 at 03:55:54PM +0100, Takashi Iwai wrote:
>>
>> But, more reading the code, I suspect whether the function does work
>> correctly at all...  How is the mask calculation done in that way?
>>    unsigned int mask = (1U << (fls(min + max) - 1)) - 1;
>> What's the difference of this function with snd_soc_put_volsw()?
> 
> Yeah, I'm not clear either - Marek mentioned _SX when he was doing the
> patch but I didn't get the bandwidth to figure out what it's doing
> properly yet.  At this point I'm not clear what _SX is supposed to do,
> I'm hoping it works well for the devices that use it but I don't have
> any of them.

Right, I wasn't sure about the remaining two -- volsw_sx and xr_sx -- 
that's why I only did this one I could at least test.

But CS42L51 is on STM32MP1 DKx devkit, CCing Alex , ST might be able to 
look at that one and test.

>> Furthermore, the mask calculation and usage in snd_soc_put_volsw()
>> isn't right, either, I'm afraid; if the range is [-10, 0], max=0, then
>> mask will 0, which will omit all values...
> 
> Indeed, if anyone did that.  Fortunately I don't *think* that's an
> issue.  The whole way that code handles signed bitfields by remapping
> them into unsigned user visible controls is a landmine, it's not even
> obvious that they handle signed bitfields in the first place.

[...]

Re: [PATCH] ASoC: ops: Shift tested values in snd_soc_put_volsw() by +min

[Marek Vasut]

On 2/23/22 17:32, Mark Brown wrote:
> On Wed, Feb 23, 2022 at 03:55:54PM +0100, Takashi Iwai wrote:
>>
>> But, more reading the code, I suspect whether the function does work
>> correctly at all...  How is the mask calculation done in that way?
>>    unsigned int mask = (1U << (fls(min + max) - 1)) - 1;
>> What's the difference of this function with snd_soc_put_volsw()?
> 
> Yeah, I'm not clear either - Marek mentioned _SX when he was doing the
> patch but I didn't get the bandwidth to figure out what it's doing
> properly yet.  At this point I'm not clear what _SX is supposed to do,
> I'm hoping it works well for the devices that use it but I don't have
> any of them.

Right, I wasn't sure about the remaining two -- volsw_sx and xr_sx -- 
that's why I only did this one I could at least test.

But CS42L51 is on STM32MP1 DKx devkit, CCing Alex , ST might be able to 
look at that one and test.

>> Furthermore, the mask calculation and usage in snd_soc_put_volsw()
>> isn't right, either, I'm afraid; if the range is [-10, 0], max=0, then
>> mask will 0, which will omit all values...
> 
> Indeed, if anyone did that.  Fortunately I don't *think* that's an
> issue.  The whole way that code handles signed bitfields by remapping
> them into unsigned user visible controls is a landmine, it's not even
> obvious that they handle signed bitfields in the first place.

[...]

Re: [PATCH] ASoC: ops: Shift tested values in snd_soc_put_volsw() by +min

[Tan N.]

The same changes that are applied to the snd_soc_put_volsw should also 
be applied
to the volsw_sx and xr_sx put callback functions.

Most of the Qualcomm codecs set the volume levels of controls like this
-- SOC_SINGLE_SX_TLV("IIR1 INP1 Volume", LPASS_CDC_IIR1_GAIN_B1_CTL, 0,  
-84, 40, digital_gain) --
which causes the values from the caller to be rejected incorrectly on 
the put callback function.

It took me a lot of time to debug this but because those two functions 
aren't changed
in this patch, it creates an issue where some Android phones have extremely
high amplification on the sidetone mixer during calls which in turn causes
a feedback loop because the kernel can't set the correct level on the 
controls.

Re: [PATCH] ASoC: ops: Shift tested values in snd_soc_put_volsw() by +min

[Tan N.]

The same changes that are applied to the snd_soc_put_volsw should also 
be applied
to the volsw_sx and xr_sx put callback functions.

Most of the Qualcomm codecs set the volume levels of controls like this
-- SOC_SINGLE_SX_TLV("IIR1 INP1 Volume", LPASS_CDC_IIR1_GAIN_B1_CTL, 0,  
-84, 40, digital_gain) --
which causes the values from the caller to be rejected incorrectly on 
the put callback function.

It took me a lot of time to debug this but because those two functions 
aren't changed
in this patch, it creates an issue where some Android phones have extremely
high amplification on the sidetone mixer during calls which in turn causes
a feedback loop because the kernel can't set the correct level on the 
controls.

[PATCH] ASoC: ops: Fix the bounds checking in snd_soc_put_volsw_sx and snd_soc_put_xr_sx

[Tan Nayir]

The $val in both functions has a range between 0 and an arbitrary limit
whereas the range specified with the $min and $max can start
from a negative number. To do the out of bound check correctly, the
$val must be added the $min offset.

Previous-discussion: https://lore.kernel.org/all/c2163c71-2f71-9011-3966-baeab8e8dc8f@gmail.com/
Fixes: 4f1e50d6a9cf9 ("ASoC: ops: Reject out of bounds values in snd_soc_put_volsw_sx()")
Fixes: 4cf28e9ae6e2e ("ASoC: ops: Reject out of bounds values in snd_soc_put_xr_sx()")
Signed-off-by: Tan Nayir <tannayir@gmail.com>
---
 sound/soc/soc-ops.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index e693070f5..42191968c 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -433,7 +433,7 @@ int snd_soc_put_volsw_sx(struct snd_kcontrol *kcontrol,
 	if (ucontrol->value.integer.value[0] < 0)
 		return -EINVAL;
 	val = ucontrol->value.integer.value[0];
-	if (mc->platform_max && val > mc->platform_max)
+	if (mc->platform_max && ((int)val + min) > mc->platform_max)
 		return -EINVAL;
 	if (val > max - min)
 		return -EINVAL;
@@ -910,11 +910,12 @@ int snd_soc_put_xr_sx(struct snd_kcontrol *kcontrol,
 	unsigned int invert = mc->invert;
 	unsigned long mask = (1UL<<mc->nbits)-1;
 	long max = mc->max;
+	long min = mc->min;
 	long val = ucontrol->value.integer.value[0];
 	int ret = 0;
 	unsigned int i;
 
-	if (val < mc->min || val > mc->max)
+	if (val < mc->min || ((int)val + min) > mc->max)
 		return -EINVAL;
 	if (invert)
 		val = max - val;
-- 
2.25.1

[PATCH] ASoC: ops: Fix the bounds checking in snd_soc_put_volsw_sx and snd_soc_put_xr_sx

[Tan Nayir]

The $val in both functions has a range between 0 and an arbitrary limit
whereas the range specified with the $min and $max can start
from a negative number. To do the out of bound check correctly, the
$val must be added the $min offset.

Previous-discussion: https://lore.kernel.org/all/c2163c71-2f71-9011-3966-baeab8e8dc8f@gmail.com/
Fixes: 4f1e50d6a9cf9 ("ASoC: ops: Reject out of bounds values in snd_soc_put_volsw_sx()")
Fixes: 4cf28e9ae6e2e ("ASoC: ops: Reject out of bounds values in snd_soc_put_xr_sx()")
Signed-off-by: Tan Nayir <tannayir@gmail.com>
---
 sound/soc/soc-ops.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index e693070f5..42191968c 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -433,7 +433,7 @@ int snd_soc_put_volsw_sx(struct snd_kcontrol *kcontrol,
 	if (ucontrol->value.integer.value[0] < 0)
 		return -EINVAL;
 	val = ucontrol->value.integer.value[0];
-	if (mc->platform_max && val > mc->platform_max)
+	if (mc->platform_max && ((int)val + min) > mc->platform_max)
 		return -EINVAL;
 	if (val > max - min)
 		return -EINVAL;
@@ -910,11 +910,12 @@ int snd_soc_put_xr_sx(struct snd_kcontrol *kcontrol,
 	unsigned int invert = mc->invert;
 	unsigned long mask = (1UL<<mc->nbits)-1;
 	long max = mc->max;
+	long min = mc->min;
 	long val = ucontrol->value.integer.value[0];
 	int ret = 0;
 	unsigned int i;
 
-	if (val < mc->min || val > mc->max)
+	if (val < mc->min || ((int)val + min) > mc->max)
 		return -EINVAL;
 	if (invert)
 		val = max - val;
-- 
2.25.1

Re: [PATCH] ASoC: ops: Fix the bounds checking in snd_soc_put_volsw_sx and snd_soc_put_xr_sx

[Mark Brown]

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

On Tue, May 17, 2022 at 04:12:04AM +0300, Tan Nayir wrote:

> The $val in both functions has a range between 0 and an arbitrary limit
> whereas the range specified with the $min and $max can start
> from a negative number. To do the out of bound check correctly, the
> $val must be added the $min offset.

>  	val = ucontrol->value.integer.value[0];
> -	if (mc->platform_max && val > mc->platform_max)
> +	if (mc->platform_max && ((int)val + min) > mc->platform_max)

No, the minimum value we expose to userspace is always scaled so that
userspace sees a range starting from zero and that's where platform_max
is referenced to - we're applying this limit before we start remapping
to actual register values.  The code would be a lot simpler if we didn't
do this rescaling.

Please don't send new patches in reply to old patches or serieses, this
makes it harder for both people and tools to understand what is going
on - it can bury things in mailboxes and make it difficult to keep track
of what current patches are, both for the new patches and the old ones.

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

Re: [PATCH] ASoC: ops: Fix the bounds checking in snd_soc_put_volsw_sx and snd_soc_put_xr_sx

[Mark Brown]

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

On Tue, May 17, 2022 at 04:12:04AM +0300, Tan Nayir wrote:

> The $val in both functions has a range between 0 and an arbitrary limit
> whereas the range specified with the $min and $max can start
> from a negative number. To do the out of bound check correctly, the
> $val must be added the $min offset.

>  	val = ucontrol->value.integer.value[0];
> -	if (mc->platform_max && val > mc->platform_max)
> +	if (mc->platform_max && ((int)val + min) > mc->platform_max)

No, the minimum value we expose to userspace is always scaled so that
userspace sees a range starting from zero and that's where platform_max
is referenced to - we're applying this limit before we start remapping
to actual register values.  The code would be a lot simpler if we didn't
do this rescaling.

Please don't send new patches in reply to old patches or serieses, this
makes it harder for both people and tools to understand what is going
on - it can bury things in mailboxes and make it difficult to keep track
of what current patches are, both for the new patches and the old ones.

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

Re: [PATCH] ASoC: ops: Fix the bounds checking in snd_soc_put_volsw_sx and snd_soc_put_xr_sx

[Tan Nayır]

 > No, the minimum value we expose to userspace is always scaled so that
 > userspace sees a range starting from zero and that's where platform_max
 > is referenced to - we're applying this limit before we start remapping
 > to actual register values. The code would be a lot simpler if we didn't
 > do this rescaling.

These are the results that I got from debugging my phone
which has a wcd9340 audio codec and a kernel version of 4.9.314:
The control is defined like
-- SOC_SINGLE_S8_TLV("IIR0 INP0 Volume", 
WCD934X_CDC_SIDETONE_IIR0_IIR_GAIN_B1_CTL, -84, 40, digital_gain) --

Now the OEM mixer_path.xml file defines the value of the aforementioned 
control as 54
which is read by the user-mode Qualcomm HAL, the HAL then uses the 
library libalsa-intf
to issue an IOCTL to pass this value directly to the ALSA driver.
At this point, the snd_soc_put_volsw_sx is called and the $val is 54 as 
expected.
$mc->platform_max is 40, $mc->max is also 40 and $mc->min is -84.

The problem is that the snd_soc_put_volsw_sx, checks the userspace value 
that has a range
starting from 0, directly against the $mc->platform_max value mentioned 
above
which is set to 40 at that point so it checks for the incorrect range.

Re: [PATCH] ASoC: ops: Fix the bounds checking in snd_soc_put_volsw_sx and snd_soc_put_xr_sx

[Tan Nayır]

 > No, the minimum value we expose to userspace is always scaled so that
 > userspace sees a range starting from zero and that's where platform_max
 > is referenced to - we're applying this limit before we start remapping
 > to actual register values. The code would be a lot simpler if we didn't
 > do this rescaling.

These are the results that I got from debugging my phone
which has a wcd9340 audio codec and a kernel version of 4.9.314:
The control is defined like
-- SOC_SINGLE_S8_TLV("IIR0 INP0 Volume", 
WCD934X_CDC_SIDETONE_IIR0_IIR_GAIN_B1_CTL, -84, 40, digital_gain) --

Now the OEM mixer_path.xml file defines the value of the aforementioned 
control as 54
which is read by the user-mode Qualcomm HAL, the HAL then uses the 
library libalsa-intf
to issue an IOCTL to pass this value directly to the ALSA driver.
At this point, the snd_soc_put_volsw_sx is called and the $val is 54 as 
expected.
$mc->platform_max is 40, $mc->max is also 40 and $mc->min is -84.

The problem is that the snd_soc_put_volsw_sx, checks the userspace value 
that has a range
starting from 0, directly against the $mc->platform_max value mentioned 
above
which is set to 40 at that point so it checks for the incorrect range.

Re: [PATCH] ASoC: ops: Fix the bounds checking in snd_soc_put_volsw_sx and snd_soc_put_xr_sx

[Mark Brown]

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

On Tue, May 17, 2022 at 05:25:48PM +0300, Tan Nayır wrote:

> The problem is that the snd_soc_put_volsw_sx, checks the userspace value
> that has a range
> starting from 0, directly against the $mc->platform_max value mentioned
> above
> which is set to 40 at that point so it checks for the incorrect range.

Do you have the fix in 698813ba8c58 ("ASoC: ops: Fix bounds check for
_sx controls")?

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

Re: [PATCH] ASoC: ops: Fix the bounds checking in snd_soc_put_volsw_sx and snd_soc_put_xr_sx

[Mark Brown]

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

On Tue, May 17, 2022 at 05:25:48PM +0300, Tan Nayır wrote:

> The problem is that the snd_soc_put_volsw_sx, checks the userspace value
> that has a range
> starting from 0, directly against the $mc->platform_max value mentioned
> above
> which is set to 40 at that point so it checks for the incorrect range.

Do you have the fix in 698813ba8c58 ("ASoC: ops: Fix bounds check for
_sx controls")?

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

Re: [PATCH] ASoC: ops: Fix the bounds checking in snd_soc_put_volsw_sx and snd_soc_put_xr_sx

[Tan Nayır]

I've debugged the kernel again after applying the fix in
698813ba8c58 ("ASoC: ops: Fix bounds check for _sx controls")
but it didn't fix the problem.

The commit message in your fix states this:
 > For _sx controls the semantics of the max field is not the usual one, max
 > is the number of steps rather than the maximum value. This means that our
 > check in snd_soc_put_volsw_sx() needs to just check against the maximum
 > value.

For some reason, this is not the case on my end.
Both the $platform_max and $max fields are set to the maximum value
of the range that is specified inside the codec code which is -84 to 40
and not the number of steps.
This was also the reason behind my patch to the bounds check.

Re: [PATCH] ASoC: ops: Fix the bounds checking in snd_soc_put_volsw_sx and snd_soc_put_xr_sx

[Tan Nayır]

I've debugged the kernel again after applying the fix in
698813ba8c58 ("ASoC: ops: Fix bounds check for _sx controls")
but it didn't fix the problem.

The commit message in your fix states this:
 > For _sx controls the semantics of the max field is not the usual one, max
 > is the number of steps rather than the maximum value. This means that our
 > check in snd_soc_put_volsw_sx() needs to just check against the maximum
 > value.

For some reason, this is not the case on my end.
Both the $platform_max and $max fields are set to the maximum value
of the range that is specified inside the codec code which is -84 to 40
and not the number of steps.
This was also the reason behind my patch to the bounds check.

Re: [PATCH] ASoC: ops: Fix the bounds checking in snd_soc_put_volsw_sx and snd_soc_put_xr_sx

[Mark Brown]

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

On Tue, May 17, 2022 at 10:58:40PM +0300, Tan Nayır wrote:

> The commit message in your fix states this:
> > For _sx controls the semantics of the max field is not the usual one, max
> > is the number of steps rather than the maximum value. This means that our
> > check in snd_soc_put_volsw_sx() needs to just check against the maximum
> > value.

> For some reason, this is not the case on my end.
> Both the $platform_max and $max fields are set to the maximum value
> of the range that is specified inside the codec code which is -84 to 40
> and not the number of steps.
> This was also the reason behind my patch to the bounds check.

If you look at snd_soc_info_volsw_sx() you can see the code reporting
the range to userspace - you can see the range reported to userspace
there, note that the minimum value reported is unconditionally set to 0.
This is also visible through the API.  What exactly is reported through
the API on your system, and what value is being written?

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

Re: [PATCH] ASoC: ops: Fix the bounds checking in snd_soc_put_volsw_sx and snd_soc_put_xr_sx

[Mark Brown]

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

On Tue, May 17, 2022 at 10:58:40PM +0300, Tan Nayır wrote:

> The commit message in your fix states this:
> > For _sx controls the semantics of the max field is not the usual one, max
> > is the number of steps rather than the maximum value. This means that our
> > check in snd_soc_put_volsw_sx() needs to just check against the maximum
> > value.

> For some reason, this is not the case on my end.
> Both the $platform_max and $max fields are set to the maximum value
> of the range that is specified inside the codec code which is -84 to 40
> and not the number of steps.
> This was also the reason behind my patch to the bounds check.

If you look at snd_soc_info_volsw_sx() you can see the code reporting
the range to userspace - you can see the range reported to userspace
there, note that the minimum value reported is unconditionally set to 0.
This is also visible through the API.  What exactly is reported through
the API on your system, and what value is being written?

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

Re: [PATCH] ASoC: ops: Fix the bounds checking in snd_soc_put_volsw_sx and snd_soc_put_xr_sx

[Tan Nayır]

For a control defined like this:
-- SOC_SINGLE_S8_TLV("IIR0 INP0 Volume", 
WCD934X_CDC_SIDETONE_IIR0_IIR_GAIN_B1_CTL, -84, 40, digital_gain) --
This is what the snd_soc_info_volsw_sx reports:
$mc->platform_max:40, $mc->max:40, $mc->min:-84,
$uinfo->value.integer.max:40, $uinfo->value.integer.min:0

Now this is obviously wrong which is another issue which I'll explain a 
bit later
but the mixer control($mc) limits are exactly the same inside
the snd_soc_put_volsw_sx function.
So the min and max fields inside the $mc are the same in 
snd_soc_put_volsw_sx
so this means that the code without my patch has an incorrect check.

Here's an example, this is the check before the patch:
-- if (mc->platform_max && val > mc->platform_max) --
Let's say the userspace passes 50 as $val which should be within the 
range of
0 to 124 so it is a valid value.
The check is done before the val is re-scaled, so it checks whether the val
is bigger than 40 which is the value of platform_max at that point.

Is the $mc->platform_max supposed to be set to the number of steps
as opposed to the maximum value?

Back to the first issue that I've mentioned in this message, the 
snd_soc_info_volsw_sx
reports the wrong value because it adds the $mc->min to the value which 
not necessary.
Curiously enough, there are actually two commits from 6 years ago
on the Qualcomm's fork of Linux that fix this one.
Neither of these commits exist on the upstream Linux kernel at the 
moment. I've linked them below.

For the sake of integrity, all of the values that I've gathered from 
debugging
were the same before and after applying these patches.
What I mean by that is that the only thing that changes when the patches 
below are applied
is that the snd_soc_info_volsw_sx reports the correct range to the userspace
which should be 0 to 124.
Also the snd_soc_put_volsw_sx still checks the value from userspace
which has a range of 0 to 124 against the maximum of the signed range
which is from -84 to 40 regardless of the patches below.

65c7d020fbee8 ("ASoC: Update the Max value of integer controls.")
https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/commit/65c7d020fbee8070f33072291c32eef7584a56d4

0d873de90eb16 ("ASoC: sound: soc: fix incorrect max value")
https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/commit/0d873de90eb16e3af499eb87da1ed14440b788d5

Re: [PATCH] ASoC: ops: Fix the bounds checking in snd_soc_put_volsw_sx and snd_soc_put_xr_sx

[Tan Nayır]

For a control defined like this:
-- SOC_SINGLE_S8_TLV("IIR0 INP0 Volume", 
WCD934X_CDC_SIDETONE_IIR0_IIR_GAIN_B1_CTL, -84, 40, digital_gain) --
This is what the snd_soc_info_volsw_sx reports:
$mc->platform_max:40, $mc->max:40, $mc->min:-84,
$uinfo->value.integer.max:40, $uinfo->value.integer.min:0

Now this is obviously wrong which is another issue which I'll explain a 
bit later
but the mixer control($mc) limits are exactly the same inside
the snd_soc_put_volsw_sx function.
So the min and max fields inside the $mc are the same in 
snd_soc_put_volsw_sx
so this means that the code without my patch has an incorrect check.

Here's an example, this is the check before the patch:
-- if (mc->platform_max && val > mc->platform_max) --
Let's say the userspace passes 50 as $val which should be within the 
range of
0 to 124 so it is a valid value.
The check is done before the val is re-scaled, so it checks whether the val
is bigger than 40 which is the value of platform_max at that point.

Is the $mc->platform_max supposed to be set to the number of steps
as opposed to the maximum value?

Back to the first issue that I've mentioned in this message, the 
snd_soc_info_volsw_sx
reports the wrong value because it adds the $mc->min to the value which 
not necessary.
Curiously enough, there are actually two commits from 6 years ago
on the Qualcomm's fork of Linux that fix this one.
Neither of these commits exist on the upstream Linux kernel at the 
moment. I've linked them below.

For the sake of integrity, all of the values that I've gathered from 
debugging
were the same before and after applying these patches.
What I mean by that is that the only thing that changes when the patches 
below are applied
is that the snd_soc_info_volsw_sx reports the correct range to the userspace
which should be 0 to 124.
Also the snd_soc_put_volsw_sx still checks the value from userspace
which has a range of 0 to 124 against the maximum of the signed range
which is from -84 to 40 regardless of the patches below.

65c7d020fbee8 ("ASoC: Update the Max value of integer controls.")
https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/commit/65c7d020fbee8070f33072291c32eef7584a56d4

0d873de90eb16 ("ASoC: sound: soc: fix incorrect max value")
https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/commit/0d873de90eb16e3af499eb87da1ed14440b788d5

Re: [PATCH] ASoC: ops: Fix the bounds checking in snd_soc_put_volsw_sx and snd_soc_put_xr_sx

[Mark Brown]

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

On Thu, May 19, 2022 at 02:56:34AM +0300, Tan Nayır wrote:
> For a control defined like this:
> -- SOC_SINGLE_S8_TLV("IIR0 INP0 Volume",
> WCD934X_CDC_SIDETONE_IIR0_IIR_GAIN_B1_CTL, -84, 40, digital_gain) --
> This is what the snd_soc_info_volsw_sx reports:
> $mc->platform_max:40, $mc->max:40, $mc->min:-84,
> $uinfo->value.integer.max:40, $uinfo->value.integer.min:0

OK, so anything setting a value outside of 0..40 was buggy.  Note that
we've not changed the info() code at all here, snd_soc_info_volsw()
subtracts min and then snd_soc_volsw_sx() adds it back on so what we end
up with is whatever max was set to reported as the maximum to userspace,
with the userpace minimum hard coded to zero meaning the range the
control has is 0..max.

> So the min and max fields inside the $mc are the same in snd_soc_put_volsw_sx
> so this means that the code without my patch has an incorrect check.

The check is enforcing the constraint we advertised to userspace, which
should be all that any well written userpace application has accessed
(though I appreciate that due to lack of bounds checking in the ALSA
core it's been possible to do so).

> Is the $mc->platform_max supposed to be set to the number of steps
> as opposed to the maximum value?

It is hard to understand why one would set platform_max in the above
situation other than to limit to -44, however there *is* a lot of
confusion in the code since in the generic function it gets substituted
in like a register value.

> Also the snd_soc_put_volsw_sx still checks the value from userspace
> which has a range of 0 to 124 against the maximum of the signed range
> which is from -84 to 40 regardless of the patches below.

> 65c7d020fbee8 ("ASoC: Update the Max value of integer controls.")
> https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/commit/65c7d020fbee8070f33072291c32eef7584a56d4

That looks confused since it makes the interpretation of platform_max
depend on if the control has a negative bottom for the range which isn't
going to help with maintainability...

> 0d873de90eb16 ("ASoC: sound: soc: fix incorrect max value")
> https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/commit/0d873de90eb16e3af499eb87da1ed14440b788d5

...which I guess is why that bit of the change is reverted in this
commit, though that then has two different interpretations of
platform_max depending on if the control is an integer control for some
reason I can't fathom.  These two would need to be squashed together for
upstreaming, but note that these controls were added by and are used by
non-Qualcomm people (see 34198710f55b5 ASoC: Add info callback for
SX_TLV controls), and note the comment in there about the max being set
to the number of levels rather than a value, so I'm concerned about
other users here, the code doesn't look as self consistent as it should
be.

I think these controls need a separate, clearly written, info() callback
rather than trying to bodge on the side of the main one.  That would
help a lot with working out if the put() is consistent with it.  We
probably also need an audit of all the existing users to try to figure
out what they think they're doing and what, if anything, it's consistent
with.  Your patch is clearly not consistent with the info() callback as
it stands if nothing else.

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

Re: [PATCH] ASoC: ops: Fix the bounds checking in snd_soc_put_volsw_sx and snd_soc_put_xr_sx

[Mark Brown]

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

On Thu, May 19, 2022 at 02:56:34AM +0300, Tan Nayır wrote:
> For a control defined like this:
> -- SOC_SINGLE_S8_TLV("IIR0 INP0 Volume",
> WCD934X_CDC_SIDETONE_IIR0_IIR_GAIN_B1_CTL, -84, 40, digital_gain) --
> This is what the snd_soc_info_volsw_sx reports:
> $mc->platform_max:40, $mc->max:40, $mc->min:-84,
> $uinfo->value.integer.max:40, $uinfo->value.integer.min:0

OK, so anything setting a value outside of 0..40 was buggy.  Note that
we've not changed the info() code at all here, snd_soc_info_volsw()
subtracts min and then snd_soc_volsw_sx() adds it back on so what we end
up with is whatever max was set to reported as the maximum to userspace,
with the userpace minimum hard coded to zero meaning the range the
control has is 0..max.

> So the min and max fields inside the $mc are the same in snd_soc_put_volsw_sx
> so this means that the code without my patch has an incorrect check.

The check is enforcing the constraint we advertised to userspace, which
should be all that any well written userpace application has accessed
(though I appreciate that due to lack of bounds checking in the ALSA
core it's been possible to do so).

> Is the $mc->platform_max supposed to be set to the number of steps
> as opposed to the maximum value?

It is hard to understand why one would set platform_max in the above
situation other than to limit to -44, however there *is* a lot of
confusion in the code since in the generic function it gets substituted
in like a register value.

> Also the snd_soc_put_volsw_sx still checks the value from userspace
> which has a range of 0 to 124 against the maximum of the signed range
> which is from -84 to 40 regardless of the patches below.

> 65c7d020fbee8 ("ASoC: Update the Max value of integer controls.")
> https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/commit/65c7d020fbee8070f33072291c32eef7584a56d4

That looks confused since it makes the interpretation of platform_max
depend on if the control has a negative bottom for the range which isn't
going to help with maintainability...

> 0d873de90eb16 ("ASoC: sound: soc: fix incorrect max value")
> https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/commit/0d873de90eb16e3af499eb87da1ed14440b788d5

...which I guess is why that bit of the change is reverted in this
commit, though that then has two different interpretations of
platform_max depending on if the control is an integer control for some
reason I can't fathom.  These two would need to be squashed together for
upstreaming, but note that these controls were added by and are used by
non-Qualcomm people (see 34198710f55b5 ASoC: Add info callback for
SX_TLV controls), and note the comment in there about the max being set
to the number of levels rather than a value, so I'm concerned about
other users here, the code doesn't look as self consistent as it should
be.

I think these controls need a separate, clearly written, info() callback
rather than trying to bodge on the side of the main one.  That would
help a lot with working out if the put() is consistent with it.  We
probably also need an audit of all the existing users to try to figure
out what they think they're doing and what, if anything, it's consistent
with.  Your patch is clearly not consistent with the info() callback as
it stands if nothing else.

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