Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1489930 > unrolled thread
| Started by | Colin King <colin.king@canonical.com> |
|---|---|
| First post | 2016-09-23 12:30 +0200 |
| Last post | 2016-09-23 20:30 +0200 |
| Articles | 4 — 4 participants |
Back to article view | Back to linux.kernel
[PATCH] greybus: audio: ensure module is set to avoid crash on dev_err message Colin King <colin.king@canonical.com> - 2016-09-23 12:30 +0200
Re: [PATCH] greybus: audio: ensure module is set to avoid crash on dev_err message Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-09-23 19:00 +0200
Re: [PATCH] greybus: audio: ensure module is set to avoid crash on dev_err message Vaibhav Agarwal <vaibhav.sr@gmail.com> - 2016-09-23 20:30 +0200
Re: [PATCH] greybus: audio: ensure module is set to avoid crash on dev_err message Colin Ian King <colin.king@canonical.com> - 2016-09-23 20:30 +0200
| From | Colin King <colin.king@canonical.com> |
|---|---|
| Date | 2016-09-23 12:30 +0200 |
| Subject | [PATCH] greybus: audio: ensure module is set to avoid crash on dev_err message |
| Message-ID | <skvTY-2pQ-19@gated-at.bofh.it> |
From: Colin Ian King <colin.king@canonical.com>
Currently, if info is null, the dev_err message is dereferencing an
uninitialized module pointer. Instead, initialize module before the
dev_err call to fix this issue.
Found using static analysis with cppcheck:
[drivers/staging/greybus/audio_topology.c:175]: (error)
Uninitialized variable: module
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/staging/greybus/audio_topology.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c
index 5eef536..c43a959 100644
--- a/drivers/staging/greybus/audio_topology.c
+++ b/drivers/staging/greybus/audio_topology.c
@@ -171,6 +171,9 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol *kcontrol,
data = (struct gbaudio_ctl_pvt *)kcontrol->private_value;
info = (struct gb_audio_ctl_elem_info *)data->info;
+ module = find_gb_module(gbcodec, kcontrol->id.name);
+ if (!module)
+ return -EINVAL;
if (!info) {
dev_err(module->dev, "NULL info for %s\n", uinfo->id.name);
return -EINVAL;
@@ -192,9 +195,6 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol *kcontrol,
uinfo->value.enumerated.items = max;
if (uinfo->value.enumerated.item > max - 1)
uinfo->value.enumerated.item = max - 1;
- module = find_gb_module(gbcodec, kcontrol->id.name);
- if (!module)
- return -EINVAL;
name = gbaudio_map_controlid(module, data->ctl_id,
uinfo->value.enumerated.item);
strlcpy(uinfo->value.enumerated.name, name, NAME_SIZE);
--
2.9.3
[toc] | [next] | [standalone]
| From | Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
|---|---|
| Date | 2016-09-23 19:00 +0200 |
| Subject | Re: [PATCH] greybus: audio: ensure module is set to avoid crash on dev_err message |
| Message-ID | <skBZo-6mq-27@gated-at.bofh.it> |
| In reply to | #1489930 |
On Fri, Sep 23, 2016 at 11:25:40AM +0100, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > Currently, if info is null, the dev_err message is dereferencing an > uninitialized module pointer. Instead, initialize module before the > dev_err call to fix this issue. > > Found using static analysis with cppcheck: > [drivers/staging/greybus/audio_topology.c:175]: (error) > Uninitialized variable: module > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/staging/greybus/audio_topology.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c > index 5eef536..c43a959 100644 > --- a/drivers/staging/greybus/audio_topology.c > +++ b/drivers/staging/greybus/audio_topology.c > @@ -171,6 +171,9 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol *kcontrol, > data = (struct gbaudio_ctl_pvt *)kcontrol->private_value; > info = (struct gb_audio_ctl_elem_info *)data->info; > > + module = find_gb_module(gbcodec, kcontrol->id.name); > + if (!module) > + return -EINVAL; How do you know you can get a module at this point in time, you haven't looked at the type of info to know that? I agree that there is an issue here, but I don't think this is the correct fix. Vaibhav?
[toc] | [prev] | [next] | [standalone]
| From | Vaibhav Agarwal <vaibhav.sr@gmail.com> |
|---|---|
| Date | 2016-09-23 20:30 +0200 |
| Subject | Re: [PATCH] greybus: audio: ensure module is set to avoid crash on dev_err message |
| Message-ID | <skDot-7lF-1@gated-at.bofh.it> |
| In reply to | #1490334 |
On Fri, Sep 23, 2016 at 10:28 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Fri, Sep 23, 2016 at 11:25:40AM +0100, Colin King wrote: >> From: Colin Ian King <colin.king@canonical.com> >> >> Currently, if info is null, the dev_err message is dereferencing an >> uninitialized module pointer. Instead, initialize module before the >> dev_err call to fix this issue. >> >> Found using static analysis with cppcheck: >> [drivers/staging/greybus/audio_topology.c:175]: (error) >> Uninitialized variable: module >> >> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> --- >> drivers/staging/greybus/audio_topology.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c >> index 5eef536..c43a959 100644 >> --- a/drivers/staging/greybus/audio_topology.c >> +++ b/drivers/staging/greybus/audio_topology.c >> @@ -171,6 +171,9 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol *kcontrol, >> data = (struct gbaudio_ctl_pvt *)kcontrol->private_value; >> info = (struct gb_audio_ctl_elem_info *)data->info; >> >> + module = find_gb_module(gbcodec, kcontrol->id.name); >> + if (!module) >> + return -EINVAL; > > How do you know you can get a module at this point in time, you haven't > looked at the type of info to know that? > > I agree that there is an issue here, but I don't think this is the > correct fix. Vaibhav? Actually, fetching module is not related to info->type. However it is only required in case of ENUMERATED element_type, thus used in specific case. Also, I think it's better to use codec->dev in the err message and align with other err_msg in this function. Hi Colin, thanks for sharing this patch. Do you mind updating this patch including my above suggestion? Otherwise, I can share a separate patch. -- thanks, vaibhav >
[toc] | [prev] | [next] | [standalone]
| From | Colin Ian King <colin.king@canonical.com> |
|---|---|
| Date | 2016-09-23 20:30 +0200 |
| Subject | Re: [PATCH] greybus: audio: ensure module is set to avoid crash on dev_err message |
| Message-ID | <skDou-7lF-29@gated-at.bofh.it> |
| In reply to | #1490372 |
On 23/09/16 19:20, Vaibhav Agarwal wrote: > On Fri, Sep 23, 2016 at 10:28 PM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: >> On Fri, Sep 23, 2016 at 11:25:40AM +0100, Colin King wrote: >>> From: Colin Ian King <colin.king@canonical.com> >>> >>> Currently, if info is null, the dev_err message is dereferencing an >>> uninitialized module pointer. Instead, initialize module before the >>> dev_err call to fix this issue. >>> >>> Found using static analysis with cppcheck: >>> [drivers/staging/greybus/audio_topology.c:175]: (error) >>> Uninitialized variable: module >>> >>> Signed-off-by: Colin Ian King <colin.king@canonical.com> >>> --- >>> drivers/staging/greybus/audio_topology.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/staging/greybus/audio_topology.c b/drivers/staging/greybus/audio_topology.c >>> index 5eef536..c43a959 100644 >>> --- a/drivers/staging/greybus/audio_topology.c >>> +++ b/drivers/staging/greybus/audio_topology.c >>> @@ -171,6 +171,9 @@ static int gbcodec_mixer_ctl_info(struct snd_kcontrol *kcontrol, >>> data = (struct gbaudio_ctl_pvt *)kcontrol->private_value; >>> info = (struct gb_audio_ctl_elem_info *)data->info; >>> >>> + module = find_gb_module(gbcodec, kcontrol->id.name); >>> + if (!module) >>> + return -EINVAL; >> >> How do you know you can get a module at this point in time, you haven't >> looked at the type of info to know that? >> >> I agree that there is an issue here, but I don't think this is the >> correct fix. Vaibhav? > > Actually, fetching module is not related to info->type. However it is > only required in case of ENUMERATED element_type, thus used in > specific case. Also, I think it's better to use codec->dev in the err > message and align with other err_msg in this function. > > Hi Colin, thanks for sharing this patch. Do you mind updating this > patch including my above suggestion? Otherwise, I can share a separate > patch. Vaibhav, I'm currently traveling, so won't be able to check this until next week, so if you would rather send a correct fix sooner rather me re-sending a fix then I'm fine with that. Thanks, Colin > > -- > thanks, > vaibhav >>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web