[PATCH] ASoC: ops: Shift tested values in snd_soc_put_volsw() by +min
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;
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
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
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.
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
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.
[...]
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.
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;
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.
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.
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")?
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.
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?
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/65c7d020fbee8070f3...
0d873de90eb16 ("ASoC: sound: soc: fix incorrect max value") https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/commit/0d873de90eb16e3af4...
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/65c7d020fbee8070f3...
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/0d873de90eb16e3af4...
...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.
participants (6)
-
Marek Vasut -
Mark Brown -
Takashi Iwai -
Tan N. -
Tan Nayir
-
Tan Nayır