Messages in this thread | | | Subject | Re: [PATCH] platform/x86: dell-wmi-sysman: fix a NULL pointer dereference | From | Perry Yuan <> | Date | Mon, 1 Feb 2021 10:36:21 +0800 |
| |
On 2021/2/1 4:59, Hans de Goede wrote: > Hi, > > On 1/31/21 3:04 PM, Limonciello, Mario wrote: >> >> >>> -----Original Message----- >>> From: Hans de Goede <hdegoede@redhat.com> >>> Sent: Saturday, January 30, 2021 15:44 >>> To: Limonciello, Mario; Mark Gross >>> Cc: LKML; platform-driver-x86@vger.kernel.org >>> Subject: Re: [PATCH] platform/x86: dell-wmi-sysman: fix a NULL pointer >>> dereference >>> >>> >>> [EXTERNAL EMAIL] >>> >>> Hi, >>> >>> On 1/29/21 6:26 PM, Mario Limonciello wrote: >>>> An upcoming Dell platform is causing a NULL pointer dereference >>>> in dell-wmi-sysman initialization. Validate that the input from >>>> BIOS matches correct ACPI types and abort module initialization >>>> if it fails. >>>> >>>> This leads to a memory leak that needs to be cleaned up properly. >>>> >>>> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> >>>> --- >>>> drivers/platform/x86/dell-wmi-sysman/sysman.c | 8 +++++++- >>>> 1 file changed, 7 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/platform/x86/dell-wmi-sysman/sysman.c >>> b/drivers/platform/x86/dell-wmi-sysman/sysman.c >>>> index dc6dd531c996..38b497991071 100644 >>>> --- a/drivers/platform/x86/dell-wmi-sysman/sysman.c >>>> +++ b/drivers/platform/x86/dell-wmi-sysman/sysman.c >>>> @@ -419,13 +419,19 @@ static int init_bios_attributes(int attr_type, const >>> char *guid) >>>> return retval; >>>> /* need to use specific instance_id and guid combination to get right >>> data */ >>>> obj = get_wmiobj_pointer(instance_id, guid); >>>> - if (!obj) >>>> + if (!obj || obj->type != ACPI_TYPE_PACKAGE) { >>>> + release_attributes_data(); >>> >>> All calls of init_bios_attributes() have the following error handling: >>> >>> ret = init_bios_attributes(INT, DELL_WMI_BIOS_INTEGER_ATTRIBUTE_GUID); >>> if (ret) { >>> pr_debug("failed to populate integer type attributes\n"); >>> goto fail_create_group; >>> } >>> >>> ... >>> >>> fail_create_group: >>> release_attributes_data(); >>> >>> So that added release_attributes_data() call is not necessary. If you can >>> respin >>> this patch Monday with the release_attributes_data(); addition dropped, then >>> I will try to get this to Linus in time for 5.11 . >>> >>> Or I can fix this up locally if you agree with dropping the unnecessary >>> release_attributes_data() call. >>> >> >> Yes, please go ahead and drop the unnecessary call locally. > > Ok, I've merged this into my review-hans branch and I will push this out > to for-next as soon as a local build has finished. I'll also include > this in my last fixes pull-req for Linus later this week. > > While working on this I did notice that the function in question still > has a bunch of further issues. But since this patch fixes a crash and has > been tested I've decided to move forward with it as is (with the duplicate > release_attributes_data() call dropped). The further issues can be fixed > with follow-up patches. > > So the other issues which I noticed are: > > 1. Calling release_attributes_data() in the error-path here: > > obj = get_wmiobj_pointer(instance_id, guid); > if (!obj || obj->type != ACPI_TYPE_PACKAGE) { > return -ENODEV; > } > > Is not necessary as discussed, but the added obj->type != ACPI_TYPE_PACKAGE > which I assume triggers to fix the reported crash, means that obj is not > NULL in which case we should free it. So this should become: > > obj = get_wmiobj_pointer(instance_id, guid); > if (!obj || obj->type != ACPI_TYPE_PACKAGE) { > kfree(obj); > return -ENODEV; > } > > Note that the kfree() will be a no-op when obj == NULL. This means that > with just the current fix merged there is a small memleak on machines > where we hit the error-path. I've decided that we can live with that, > since the alternative is the crash or me pushing something untested. > > > 2. There is a while below this if, which gets a new obj pointer: > > obj = get_wmiobj_pointer(instance_id, guid); > if (!obj || obj->type != ACPI_TYPE_PACKAGE) { > kfree(obj); > return -ENODEV; > } > elements = obj->package.elements; > > mutex_lock(&wmi_priv.mutex); > while (elements) { > ... > > nextobj: > kfree(obj); > instance_id++; > obj = get_wmiobj_pointer(instance_id, guid); > elements = obj ? obj->package.elements : NULL; > } > > This is missing a check for the obj->type for the new obj when > going into a second (or higher) iteration of the loop. > > This check can be added by moving the original check to inside > the loop like this: > > obj = get_wmiobj_pointer(instance_id, guid); > > mutex_lock(&wmi_priv.mutex); > while (obj) { > if (obj->type != ACPI_TYPE_PACKAGE) { > err = ENODEV; > goto err_attr_init; > } > elements = obj->package.elements; > > ... > > nextobj: > kfree(obj); > instance_id++; > obj = get_wmiobj_pointer(instance_id, guid); > } > > > 3. Functions like populate_enum_data() (and the others) index the > elements array with an index > 0 without checking the package length > and also make assumptions about the types embedded in the package > without checking them. > > > 4. The err_attr_init exit path of init_bios_attributes() calls > release_attributes_data() but that call does not just cleanup > the results of that single init_bios_attributes() call but also > of all previous init_bios_attributes() calls as such it makes more > sense to leave the calling of release_attributes_data() to the caller. > > Either way calling it twice once from the err_attr_init exit path > and then again in sysman_init() feels wrong, even though I think it > does no harm. > > Regards, > > Hans > > > > > > > > > > > > > > >> >> Thank you >> >>> Regards, >>> >>> Hans >>> >>> >>> >>> >>>> return -ENODEV; >>>> + } >>>> elements = obj->package.elements; >>>> >>>> mutex_lock(&wmi_priv.mutex); >>>> while (elements) { >>>> /* sanity checking */ >>>> + if (elements[ATTR_NAME].type != ACPI_TYPE_STRING) { >>>> + pr_debug("incorrect element type\n"); >>>> + goto nextobj; >>>> + } >>>> if (strlen(elements[ATTR_NAME].string.pointer) == 0) { >>>> pr_debug("empty attribute found\n"); >>>> goto nextobj; >>>> >> > Hi Hans. Could you share your the commit link after you apply this patch to your for-next branch?
Thank you! Perry.
| |