Messages in this thread | | | Subject | Re: [PATCH v2] platform/x86: think-lmi: Fix memory leak when showing current settings | From | Armin Wolf <> | Date | Fri, 31 Mar 2023 23:26:49 +0200 |
| |
Am 31.03.23 um 21:34 schrieb Mirsad Goran Todorovac:
> On 31. 03. 2023. 20:09, Armin Wolf wrote: >> When retriving a item string with tlmi_setting(), the result has to be >> freed using kfree(). In current_value_show() however, malformed >> item strings are not freed, causing a memory leak. >> Fix this by eliminating the early return responsible for this. >> >> Reported-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr> >> Link: https://lore.kernel.org/platform-driver-x86/01e920bc-5882-ba0c-dd15-868bf0eca0b8@alu.unizg.hr/T/#t >> Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms") >> Signed-off-by: Armin Wolf <W_Armin@gmx.de> >> --- >> Changes in v2: >> - Add Reported-by: and Link: tags >> --- >> drivers/platform/x86/think-lmi.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c >> index cc66f7cbccf2..8cafb9d4016c 100644 >> --- a/drivers/platform/x86/think-lmi.c >> +++ b/drivers/platform/x86/think-lmi.c >> @@ -930,10 +930,12 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a >> /* validate and split from `item,value` -> `value` */ >> value = strpbrk(item, ","); >> if (!value || value == item || !strlen(value + 1)) >> - return -EINVAL; >> + ret = -EINVAL; >> + else >> + ret = sysfs_emit(buf, "%s\n", value + 1); >> >> - ret = sysfs_emit(buf, "%s\n", value + 1); >> kfree(item); >> + >> return ret; >> } > Hi, Armin, > > You might have wanted it to be tested in the original setting? > > Should this patch work as a standalone fix, without the others? > > This part: > > @@ -929,8 +929,10 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a > > /* validate and split from `item,value` -> `value` */ > value = strpbrk(item, ","); > - if (!value || value == item || !strlen(value + 1)) > + if (!value || value == item || !strlen(value + 1)) { > + kfree(item); > return -EINVAL; > + } > > ret = sysfs_emit(buf, "%s\n", value + 1); > kfree(item); > > was apparently superseded.
Hi,
this part is indeed superseded by the patch, and it should work as a standalone fix. I thought it might be better to have two patches for those two memory leaks, as they are not directly connected.
> Should this one be applied? I guess it should, as I stated in email > <4dc118c2-0dde-bd5e-ea41-427ed33e4545@alu.unizg.hr> from 2023-03-29 20:49 UTC+02: > > @@ -1457,10 +1458,10 @@ static int tlmi_analyze(void) > * name string. > * Try and pull that out if it's available. > */ > - char *item, *optstart, *optend; > + char *optitem, *optstart, *optend; > > - if (!tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID)) { > - optstart = strstr(item, "[Optional:"); > + if (!tlmi_setting(setting->index, &optitem, LENOVO_BIOS_SETTING_GUID)) { > + optstart = strstr(optitem, "[Optional:"); > if (optstart) { > optstart += strlen("[Optional:"); > optend = strstr(optstart, "]"); > @@ -1469,6 +1470,7 @@ static int tlmi_analyze(void) > kstrndup(optstart, optend - optstart, > GFP_KERNEL); > } > + kfree(optitem); > } > } > /* > > If Mark had found a better fix, then that one goes away, too. > > NOTE PLEASE that in the above-mentioned message (like all the others) I just specified the > commit at which the test kernel was built + all the applied patches (git diff did not give > authors). > > This did not imply that I claim Mr. Weißschuh's fix for tlmi_analyze() return, God forbid! > I apologise if I made room for such an impression. > > That's all, I think. Thank Heavens. God bless! > > I will assume the test build on the bottom patch + the Thomas's patch still apply + your patch.
All good.
Armin Wolf
> Best regards, > Mirsad >
| |