The current implementation for gbcodec_mixer_dapm_ctl_put() uses uninitialized gbvalue for comparison with updated value. This was found using static analysis with coverity.
Uninitialized scalar variable (UNINIT) 11. uninit_use: Using uninitialized value gbvalue.value.integer_value[0]. 460 if (gbvalue.value.integer_value[0] != val) {
This patch fixes the issue with fetching the gbvalue before using it for comparision.
Fixes: 6339d2322c47 ("greybus: audio: Add topology parser for GB codec") Reported-by: Colin Ian King colin.king@canonical.com Signed-off-by: Vaibhav Agarwal vaibhav.sr@gmail.com --- drivers/staging/greybus/audio_topology.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c index 2f9fdbdcd547..4b914d0edef2 100644 --- a/drivers/staging/greybus/audio_topology.c +++ b/drivers/staging/greybus/audio_topology.c @@ -456,6 +456,13 @@ static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol, val = ucontrol->value.integer.value[0] & mask; connect = !!val;
+ ret = gb_pm_runtime_get_sync(bundle); + if (ret) + return ret; + + ret = gb_audio_gb_get_control(module->mgmt_connection, data->ctl_id, + GB_AUDIO_INVALID_INDEX, &gbvalue); + /* update ucontrol */ if (gbvalue.value.integer_value[0] != val) { for (wi = 0; wi < wlist->num_widgets; wi++) { @@ -466,16 +473,10 @@ static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol, gbvalue.value.integer_value[0] = cpu_to_le32(ucontrol->value.integer.value[0]);
- ret = gb_pm_runtime_get_sync(bundle); - if (ret) - return ret; - ret = gb_audio_gb_set_control(module->mgmt_connection, data->ctl_id, GB_AUDIO_INVALID_INDEX, &gbvalue);
- gb_pm_runtime_put_autosuspend(bundle); - if (ret) { dev_err_ratelimited(codec_dev, "%d:Error in %s for %s\n", ret, @@ -483,6 +484,7 @@ static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol, return ret; } } + gb_pm_runtime_put_autosuspend(bundle);
return 0; }
base-commit: 5bbd90550da8f7bdac769b5825597e67183c9411 prerequisite-patch-id: 2b8901339222ff7b94f10cf2341734c0fb82591c prerequisite-patch-id: 38dad8879a2b73bce6e89481973c7c5b82bd7145 prerequisite-patch-id: 5f0042ccedae292395ec617789be6bf465463c1c prerequisite-patch-id: 35d001c366dfa4b567e59abbb37bd691a18f5e14 prerequisite-patch-id: f13ce918ebc3796cd3c81716a7b2adf4519e7387 prerequisite-patch-id: 0fcc6d38699a9b72ca94280d7a4dc18f0823b6f7 prerequisite-patch-id: 8074e935bdc3dd7b114245b0648552d0ff6871c9
On Thu, Aug 06, 2020 at 09:51:57PM +0530, Vaibhav Agarwal wrote:
diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c index 2f9fdbdcd547..4b914d0edef2 100644 --- a/drivers/staging/greybus/audio_topology.c +++ b/drivers/staging/greybus/audio_topology.c @@ -456,6 +456,13 @@ static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol, val = ucontrol->value.integer.value[0] & mask; connect = !!val;
- ret = gb_pm_runtime_get_sync(bundle);
- if (ret)
return ret;
- ret = gb_audio_gb_get_control(module->mgmt_connection, data->ctl_id,
GB_AUDIO_INVALID_INDEX, &gbvalue);
We need to check "ret" after this.
- /* update ucontrol */ if (gbvalue.value.integer_value[0] != val) { for (wi = 0; wi < wlist->num_widgets; wi++) {
@@ -466,16 +473,10 @@ static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol,
regards, dan carpenter
On Mon, Aug 10, 2020 at 8:34 PM Dan Carpenter dan.carpenter@oracle.com wrote:
On Thu, Aug 06, 2020 at 09:51:57PM +0530, Vaibhav Agarwal wrote:
diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c index 2f9fdbdcd547..4b914d0edef2 100644 --- a/drivers/staging/greybus/audio_topology.c +++ b/drivers/staging/greybus/audio_topology.c @@ -456,6 +456,13 @@ static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol, val = ucontrol->value.integer.value[0] & mask; connect = !!val;
ret = gb_pm_runtime_get_sync(bundle);
if (ret)
return ret;
ret = gb_audio_gb_get_control(module->mgmt_connection, data->ctl_id,
GB_AUDIO_INVALID_INDEX, &gbvalue);
We need to check "ret" after this.
Oops, my bad. Thanks Dan for reporting this. I'll share an updated patch soon.
-- regards, vaibhav
/* update ucontrol */ if (gbvalue.value.integer_value[0] != val) { for (wi = 0; wi < wlist->num_widgets; wi++) {
@@ -466,16 +473,10 @@ static int gbcodec_mixer_dapm_ctl_put(struct snd_kcontrol *kcontrol,
regards, dan carpenter