Messages in this thread Patch in this message | | | Date | Wed, 29 Mar 2023 20:49:27 +0200 | Subject | Re: [BUG] [RFC] systemd-devd triggers kernel memleak apparently in drivers/core/dd.c: driver_register() | From | Mirsad Goran Todorovac <> |
| |
On 29.3.2023. 18:24, Mark Pearson wrote: > Hi > > On Wed, Mar 29, 2023, at 11:46 AM, Hans de Goede wrote: >> Hi, >> >> On 3/29/23 16:18, Mirsad Goran Todorovac wrote: >>> On 29.3.2023. 15:35, Thomas Weißschuh wrote: >>>> >>>> Mar 29, 2023 08:31:31 Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>: >>>> > <snip> >>> >>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c >>> index c816646eb661..e8c28f4f5a71 100644 >>> --- a/drivers/platform/x86/think-lmi.c >>> +++ b/drivers/platform/x86/think-lmi.c >>> @@ -1469,6 +1469,7 @@ static int tlmi_analyze(void) >>> kstrndup(optstart, optend - optstart, >>> GFP_KERNEL); >>> } >>> + kfree(item); >>> } >>> } >>> /* >>> >>> You were 3 minutes faster ;-) >>> >>> The build with this patch is finished. Apparently, that was the culprit, for now > <snip> >>> >>> >>> So, the "tlmi_setting" memory leak appears to be fixed by this diff. >>> > My only concern here is it looks like I was dumb and used the variable name 'item' twice in the same function. I guess the compiler is smart enough to handle it but I'd like to change the name to make it clearer which 'item' is being freed in each context. > > In that block I would change it to be: > char *optitem, *optstart, *optend; > and fix all the pieces in the block to then be correct too (with the needed free) > >>> The next step is to add Armin-suggested patch: >>> >>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c >>> index c816646eb661..1e77ecb0cba8 100644 >>> --- a/drivers/platform/x86/think-lmi.c >>> +++ b/drivers/platform/x86/think-lmi.c >>> @@ -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); >>> > This looks good to me - thank you! > >>> and Thomas' correction for the return type of the tlmi_setting() function: >>> >>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c >>> index 86b33b74519be..c924e9e4a6a5b 100644 >>> --- a/drivers/platform/x86/think-lmi.c >>> +++ b/drivers/platform/x86/think-lmi.c >>> @@ -1353,7 +1353,6 @@ static struct tlmi_pwd_setting *tlmi_create_auth(const char *pwd_type, >>> >>> static int tlmi_analyze(void) >>> { >>> - acpi_status status; >>> int i, ret; >>> >>> if (wmi_has_guid(LENOVO_SET_BIOS_SETTINGS_GUID) && >>> @@ -1390,8 +1389,8 @@ static int tlmi_analyze(void) >>> char *p; >>> >>> tlmi_priv.setting[i] = NULL; >>> - status = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID); >>> - if (ACPI_FAILURE(status)) >>> + ret = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID); >>> + if (ret) >>> break; >>> if (!item) >>> break; >>> >>> A build on top of 6.3-rc4+ fcd476ea6a88 commit is on the way, with all three included. >> >> Good work on catching these issues, thank you all for your work on this. >> > Seconded - thank you for flagging and catching this. These were my mistakes :( > >> I assume that these fixes will be posted as a proper 3 patch >> patch-series (one patch per fix) once you are done testing? >> > Let me know if you are happy to propose the changes as a patch-series. If you don't have time I can help and get these in ASAP as I was the original culprit. > > Happy to help out with testing too as I have access to HW. Let me know. > > Mark
Here is the patch proposal according to what Mark advised (using different name for optitem):
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c index c816646eb661..ab17254781c4 100644 --- a/drivers/platform/x86/think-lmi.c +++ b/drivers/platform/x86/think-lmi.c @@ -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); @@ -1380,7 +1382,6 @@ static struct tlmi_pwd_setting *tlmi_create_auth(const char *pwd_type,
static int tlmi_analyze(void) { - acpi_status status; int i, ret;
if (wmi_has_guid(LENOVO_SET_BIOS_SETTINGS_GUID) && @@ -1417,8 +1418,8 @@ static int tlmi_analyze(void) char *p;
tlmi_priv.setting[i] = NULL; - status = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID); - if (ACPI_FAILURE(status)) + ret = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID); + if (ret) break; if (!item) break; @@ -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); } } /* I have tested it, but without a few blunders of my own. I guess "nobody wins them all".
Best regards, Mirsad
-- Mirsad Todorovac System engineer Faculty of Graphic Arts | Academy of Fine Arts University of Zagreb Republic of Croatia, the European Union Sistem inženjer Grafički fakultet | Akademija likovnih umjetnosti Sveučilište u Zagrebu
| |