| Message ID | 20230923204154.86815-1-W_Armin@gmx.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Hans de Goede |
| Headers | show |
| Series | platform/x86: think-lmi: Fix reference leak | expand |
On Sat, 23 Sep 2023, Armin Wolf wrote: > If a duplicate attribute is found using kset_find_obj(), a reference > to that attribute is returned which needs to be disposed accordingly > using kobject_put(). Move the setting name validation into a separate > function to allow for this change without having to duplicate the > cleanup code for this setting. > As a side note, a very similar bug was fixed in > commit 7295a996fdab ("platform/x86: dell-sysman: Fix reference leak"), > so it seems that the bug was copied from that driver. > > Compile-tested only. > > Fixes: 1bcad8e510b2 ("platform/x86: think-lmi: Fix issues with duplicate attributes") > Signed-off-by: Armin Wolf <W_Armin@gmx.de> > --- > drivers/platform/x86/think-lmi.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c > index 4be6f28d4600..3a396b763c49 100644 > --- a/drivers/platform/x86/think-lmi.c > +++ b/drivers/platform/x86/think-lmi.c > @@ -1344,6 +1344,24 @@ static void tlmi_release_attr(void) > kset_unregister(tlmi_priv.authentication_kset); > } > > +static int tlmi_validate_setting_name(struct kset *attribute_kset, char *name) > +{ > + struct kobject *duplicate; > + > + if (!strcmp(name, "Reserved")) > + return -EINVAL; > + > + duplicate = kset_find_obj(attribute_kset, name); > + if (duplicate) { > + pr_debug("Duplicate attribute name found - %s\n", name); > + /* kset_find_obj() returns a reference */ > + kobject_put(duplicate); > + return -EBUSY; > + } > + > + return 0; > +} > + > static int tlmi_sysfs_init(void) > { > int i, ret; > @@ -1372,10 +1390,8 @@ static int tlmi_sysfs_init(void) > continue; > > /* check for duplicate or reserved values */ > - if (kset_find_obj(tlmi_priv.attribute_kset, tlmi_priv.setting[i]->display_name) || > - !strcmp(tlmi_priv.setting[i]->display_name, "Reserved")) { > - pr_debug("duplicate or reserved attribute name found - %s\n", > - tlmi_priv.setting[i]->display_name); > + if (tlmi_validate_setting_name(tlmi_priv.attribute_kset, > + tlmi_priv.setting[i]->display_name) < 0) { > kfree(tlmi_priv.setting[i]->possible_values); > kfree(tlmi_priv.setting[i]); > tlmi_priv.setting[i] = NULL; Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> There seem to be two more of these in hp-bioscfg.
Thanks Armin, On 9/23/23 16:41, Armin Wolf wrote: > If a duplicate attribute is found using kset_find_obj(), a reference > to that attribute is returned which needs to be disposed accordingly > using kobject_put(). Move the setting name validation into a separate > function to allow for this change without having to duplicate the > cleanup code for this setting. > As a side note, a very similar bug was fixed in > commit 7295a996fdab ("platform/x86: dell-sysman: Fix reference leak"), > so it seems that the bug was copied from that driver. > > Compile-tested only. > > Fixes: 1bcad8e510b2 ("platform/x86: think-lmi: Fix issues with duplicate attributes") > Signed-off-by: Armin Wolf <W_Armin@gmx.de> > --- > drivers/platform/x86/think-lmi.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c > index 4be6f28d4600..3a396b763c49 100644 > --- a/drivers/platform/x86/think-lmi.c > +++ b/drivers/platform/x86/think-lmi.c > @@ -1344,6 +1344,24 @@ static void tlmi_release_attr(void) > kset_unregister(tlmi_priv.authentication_kset); > } > > +static int tlmi_validate_setting_name(struct kset *attribute_kset, char *name) > +{ > + struct kobject *duplicate; > + > + if (!strcmp(name, "Reserved")) > + return -EINVAL; > + > + duplicate = kset_find_obj(attribute_kset, name); > + if (duplicate) { > + pr_debug("Duplicate attribute name found - %s\n", name); > + /* kset_find_obj() returns a reference */ > + kobject_put(duplicate); > + return -EBUSY; > + } > + > + return 0; > +} > + > static int tlmi_sysfs_init(void) > { > int i, ret; > @@ -1372,10 +1390,8 @@ static int tlmi_sysfs_init(void) > continue; > > /* check for duplicate or reserved values */ > - if (kset_find_obj(tlmi_priv.attribute_kset, tlmi_priv.setting[i]->display_name) || > - !strcmp(tlmi_priv.setting[i]->display_name, "Reserved")) { > - pr_debug("duplicate or reserved attribute name found - %s\n", > - tlmi_priv.setting[i]->display_name); > + if (tlmi_validate_setting_name(tlmi_priv.attribute_kset, > + tlmi_priv.setting[i]->display_name) < 0) { > kfree(tlmi_priv.setting[i]->possible_values); > kfree(tlmi_priv.setting[i]); > tlmi_priv.setting[i] = NULL; > -- > 2.39.2 > Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca> Do you have any reports of our platforms where these are seen? If so I'd like to get it fixed in FW too (and I can get it tested on HW if that helps) Mark
Am 25.09.23 um 16:00 schrieb Mark Pearson: > Thanks Armin, > > On 9/23/23 16:41, Armin Wolf wrote: >> If a duplicate attribute is found using kset_find_obj(), a reference >> to that attribute is returned which needs to be disposed accordingly >> using kobject_put(). Move the setting name validation into a separate >> function to allow for this change without having to duplicate the >> cleanup code for this setting. >> As a side note, a very similar bug was fixed in >> commit 7295a996fdab ("platform/x86: dell-sysman: Fix reference leak"), >> so it seems that the bug was copied from that driver. >> >> Compile-tested only. >> >> Fixes: 1bcad8e510b2 ("platform/x86: think-lmi: Fix issues with >> duplicate attributes") >> Signed-off-by: Armin Wolf <W_Armin@gmx.de> >> --- >> drivers/platform/x86/think-lmi.c | 24 ++++++++++++++++++++---- >> 1 file changed, 20 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/platform/x86/think-lmi.c >> b/drivers/platform/x86/think-lmi.c >> index 4be6f28d4600..3a396b763c49 100644 >> --- a/drivers/platform/x86/think-lmi.c >> +++ b/drivers/platform/x86/think-lmi.c >> @@ -1344,6 +1344,24 @@ static void tlmi_release_attr(void) >> kset_unregister(tlmi_priv.authentication_kset); >> } >> >> +static int tlmi_validate_setting_name(struct kset *attribute_kset, >> char *name) >> +{ >> + struct kobject *duplicate; >> + >> + if (!strcmp(name, "Reserved")) >> + return -EINVAL; >> + >> + duplicate = kset_find_obj(attribute_kset, name); >> + if (duplicate) { >> + pr_debug("Duplicate attribute name found - %s\n", name); >> + /* kset_find_obj() returns a reference */ >> + kobject_put(duplicate); >> + return -EBUSY; >> + } >> + >> + return 0; >> +} >> + >> static int tlmi_sysfs_init(void) >> { >> int i, ret; >> @@ -1372,10 +1390,8 @@ static int tlmi_sysfs_init(void) >> continue; >> >> /* check for duplicate or reserved values */ >> - if (kset_find_obj(tlmi_priv.attribute_kset, >> tlmi_priv.setting[i]->display_name) || >> - !strcmp(tlmi_priv.setting[i]->display_name, "Reserved")) { >> - pr_debug("duplicate or reserved attribute name found - >> %s\n", >> - tlmi_priv.setting[i]->display_name); >> + if (tlmi_validate_setting_name(tlmi_priv.attribute_kset, >> + tlmi_priv.setting[i]->display_name) < 0) { >> kfree(tlmi_priv.setting[i]->possible_values); >> kfree(tlmi_priv.setting[i]); >> tlmi_priv.setting[i] = NULL; >> -- >> 2.39.2 >> > > Reviewed-by: Mark Pearson <mpearson-lenovo@squebb.ca> > > Do you have any reports of our platforms where these are seen? If so > I'd like to get it fixed in FW too (and I can get it tested on HW if > that helps) > > Mark > No, i do not have any reports regarding this issue. I stumbled upon this bug by chance, after fixing a similar bug in dell-wmi-sysman. I suspect that the bug was copied from there to the other drivers, like think-lmi and hp-bioscfg. Armin Wolf
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c index 4be6f28d4600..3a396b763c49 100644 --- a/drivers/platform/x86/think-lmi.c +++ b/drivers/platform/x86/think-lmi.c @@ -1344,6 +1344,24 @@ static void tlmi_release_attr(void) kset_unregister(tlmi_priv.authentication_kset); } +static int tlmi_validate_setting_name(struct kset *attribute_kset, char *name) +{ + struct kobject *duplicate; + + if (!strcmp(name, "Reserved")) + return -EINVAL; + + duplicate = kset_find_obj(attribute_kset, name); + if (duplicate) { + pr_debug("Duplicate attribute name found - %s\n", name); + /* kset_find_obj() returns a reference */ + kobject_put(duplicate); + return -EBUSY; + } + + return 0; +} + static int tlmi_sysfs_init(void) { int i, ret; @@ -1372,10 +1390,8 @@ static int tlmi_sysfs_init(void) continue; /* check for duplicate or reserved values */ - if (kset_find_obj(tlmi_priv.attribute_kset, tlmi_priv.setting[i]->display_name) || - !strcmp(tlmi_priv.setting[i]->display_name, "Reserved")) { - pr_debug("duplicate or reserved attribute name found - %s\n", - tlmi_priv.setting[i]->display_name); + if (tlmi_validate_setting_name(tlmi_priv.attribute_kset, + tlmi_priv.setting[i]->display_name) < 0) { kfree(tlmi_priv.setting[i]->possible_values); kfree(tlmi_priv.setting[i]); tlmi_priv.setting[i] = NULL;
If a duplicate attribute is found using kset_find_obj(), a reference to that attribute is returned which needs to be disposed accordingly using kobject_put(). Move the setting name validation into a separate function to allow for this change without having to duplicate the cleanup code for this setting. As a side note, a very similar bug was fixed in commit 7295a996fdab ("platform/x86: dell-sysman: Fix reference leak"), so it seems that the bug was copied from that driver. Compile-tested only. Fixes: 1bcad8e510b2 ("platform/x86: think-lmi: Fix issues with duplicate attributes") Signed-off-by: Armin Wolf <W_Armin@gmx.de> --- drivers/platform/x86/think-lmi.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) -- 2.39.2