ALSA: hda: cs35l41: fix double free in cs35l41_hda_probe()
PATCH 1/1 ALSA: hda: cs35l41: fix double free in cs35l41_hda_probe()
From: Tom Rix <[email protected]>
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.
--- sound/pci/hda/cs35l41_hda.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
sound/pci/hda/cs35l41_hda.c +6 -6
@@ -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 idev_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; }
Signature · 1 line
2.26.3
Quoted 39 lines · On Saturday, January 8, 2022, <[email protected]> wrote:
From: Tom Rix <[email protected]>
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_cfgis 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 HDAsystems")
Signed-off-by Tom Rix <[email protected]>--- sound/pci/hda/cs35l41_hda.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)sound/pci/hda/cs35l41_hda.c +0
-1@@ -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,sound/pci/hda/cs35l41_hda.c +0
-0@@ -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);
sound/pci/hda/cs35l41_hda.c int cs35l41_hda_probe(struct device *dev, const char · line 502
Quoted 10 lines
sound/pci/hda/cs35l41_hda.c +2
-5-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.
Quoted 11 lines
sound/pci/hda/cs35l41_hda.c +4
-0+if (!cs35l41->vspk_always_on) +gpiod_set_value_cansleep (cs35l41->reset_gpio,0 ); +gpiod_put (cs35l41->reset_gpio); + }return ret;}
--
2.26.3
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 55 lines · Andy Shevchenko
On Saturday, January 8, 2022, <[email protected] <mailto:[email protected]>> wrote:
From: Tom Rix <[email protected] <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]>> --- 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)) {
Quoted 1 line
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.
Quoted 18 lines
+ if (!cs35l41->vspk_always_on) + gpiod_set_value_cansleep(cs35l41->reset_gpio, 0); + gpiod_put(cs35l41->reset_gpio); + } return ret; } -- 2.26.3Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 2 lines · Tom Rix
On 1/9/22 2:33 PM, Andy Shevchenko wrote:
On Saturday, January 8, 2022, <[email protected]> wrote:
...
Quoted 5 lines
Quoted 1 line
+ 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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 11 lines · Andy Shevchenko
Quoted 2 lines · Tom Rix
On 1/9/22 2:33 PM, Andy Shevchenko wrote:
On Saturday, January 8, 2022, <[email protected]> wrote:...
Quoted 4 lines
Quoted 1 line
+ 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.
Quoted 7 lines
Quoted 1 line
Unlikely to have an error.
We don't use
likely()andunlikely()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