Messages in this thread | | | Date | Wed, 29 Mar 2023 18:43:22 +0200 | Subject | Re: [BUG] [BISECTED] [CORRECTION] 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 :(
Armin's hint was the largest part of the catch. I did not even suspect think-lmi.c, to be honest ...
Apparently, the three patches together do not raise any new issues on my box, but of course, proper testing and peer review is required.
Best regards, Mirsad
>> 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
-- 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
| |