| Message ID | 20220215130645.164025-1-marex@denx.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9bdd10d57a8807dba0003af0325191f3cec0f11c |
| 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. > > 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.
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(-)