issue with uninitialized value used in a comparison in gbcodec_mixer_dapm_ctl_put

issue with uninitialized value used in a comparison in gbcodec_mixer_dapm_ctl_put

[Colin Ian King]

Hi,

Static analysis with Coverity has detected an uninitialized value being
used in a comparison.  The error was detected on a recent change to
drivers/staging/greybus/audio_topology.c however the issue actually
dates back to the original commit:

commit 6339d2322c47f4b8ebabf9daf0130328ed72648b
Author: Vaibhav Agarwal <vaibhav.agarwal@linaro.org>
Date:   Wed Jan 13 14:07:51 2016 -0700

    greybus: audio: Add topology parser for GB codec

The analysis is as follows:

425 static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol,
426                                      struct snd_ctl_elem_value
*ucontrol)
427 {
428        int ret, wi, max, connect;
429        unsigned int mask, val;
430        struct gb_audio_ctl_elem_info *info;
431        struct gbaudio_ctl_pvt *data;

   1. var_decl: Declaring variable gbvalue without initializer.
432        struct gb_audio_ctl_elem_value gbvalue;
433        struct gbaudio_module_info *module;
434        struct snd_soc_dapm_widget_list *wlist =
snd_kcontrol_chip(kcontrol);
435        struct snd_soc_dapm_widget *widget = wlist->widgets[0];
436        struct device *codec_dev = widget->dapm->dev;
437        struct gbaudio_codec_info *gb = dev_get_drvdata(codec_dev);
438        struct gb_bundle *bundle;
439

   2. Condition 0 /* __builtin_types_compatible_p() */, taking false branch.
   3. Condition 1 /* __builtin_types_compatible_p() */, taking true branch.
   4. Falling through to end of if statement.
   5. Condition !!branch, taking false branch.
   6. Condition ({...; !!branch;}), taking false branch.

440        dev_dbg(codec_dev, "Entered %s:%s\n", __func__,
kcontrol->id.name);
441        module = find_gb_module(gb, kcontrol->id.name);

   7. Condition !module, taking false branch.
442        if (!module)
443                return -EINVAL;
444
445        data = (struct gbaudio_ctl_pvt *)kcontrol->private_value;
446        info = (struct gb_audio_ctl_elem_info *)data->info;

   8. Condition 0 /* !!(!__builtin_types_compatible_p() &&
!__builtin_types_compatible_p()) */, taking false branch.
447        bundle = to_gb_bundle(module->dev);
448

   9. Condition data->vcount == 2, taking true branch.
449        if (data->vcount == 2)
450                dev_warn(widget->dapm->dev,
451                         "GB: Control '%s' is stereo, which is not
supported\n",
452                         kcontrol->id.name);
453
454        max = le32_to_cpu(info->value.integer.max);
455        mask = (1 << fls(max)) - 1;
456        val = ucontrol->value.integer.value[0] & mask;

   10. Condition !!val, taking true branch.
457        connect = !!val;
458
459        /* update ucontrol */

Uninitialized scalar variable (UNINIT)
   11. uninit_use: Using uninitialized value gbvalue.value.integer_value[0].
460        if (gbvalue.value.integer_value[0] != val) {

The gbvalue.value.integer_value[0] read is bogus since gbvalue was
declared on the stack but was not initialized.  There seems to be no
where that sets this data. I'm assuming most of the time that the
comparison works because the garbage value is different from val and so
the code in the if stanza is executed.

Anyhow, I'm unsure what the original intent of the code was, so I've not
attempted to fix this.

Colin

issue with uninitialized value used in a comparison in gbcodec_mixer_dapm_ctl_put

[Colin Ian King]

Hi,

Static analysis with Coverity has detected an uninitialized value being
used in a comparison.  The error was detected on a recent change to
drivers/staging/greybus/audio_topology.c however the issue actually
dates back to the original commit:

commit 6339d2322c47f4b8ebabf9daf0130328ed72648b
Author: Vaibhav Agarwal <vaibhav.agarwal@linaro.org>
Date:   Wed Jan 13 14:07:51 2016 -0700

    greybus: audio: Add topology parser for GB codec

The analysis is as follows:

425 static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol,
426                                      struct snd_ctl_elem_value
*ucontrol)
427 {
428        int ret, wi, max, connect;
429        unsigned int mask, val;
430        struct gb_audio_ctl_elem_info *info;
431        struct gbaudio_ctl_pvt *data;

   1. var_decl: Declaring variable gbvalue without initializer.
432        struct gb_audio_ctl_elem_value gbvalue;
433        struct gbaudio_module_info *module;
434        struct snd_soc_dapm_widget_list *wlist =
snd_kcontrol_chip(kcontrol);
435        struct snd_soc_dapm_widget *widget = wlist->widgets[0];
436        struct device *codec_dev = widget->dapm->dev;
437        struct gbaudio_codec_info *gb = dev_get_drvdata(codec_dev);
438        struct gb_bundle *bundle;
439

   2. Condition 0 /* __builtin_types_compatible_p() */, taking false branch.
   3. Condition 1 /* __builtin_types_compatible_p() */, taking true branch.
   4. Falling through to end of if statement.
   5. Condition !!branch, taking false branch.
   6. Condition ({...; !!branch;}), taking false branch.

440        dev_dbg(codec_dev, "Entered %s:%s\n", __func__,
kcontrol->id.name);
441        module = find_gb_module(gb, kcontrol->id.name);

   7. Condition !module, taking false branch.
442        if (!module)
443                return -EINVAL;
444
445        data = (struct gbaudio_ctl_pvt *)kcontrol->private_value;
446        info = (struct gb_audio_ctl_elem_info *)data->info;

   8. Condition 0 /* !!(!__builtin_types_compatible_p() &&
!__builtin_types_compatible_p()) */, taking false branch.
447        bundle = to_gb_bundle(module->dev);
448

   9. Condition data->vcount == 2, taking true branch.
449        if (data->vcount == 2)
450                dev_warn(widget->dapm->dev,
451                         "GB: Control '%s' is stereo, which is not
supported\n",
452                         kcontrol->id.name);
453
454        max = le32_to_cpu(info->value.integer.max);
455        mask = (1 << fls(max)) - 1;
456        val = ucontrol->value.integer.value[0] & mask;

   10. Condition !!val, taking true branch.
457        connect = !!val;
458
459        /* update ucontrol */

Uninitialized scalar variable (UNINIT)
   11. uninit_use: Using uninitialized value gbvalue.value.integer_value[0].
460        if (gbvalue.value.integer_value[0] != val) {

The gbvalue.value.integer_value[0] read is bogus since gbvalue was
declared on the stack but was not initialized.  There seems to be no
where that sets this data. I'm assuming most of the time that the
comparison works because the garbage value is different from val and so
the code in the if stanza is executed.

Anyhow, I'm unsure what the original intent of the code was, so I've not
attempted to fix this.

Colin


_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: issue with uninitialized value used in a comparison in gbcodec_mixer_dapm_ctl_put

[Vaibhav Agarwal]

On Thu, Jul 30, 2020 at 05:02:22PM +0100, Colin Ian King wrote:
> Hi,
> 
> Static analysis with Coverity has detected an uninitialized value being
> used in a comparison.  The error was detected on a recent change to
> drivers/staging/greybus/audio_topology.c however the issue actually
> dates back to the original commit:

Thanks Colin for reporting the issue. I'll fix the same and share a 
patch soon.

--
Regards,
Vaibhav

Re: issue with uninitialized value used in a comparison in gbcodec_mixer_dapm_ctl_put

[Vaibhav Agarwal]

On Thu, Jul 30, 2020 at 05:02:22PM +0100, Colin Ian King wrote:
> Hi,
> 
> Static analysis with Coverity has detected an uninitialized value being
> used in a comparison.  The error was detected on a recent change to
> drivers/staging/greybus/audio_topology.c however the issue actually
> dates back to the original commit:

Thanks Colin for reporting the issue. I'll fix the same and share a 
patch soon.

--
Regards,
Vaibhav
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: issue with uninitialized value used in a comparison in gbcodec_mixer_dapm_ctl_put

[Alex Elder]

On 7/30/20 11:02 AM, Colin Ian King wrote:
> Hi,
> 
> Static analysis with Coverity has detected an uninitialized value being
> used in a comparison.  The error was detected on a recent change to
> drivers/staging/greybus/audio_topology.c however the issue actually
> dates back to the original commit:
> 
> commit 6339d2322c47f4b8ebabf9daf0130328ed72648b
> Author: Vaibhav Agarwal <vaibhav.agarwal@linaro.org>
> Date:   Wed Jan 13 14:07:51 2016 -0700
> 
>     greybus: audio: Add topology parser for GB codec
> 
> The analysis is as follows:
> 
> 425 static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol,
> 426                                      struct snd_ctl_elem_value
> *ucontrol)
> 427 {
> 428        int ret, wi, max, connect;
> 429        unsigned int mask, val;
> 430        struct gb_audio_ctl_elem_info *info;
> 431        struct gbaudio_ctl_pvt *data;
> 
>    1. var_decl: Declaring variable gbvalue without initializer.
> 432        struct gb_audio_ctl_elem_value gbvalue;
> 433        struct gbaudio_module_info *module;
> 434        struct snd_soc_dapm_widget_list *wlist =
> snd_kcontrol_chip(kcontrol);
> 435        struct snd_soc_dapm_widget *widget = wlist->widgets[0];
> 436        struct device *codec_dev = widget->dapm->dev;
> 437        struct gbaudio_codec_info *gb = dev_get_drvdata(codec_dev);
> 438        struct gb_bundle *bundle;
> 439
> 
>    2. Condition 0 /* __builtin_types_compatible_p() */, taking false branch.
>    3. Condition 1 /* __builtin_types_compatible_p() */, taking true branch.
>    4. Falling through to end of if statement.
>    5. Condition !!branch, taking false branch.
>    6. Condition ({...; !!branch;}), taking false branch.
> 
> 440        dev_dbg(codec_dev, "Entered %s:%s\n", __func__,
> kcontrol->id.name);
> 441        module = find_gb_module(gb, kcontrol->id.name);
> 
>    7. Condition !module, taking false branch.
> 442        if (!module)
> 443                return -EINVAL;
> 444
> 445        data = (struct gbaudio_ctl_pvt *)kcontrol->private_value;
> 446        info = (struct gb_audio_ctl_elem_info *)data->info;
> 
>    8. Condition 0 /* !!(!__builtin_types_compatible_p() &&
> !__builtin_types_compatible_p()) */, taking false branch.
> 447        bundle = to_gb_bundle(module->dev);
> 448
> 
>    9. Condition data->vcount == 2, taking true branch.
> 449        if (data->vcount == 2)
> 450                dev_warn(widget->dapm->dev,
> 451                         "GB: Control '%s' is stereo, which is not
> supported\n",
> 452                         kcontrol->id.name);
> 453
> 454        max = le32_to_cpu(info->value.integer.max);
> 455        mask = (1 << fls(max)) - 1;
> 456        val = ucontrol->value.integer.value[0] & mask;
> 
>    10. Condition !!val, taking true branch.
> 457        connect = !!val;
> 458
> 459        /* update ucontrol */
> 
> Uninitialized scalar variable (UNINIT)
>    11. uninit_use: Using uninitialized value gbvalue.value.integer_value[0].
> 460        if (gbvalue.value.integer_value[0] != val) {
> 
> The gbvalue.value.integer_value[0] read is bogus since gbvalue was
> declared on the stack but was not initialized.  There seems to be no
> where that sets this data. I'm assuming most of the time that the
> comparison works because the garbage value is different from val and so
> the code in the if stanza is executed.
> 
> Anyhow, I'm unsure what the original intent of the code was, so I've not
> attempted to fix this.

I think the fix is to add a call to this:

        ret = gb_audio_gb_get_control(module->mgmt_connection, data->ctl_id,
                                      GB_AUDIO_INVALID_INDEX, &gbvalue);

before the field within gbvalue is used.

Looking at gbcodec_mixer_dapm_ctl_get() defined just above that, it
seems that the call to gb_audio_gb_get_control() should be preceded
by a call to gb_pm_runtime_get_sync().  And given that duplication,
I suggest this call and the PM runtime wrapper functions should be
placed in a new helper function.

I know that Vaibhav said he would be fixing this, so I guess my
comments are directed at him.  Thanks for sending the patch Colin.

					-Alex


> Colin
> 
> 

Re: issue with uninitialized value used in a comparison in gbcodec_mixer_dapm_ctl_put

[Alex Elder]

On 7/30/20 11:02 AM, Colin Ian King wrote:
> Hi,
> 
> Static analysis with Coverity has detected an uninitialized value being
> used in a comparison.  The error was detected on a recent change to
> drivers/staging/greybus/audio_topology.c however the issue actually
> dates back to the original commit:
> 
> commit 6339d2322c47f4b8ebabf9daf0130328ed72648b
> Author: Vaibhav Agarwal <vaibhav.agarwal@linaro.org>
> Date:   Wed Jan 13 14:07:51 2016 -0700
> 
>     greybus: audio: Add topology parser for GB codec
> 
> The analysis is as follows:
> 
> 425 static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol,
> 426                                      struct snd_ctl_elem_value
> *ucontrol)
> 427 {
> 428        int ret, wi, max, connect;
> 429        unsigned int mask, val;
> 430        struct gb_audio_ctl_elem_info *info;
> 431        struct gbaudio_ctl_pvt *data;
> 
>    1. var_decl: Declaring variable gbvalue without initializer.
> 432        struct gb_audio_ctl_elem_value gbvalue;
> 433        struct gbaudio_module_info *module;
> 434        struct snd_soc_dapm_widget_list *wlist =
> snd_kcontrol_chip(kcontrol);
> 435        struct snd_soc_dapm_widget *widget = wlist->widgets[0];
> 436        struct device *codec_dev = widget->dapm->dev;
> 437        struct gbaudio_codec_info *gb = dev_get_drvdata(codec_dev);
> 438        struct gb_bundle *bundle;
> 439
> 
>    2. Condition 0 /* __builtin_types_compatible_p() */, taking false branch.
>    3. Condition 1 /* __builtin_types_compatible_p() */, taking true branch.
>    4. Falling through to end of if statement.
>    5. Condition !!branch, taking false branch.
>    6. Condition ({...; !!branch;}), taking false branch.
> 
> 440        dev_dbg(codec_dev, "Entered %s:%s\n", __func__,
> kcontrol->id.name);
> 441        module = find_gb_module(gb, kcontrol->id.name);
> 
>    7. Condition !module, taking false branch.
> 442        if (!module)
> 443                return -EINVAL;
> 444
> 445        data = (struct gbaudio_ctl_pvt *)kcontrol->private_value;
> 446        info = (struct gb_audio_ctl_elem_info *)data->info;
> 
>    8. Condition 0 /* !!(!__builtin_types_compatible_p() &&
> !__builtin_types_compatible_p()) */, taking false branch.
> 447        bundle = to_gb_bundle(module->dev);
> 448
> 
>    9. Condition data->vcount == 2, taking true branch.
> 449        if (data->vcount == 2)
> 450                dev_warn(widget->dapm->dev,
> 451                         "GB: Control '%s' is stereo, which is not
> supported\n",
> 452                         kcontrol->id.name);
> 453
> 454        max = le32_to_cpu(info->value.integer.max);
> 455        mask = (1 << fls(max)) - 1;
> 456        val = ucontrol->value.integer.value[0] & mask;
> 
>    10. Condition !!val, taking true branch.
> 457        connect = !!val;
> 458
> 459        /* update ucontrol */
> 
> Uninitialized scalar variable (UNINIT)
>    11. uninit_use: Using uninitialized value gbvalue.value.integer_value[0].
> 460        if (gbvalue.value.integer_value[0] != val) {
> 
> The gbvalue.value.integer_value[0] read is bogus since gbvalue was
> declared on the stack but was not initialized.  There seems to be no
> where that sets this data. I'm assuming most of the time that the
> comparison works because the garbage value is different from val and so
> the code in the if stanza is executed.
> 
> Anyhow, I'm unsure what the original intent of the code was, so I've not
> attempted to fix this.

I think the fix is to add a call to this:

        ret = gb_audio_gb_get_control(module->mgmt_connection, data->ctl_id,
                                      GB_AUDIO_INVALID_INDEX, &gbvalue);

before the field within gbvalue is used.

Looking at gbcodec_mixer_dapm_ctl_get() defined just above that, it
seems that the call to gb_audio_gb_get_control() should be preceded
by a call to gb_pm_runtime_get_sync().  And given that duplication,
I suggest this call and the PM runtime wrapper functions should be
placed in a new helper function.

I know that Vaibhav said he would be fixing this, so I guess my
comments are directed at him.  Thanks for sending the patch Colin.

					-Alex


> Colin
> 
> 

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: issue with uninitialized value used in a comparison in gbcodec_mixer_dapm_ctl_put

[Vaibhav Agarwal]

On Wed, Aug 05, 2020 at 08:35:15AM -0500, Alex Elder wrote:

<snip>

> 
> I think the fix is to add a call to this:
> 
>         ret = gb_audio_gb_get_control(module->mgmt_connection, data->ctl_id,
>                                       GB_AUDIO_INVALID_INDEX, &gbvalue);
> 
> before the field within gbvalue is used.
> 
> Looking at gbcodec_mixer_dapm_ctl_get() defined just above that, it
> seems that the call to gb_audio_gb_get_control() should be preceded
> by a call to gb_pm_runtime_get_sync().  And given that duplication,
> I suggest this call and the PM runtime wrapper functions should be
> placed in a new helper function.
> 
> I know that Vaibhav said he would be fixing this, so I guess my
> comments are directed at him.  Thanks for sending the patch Colin.
> 
> 					-Alex

Thanks Alex. I'll share a patch with the proposed fix.

--
vaibhav

> 
> 
> > Colin
> > 
> > 
> 

Re: issue with uninitialized value used in a comparison in gbcodec_mixer_dapm_ctl_put

[Vaibhav Agarwal]

On Wed, Aug 05, 2020 at 08:35:15AM -0500, Alex Elder wrote:

<snip>

> 
> I think the fix is to add a call to this:
> 
>         ret = gb_audio_gb_get_control(module->mgmt_connection, data->ctl_id,
>                                       GB_AUDIO_INVALID_INDEX, &gbvalue);
> 
> before the field within gbvalue is used.
> 
> Looking at gbcodec_mixer_dapm_ctl_get() defined just above that, it
> seems that the call to gb_audio_gb_get_control() should be preceded
> by a call to gb_pm_runtime_get_sync().  And given that duplication,
> I suggest this call and the PM runtime wrapper functions should be
> placed in a new helper function.
> 
> I know that Vaibhav said he would be fixing this, so I guess my
> comments are directed at him.  Thanks for sending the patch Colin.
> 
> 					-Alex

Thanks Alex. I'll share a patch with the proposed fix.

--
vaibhav

> 
> 
> > Colin
> > 
> > 
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel