| Message ID | 20220108140756.3985487-1-trix@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: hda: cs35l41: fix double free in cs35l41_hda_probe() | expand |
On Saturday, January 8, 2022, <trix@redhat.com> wrote: > From: Tom Rix <trix@redhat.com> > > Clang static analysis reports this problem > cs35l41_hda.c:501:2: warning: Attempt to free released memory > kfree(acpi_hw_cfg); > ^~~~~~~~~~~~~~~~~~ > > This second free happens in the function's error handler which > is normally ok but acpi_hw_cfg is freed in the non error case > when it is still possible to have an error. > > Consolidate the frees. > > Fixes: 7b2f3eb492da ("ALSA: hda: cs35l41: Add support for CS35L41 in HDA > systems") > Signed-off-by: Tom Rix <trix@redhat.com> > --- > sound/pci/hda/cs35l41_hda.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c > index aa5bb6977792c..265ace98965f5 100644 > --- a/sound/pci/hda/cs35l41_hda.c > +++ b/sound/pci/hda/cs35l41_hda.c > @@ -476,7 +476,6 @@ int cs35l41_hda_probe(struct device *dev, const char > *device_name, int id, int i > ret = cs35l41_hda_apply_properties(cs35l41, acpi_hw_cfg); > if (ret) > goto err; > - kfree(acpi_hw_cfg); > > if (cs35l41->reg_seq->probe) { > ret = regmap_register_patch(cs35l41->regmap, > cs35l41->reg_seq->probe, > @@ -495,13 +494,14 @@ int cs35l41_hda_probe(struct device *dev, const char > *device_name, int id, int i > > dev_info(cs35l41->dev, "Cirrus Logic CS35L41 (%x), Revision: > %02X\n", regid, reg_revid); > > - return 0; > - > err: > kfree(acpi_hw_cfg); > - if (!cs35l41->vspk_always_on) > - gpiod_set_value_cansleep(cs35l41->reset_gpio, 0); > - gpiod_put(cs35l41->reset_gpio); > + > + if (unlikely(ret)) { This is double weird. First of all, wtf unlikely is here? Second, I commented on the patch that does something with this driver and pointed out to the return 0 in some cases. This one seems a band aid. > + if (!cs35l41->vspk_always_on) > + gpiod_set_value_cansleep(cs35l41->reset_gpio, 0); > + gpiod_put(cs35l41->reset_gpio); > + } > > return ret; > } > -- > 2.26.3 > >
On 1/9/22 2:33 PM, Andy Shevchenko wrote: > > > On Saturday, January 8, 2022, <trix@redhat.com > <mailto:trix@redhat.com>> wrote: > > From: Tom Rix <trix@redhat.com <mailto:trix@redhat.com>> > > Clang static analysis reports this problem > cs35l41_hda.c:501:2: warning: Attempt to free released memory > kfree(acpi_hw_cfg); > ^~~~~~~~~~~~~~~~~~ > > This second free happens in the function's error handler which > is normally ok but acpi_hw_cfg is freed in the non error case > when it is still possible to have an error. > > Consolidate the frees. > > Fixes: 7b2f3eb492da ("ALSA: hda: cs35l41: Add support for CS35L41 > in HDA systems") > Signed-off-by: Tom Rix <trix@redhat.com <mailto:trix@redhat.com>> > --- > sound/pci/hda/cs35l41_hda.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c > index aa5bb6977792c..265ace98965f5 100644 > --- a/sound/pci/hda/cs35l41_hda.c > +++ b/sound/pci/hda/cs35l41_hda.c > @@ -476,7 +476,6 @@ int cs35l41_hda_probe(struct device *dev, > const char *device_name, int id, int i > ret = cs35l41_hda_apply_properties(cs35l41, acpi_hw_cfg); > if (ret) > goto err; > - kfree(acpi_hw_cfg); > > if (cs35l41->reg_seq->probe) { > ret = regmap_register_patch(cs35l41->regmap, > cs35l41->reg_seq->probe, > @@ -495,13 +494,14 @@ int cs35l41_hda_probe(struct device *dev, > const char *device_name, int id, int i > > dev_info(cs35l41->dev, "Cirrus Logic CS35L41 (%x), > Revision: %02X\n", regid, reg_revid); > > - return 0; > - > err: > kfree(acpi_hw_cfg); > - if (!cs35l41->vspk_always_on) > - gpiod_set_value_cansleep(cs35l41->reset_gpio, 0); > - gpiod_put(cs35l41->reset_gpio); > + > + if (unlikely(ret)) { > > > This is double weird. First of all, wtf unlikely is here? Second, I > commented on the patch that does something with this driver and > pointed out to the return 0 in some cases. This one seems a band aid. Unlikely to have an error. > + if (!cs35l41->vspk_always_on) > + > gpiod_set_value_cansleep(cs35l41->reset_gpio, 0); > + gpiod_put(cs35l41->reset_gpio); > + } > > return ret; > } > -- > 2.26.3 > > > > -- > With Best Regards, > Andy Shevchenko > >
On Mon, Jan 10, 2022 at 2:37 AM Tom Rix <trix@redhat.com> wrote: > On 1/9/22 2:33 PM, Andy Shevchenko wrote: > On Saturday, January 8, 2022, <trix@redhat.com> wrote: ... >> + if (unlikely(ret)) { > > This is double weird. First of all, wtf unlikely is here? Second, I commented on the patch that does something with this driver and pointed out to the return 0 in some cases. This one seems a band aid. > > Unlikely to have an error. We don't use likely() and unlikely() here and there, you need to provide a very good justification of its use. For the record, I forwarded you my review against the code where you can find much more issues with it that are subject to fix / amend.
On Mon, 10 Jan 2022 11:21:11 +0100, Andy Shevchenko wrote: > > On Mon, Jan 10, 2022 at 2:37 AM Tom Rix <trix@redhat.com> wrote: > > On 1/9/22 2:33 PM, Andy Shevchenko wrote: > > On Saturday, January 8, 2022, <trix@redhat.com> wrote: > > ... > > >> + if (unlikely(ret)) { > > > > This is double weird. First of all, wtf unlikely is here? Second, I commented on the patch that does something with this driver and pointed out to the return 0 in some cases. This one seems a band aid. > > > > Unlikely to have an error. > > We don't use likely() and unlikely() here and there, you need to > provide a very good justification of its use. > > For the record, I forwarded you my review against the code where you > can find much more issues with it that are subject to fix / amend. For this particular bug fix, Dan submitted a simpler patch and I took it now: https://lore.kernel.org/r/20220111072232.GG11243@kili thanks, Takashi
diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c index aa5bb6977792c..265ace98965f5 100644 --- a/sound/pci/hda/cs35l41_hda.c +++ b/sound/pci/hda/cs35l41_hda.c @@ -476,7 +476,6 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i ret = cs35l41_hda_apply_properties(cs35l41, acpi_hw_cfg); if (ret) goto err; - kfree(acpi_hw_cfg); if (cs35l41->reg_seq->probe) { ret = regmap_register_patch(cs35l41->regmap, cs35l41->reg_seq->probe, @@ -495,13 +494,14 @@ int cs35l41_hda_probe(struct device *dev, const char *device_name, int id, int i dev_info(cs35l41->dev, "Cirrus Logic CS35L41 (%x), Revision: %02X\n", regid, reg_revid); - return 0; - err: kfree(acpi_hw_cfg); - if (!cs35l41->vspk_always_on) - gpiod_set_value_cansleep(cs35l41->reset_gpio, 0); - gpiod_put(cs35l41->reset_gpio); + + if (unlikely(ret)) { + if (!cs35l41->vspk_always_on) + gpiod_set_value_cansleep(cs35l41->reset_gpio, 0); + gpiod_put(cs35l41->reset_gpio); + } return ret; }