SOC_DOUBLE_R_SX_TLV controls broken in cs24l51 driver

SOC_DOUBLE_R_SX_TLV controls broken in cs24l51 driver

[Guillermo Rodriguez Garcia]

Hi all,

I am using a dev board with a Cirrus Logic cs24l51 codec.

This used to work fine prior to kernel version 5.x, however after 5.x
it is not possible to set certain values for ALSA controls from
userspace.

I believe this is related to the input validation that is mentioned in
this thread: https://lore.kernel.org/all/Yph8C3bRxcr6ogW7@sirena.org.uk/T/,
and possibly in this commit: 4f1e50d6a9cf9c1b8c859d449b5031cacfa8404e
("ASoC: ops: Reject out of bounds values in snd_soc_put_volsw_sx()")

For the cs24l51, all the controls that fail are using the
SOC_DOUBLE_R_SX_TLV macro.

I have traced this to the checks in snd_soc_put_volsw_sx, specifically
the (val > max - min) check:

pr_warn("Max: %d, Min: %d, Value: %d", max, min, val);
pr_warn("platform_max: %d", mc->platform_max);
if (mc->platform_max && val > mc->platform_max)
{
    return -EINVAL;
}
if (val > max - min){
    pr_warn("(val > max - min) check failed");
    return -EINVAL;
}
if (val < 0)
    return -EINVAL;

According to the datasheet of the CS24L51, section 6.13, page 61, the
PCMMIXX_VOL registers accept the following range of values:

Binary Code / Volume Setting
001 1000 / +12.0 dB
··· ···
000 0000 / 0 dB
111 1111 / -0.5 dB
111 1110 / -1.0 dB
··· ···
001 1001 / -51.5 dB

Minimum value is 0x19 (001 1001) corresponding to -51.5 dB, then there
are 127 possible gain settings from -51.5 dB to +12.0 dB, in 0.5 dB
steps.

This is declared in the driver as follows:

SOC_DOUBLE_R_SX_TLV("PCM Playback Volume",
CS42L51_PCMA_VOL, CS42L51_PCMB_VOL,
0, 0x19, 0x7F, adc_pcm_tlv),

0x19 = min value
0x7F = number of gain settings

This seems to be correct according to the semantics of the
SOC_DOUBLE_R_SX_TLV macro as described in commit
34198710f55b5f359f43e67d9a08fe5aadfbca1b ("ASoC: Add info callback for
SX_TLV controls").

However, the (val > max - min) check in snd_soc_put_volsw_sx fails in
the above example because val = 127, max - min = 127 - 25 = 102.

So I am not sure how this should be fixed. Is the SX_TLV macro being
used incorrectly here? Is the check in snd_soc_put_volsw_sx wrong?

Any pointers are welcome.

Thanks,

(If possible, please CC me in any replies)

Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com

Re: SOC_DOUBLE_R_SX_TLV controls broken in cs24l51 driver

[Charles Keepax]

On Thu, Nov 24, 2022 at 10:57:34AM +0100, Guillermo Rodriguez Garcia wrote:
> Hi all,
> 
> I am using a dev board with a Cirrus Logic cs24l51 codec.
> 
> This used to work fine prior to kernel version 5.x, however after 5.x
> it is not possible to set certain values for ALSA controls from
> userspace.
> 
> I believe this is related to the input validation that is mentioned in
> this thread: https://lore.kernel.org/all/Yph8C3bRxcr6ogW7@sirena.org.uk/T/,
> and possibly in this commit: 4f1e50d6a9cf9c1b8c859d449b5031cacfa8404e
> ("ASoC: ops: Reject out of bounds values in snd_soc_put_volsw_sx()")
> 
> For the cs24l51, all the controls that fail are using the
> SOC_DOUBLE_R_SX_TLV macro.
> 
> I have traced this to the checks in snd_soc_put_volsw_sx, specifically
> the (val > max - min) check:
> 

Can you try these two patches:

https://lore.kernel.org/all/165236477046.1016627.15470197691244479154.b4-ty@kernel.org/

It looks like they somehow got lost around 5.18, I have been
meaning to chase up what happened to them.

Thanks,
Charles

Re: SOC_DOUBLE_R_SX_TLV controls broken in cs24l51 driver

[Guillermo Rodriguez Garcia]

El jue, 24 nov 2022 a las 12:13, Charles Keepax
(<ckeepax@opensource.cirrus.com>) escribió:
>
> On Thu, Nov 24, 2022 at 10:57:34AM +0100, Guillermo Rodriguez Garcia wrote:
> > Hi all,
> >
> > I am using a dev board with a Cirrus Logic cs24l51 codec.
> >
> > This used to work fine prior to kernel version 5.x, however after 5.x
> > it is not possible to set certain values for ALSA controls from
> > userspace.
> >
> > I believe this is related to the input validation that is mentioned in
> > this thread: https://lore.kernel.org/all/Yph8C3bRxcr6ogW7@sirena.org.uk/T/,
> > and possibly in this commit: 4f1e50d6a9cf9c1b8c859d449b5031cacfa8404e
> > ("ASoC: ops: Reject out of bounds values in snd_soc_put_volsw_sx()")
> >
> > For the cs24l51, all the controls that fail are using the
> > SOC_DOUBLE_R_SX_TLV macro.
> >
> > I have traced this to the checks in snd_soc_put_volsw_sx, specifically
> > the (val > max - min) check:
> >
>
> Can you try these two patches:
>
> https://lore.kernel.org/all/165236477046.1016627.15470197691244479154.b4-ty@kernel.org/

Thanks.
In my tests, these patches seem to fix the problem for some values,
but not for all of them:

$ amixer cset name='Analog Playback Volume' '208','208'
numid=3,iface=MIXER,name='Analog Playback Volume'
  ; type=INTEGER,access=rw---R--,values=2,min=0,max=228,step=0
  : values=208,208
  | dBscale-min=-102.00dB,step=0.50dB,mute=0

$ amixer cset name='Analog Playback Volume' '180','180'
amixer: Control default element write error: Invalid argument

Looking at the code I'd say that patch 1/2 is correct however I have
doubts about patch 2/2:

        val_mask = mask << rshift;
        val2 = (ucontrol->value.integer.value[1] + min) & mask;
+
+       if (mc->platform_max && val2 > mc->platform_max)
+           return -EINVAL;
+       if (val2 > max)
+           return -EINVAL;
+
        val2 = val2 << rshift;

        err = snd_soc_component_update_bits(component, reg2, val_mask,

The checks for max and platform_max are done on val2, but val2 is
already the result of adding the minimum value ('min') and applying
the mask.
Shouldn't the checks be done on ucontrol->value.integer.value[1] instead?

Thanks,

Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com

Re: SOC_DOUBLE_R_SX_TLV controls broken in cs24l51 driver

[Charles Keepax]

On Thu, Nov 24, 2022 at 02:54:15PM +0100, Guillermo Rodriguez Garcia wrote:
> El jue, 24 nov 2022 a las 12:13, Charles Keepax
> (<ckeepax@opensource.cirrus.com>) escribió:
> >
> > On Thu, Nov 24, 2022 at 10:57:34AM +0100, Guillermo Rodriguez Garcia wrote:
> > > Hi all,
> > >
> > > I am using a dev board with a Cirrus Logic cs24l51 codec.
> > >
> > > This used to work fine prior to kernel version 5.x, however after 5.x
> > > it is not possible to set certain values for ALSA controls from
> > > userspace.
> > >
> > > I believe this is related to the input validation that is mentioned in
> > > this thread: https://lore.kernel.org/all/Yph8C3bRxcr6ogW7@sirena.org.uk/T/,
> > > and possibly in this commit: 4f1e50d6a9cf9c1b8c859d449b5031cacfa8404e
> > > ("ASoC: ops: Reject out of bounds values in snd_soc_put_volsw_sx()")
> > >
> > > For the cs24l51, all the controls that fail are using the
> > > SOC_DOUBLE_R_SX_TLV macro.
> > >
> > > I have traced this to the checks in snd_soc_put_volsw_sx, specifically
> > > the (val > max - min) check:
> > >
> >
> > Can you try these two patches:
> >
> > https://lore.kernel.org/all/165236477046.1016627.15470197691244479154.b4-ty@kernel.org/
> 
> Thanks.
> In my tests, these patches seem to fix the problem for some values,
> but not for all of them:
> 
> $ amixer cset name='Analog Playback Volume' '208','208'
> numid=3,iface=MIXER,name='Analog Playback Volume'
>   ; type=INTEGER,access=rw---R--,values=2,min=0,max=228,step=0
>   : values=208,208
>   | dBscale-min=-102.00dB,step=0.50dB,mute=0
> 
> $ amixer cset name='Analog Playback Volume' '180','180'
> amixer: Control default element write error: Invalid argument
> 
> Looking at the code I'd say that patch 1/2 is correct however I have
> doubts about patch 2/2:
> 
>         val_mask = mask << rshift;
>         val2 = (ucontrol->value.integer.value[1] + min) & mask;
> +
> +       if (mc->platform_max && val2 > mc->platform_max)
> +           return -EINVAL;
> +       if (val2 > max)
> +           return -EINVAL;
> +
>         val2 = val2 << rshift;
> 
>         err = snd_soc_component_update_bits(component, reg2, val_mask,
> 
> The checks for max and platform_max are done on val2, but val2 is
> already the result of adding the minimum value ('min') and applying
> the mask.
> Shouldn't the checks be done on ucontrol->value.integer.value[1] instead?
> 

Yeah they definitely should, I have resent the two patches
including that fixup, lets see what Mark says. You are CCed
on them so be great if you could give them a test too.

Thanks,
Charles

Re: SOC_DOUBLE_R_SX_TLV controls broken in cs24l51 driver

[Mark Brown]

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

On Fri, Nov 25, 2022 at 03:50:23PM +0000, Charles Keepax wrote:

> Yeah they definitely should, I have resent the two patches
> including that fixup, lets see what Mark says. You are CCed
> on them so be great if you could give them a test too.

Please send the incremental fix (which you didn't mention in the cover
leter...) as an incremental fix.

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