linux-kernel - Re: issue with uninitialized value used in a comparison in gbcodec_mixer_dapm_ctl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <07ad3809-de73-9a66-0e4f-3a49f395a98a@ieee.org>
Date:   Wed, 5 Aug 2020 08:35:15 -0500
From:   Alex Elder <elder@...e.org>
To:     Colin Ian King <colin.king@...onical.com>,
        Johan Hovold <johan@...nel.org>, Alex Elder <elder@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Vaibhav Agarwal <vaibhav.sr@...il.com>,
        Mark Greer <mgreer@...malcreek.com>,
        greybus-dev@...ts.linaro.org,
        "devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: issue with uninitialized value used in a comparison in
 gbcodec_mixer_dapm_ctl_put

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@...aro.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
> 
>