| Message ID | 20210129172654.2326751-1-mario.limonciello@dell.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | platform/x86: dell-wmi-sysman: fix a NULL pointer dereference | expand |
+Divya, +Prasanth, +Perry > -----Original Message----- > From: Limonciello, Mario <Mario_Limonciello@Dell.com> > Sent: Friday, January 29, 2021 11:27 > To: Hans De Goede; Mark Gross > Cc: LKML; platform-driver-x86@vger.kernel.org; Limonciello, Mario > Subject: [PATCH] platform/x86: dell-wmi-sysman: fix a NULL pointer dereference > > 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(); > 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; > -- > 2.25.1
> -----Original Message----- > From: Limonciello, Mario <Mario_Limonciello@Dell.com> > Sent: 2021年1月30日 1:29 > To: Hans De Goede; Mark Gross > Cc: LKML; platform-driver-x86@vger.kernel.org; Bharathi, Divya; Ksr, Prasanth; > Yuan, Perry > Subject: RE: [PATCH] platform/x86: dell-wmi-sysman: fix a NULL pointer > dereference > > +Divya, +Prasanth, +Perry > > > -----Original Message----- > > From: Limonciello, Mario <Mario_Limonciello@Dell.com> > > Sent: Friday, January 29, 2021 11:27 > > To: Hans De Goede; Mark Gross > > Cc: LKML; platform-driver-x86@vger.kernel.org; Limonciello, Mario > > Subject: [PATCH] platform/x86: dell-wmi-sysman: fix a NULL pointer > > dereference > > > > 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(); > > 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; > > -- > > 2.25.1 Tested on the Dell laptop system which I found the issue. Kernel can boot up with this new fix patch without any panic triggered. Tested-by: Perry Yuan <perry_yuan@dell.com>
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. 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, 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; >>> >
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.
Hi, On 2/1/21 3:36 AM, Perry Yuan wrote: <snip> > Hi Hans. > Could you share your the commit link after you apply this patch to your for-next branch? https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=for-next&id=64b0efa18f8c3b1baac369b8d74d0fdae02bc4bc Regards, Hans
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(); 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;
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(-)