| Message ID | 20220215130645.164025-1-marex@denx.de |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: ops: Shift tested values in snd_soc_put_volsw() by +min | expand |
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.
> 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.
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;
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(-)