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

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20220519/08bd5113/attachment.sig>