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@xxxxxxxxxx>
> 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
>
>
_______________________________________________
greybus-dev mailing list
greybus-dev@xxxxxxxxxxxxxxxx
https://lists.linaro.org/mailman/listinfo/greybus-dev
|