| Message ID | 1652980212-21473-1-git-send-email-khoroshilov@ispras.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: max98090: Remove unneeded check in max98090_put_enab_tlv() | expand |
On Thu, May 19, 2022 at 08:10:12PM +0300, Alexey Khoroshilov wrote: > Variable sel is of unsigned int type, so sel < 0 is not required. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > val = (val >> mc->shift) & mask; > > - if (sel < 0 || sel > mc->max) > + if (sel > mc->max) The check needs to be moved, not removed. The userspace ABI allows passing in of negative values.
On 5/19/22 12:29, Mark Brown wrote: > On Thu, May 19, 2022 at 08:10:12PM +0300, Alexey Khoroshilov wrote: >> Variable sel is of unsigned int type, so sel < 0 is not required. >> >> Found by Linux Verification Center (linuxtesting.org) with SVACE. > >> val = (val >> mc->shift) & mask; >> >> - if (sel < 0 || sel > mc->max) >> + if (sel > mc->max) > > The check needs to be moved, not removed. The userspace ABI allows > passing in of negative values. I was about to send the same cleanup, cppcheck reports the same issue with a useless test. The problem is that the values coming from userspace are cast to unsigned...
On 19.05.2022 20:29, Mark Brown wrote: > On Thu, May 19, 2022 at 08:10:12PM +0300, Alexey Khoroshilov wrote: >> Variable sel is of unsigned int type, so sel < 0 is not required. >> >> Found by Linux Verification Center (linuxtesting.org) with SVACE. > >> val = (val >> mc->shift) & mask; >> >> - if (sel < 0 || sel > mc->max) >> + if (sel > mc->max) > > The check needs to be moved, not removed. The userspace ABI allows > passing in of negative values. > Would (sel > mc->max) be enough in this case anyway? -- Alexey
On Thu, May 19, 2022 at 08:49:48PM +0300, Alexey Khoroshilov wrote: > On 19.05.2022 20:29, Mark Brown wrote: > > On Thu, May 19, 2022 at 08:10:12PM +0300, Alexey Khoroshilov wrote: > >> - if (sel < 0 || sel > mc->max) > >> + if (sel > mc->max) > > The check needs to be moved, not removed. The userspace ABI allows > > passing in of negative values. > Would (sel > mc->max) be enough in this case anyway? Oh, the check won't be working properly - it's just that like I say the fix is to move rather than remove it so it's operating on the signed value.
On 19.05.2022 20:54, Mark Brown wrote: > On Thu, May 19, 2022 at 08:49:48PM +0300, Alexey Khoroshilov wrote: >> On 19.05.2022 20:29, Mark Brown wrote: >>> On Thu, May 19, 2022 at 08:10:12PM +0300, Alexey Khoroshilov wrote: > >>>> - if (sel < 0 || sel > mc->max) >>>> + if (sel > mc->max) > >>> The check needs to be moved, not removed. The userspace ABI allows >>> passing in of negative values. > >> Would (sel > mc->max) be enough in this case anyway? > > Oh, the check won't be working properly - it's just that like I say the > fix is to move rather than remove it so it's operating on the signed > value. > Do you mean something like this? static int max98090_put_enab_tlv(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol); struct max98090_priv *max98090 = snd_soc_component_get_drvdata(component); struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value; unsigned int mask = (1 << fls(mc->max)) - 1; - unsigned int sel = ucontrol->value.integer.value[0]; + int sel_unchecked = ucontrol->value.integer.value[0]; + unsigned int sel; unsigned int val = snd_soc_component_read(component, mc->reg); unsigned int *select; switch (mc->reg) { case M98090_REG_MIC1_INPUT_LEVEL: select = &(max98090->pa1en); break; case M98090_REG_MIC2_INPUT_LEVEL: select = &(max98090->pa2en); break; case M98090_REG_ADC_SIDETONE: select = &(max98090->sidetone); break; default: return -EINVAL; } val = (val >> mc->shift) & mask; - if (sel < 0 || sel > mc->max) + if (sel_unchecked < 0 || sel_unchecked > mc->max) return -EINVAL; + sel = sel_unchecked; *select = sel;
On Thu, May 19, 2022 at 09:27:25PM +0300, Alexey Khoroshilov wrote: > On 19.05.2022 20:54, Mark Brown wrote: > > Oh, the check won't be working properly - it's just that like I say the > > fix is to move rather than remove it so it's operating on the signed > > value. > Do you mean something like this? That looks about right.
On 19.05.2022 23:07, Mark Brown wrote: > On Thu, May 19, 2022 at 09:27:25PM +0300, Alexey Khoroshilov wrote: >> On 19.05.2022 20:54, Mark Brown wrote: >>> Oh, the check won't be working properly - it's just that like I say the >>> fix is to move rather than remove it so it's operating on the signed >>> value. >> Do you mean something like this? > That looks about right. Should I prepare a patch or you will do it yourself?
On Thu, May 19, 2022 at 11:13:00PM +0300, Alexey Khoroshilov wrote: > On 19.05.2022 23:07, Mark Brown wrote: > > On Thu, May 19, 2022 at 09:27:25PM +0300, Alexey Khoroshilov wrote: > >> Do you mean something like this? > > That looks about right. > Should I prepare a patch or you will do it yourself? Please send it, you already wrote it - may as well get the credit too.
diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c index 62b41ca050a2..c535a8496bf1 100644 --- a/sound/soc/codecs/max98090.c +++ b/sound/soc/codecs/max98090.c @@ -413,7 +413,7 @@ static int max98090_put_enab_tlv(struct snd_kcontrol *kcontrol, val = (val >> mc->shift) & mask; - if (sel < 0 || sel > mc->max) + if (sel > mc->max) return -EINVAL; *select = sel;
Variable sel is of unsigned int type, so sel < 0 is not required. Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru> Fixes: 2fbe467bcbfc ("ASoC: max98090: Reject invalid values in custom control put()") --- sound/soc/codecs/max98090.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)