From: "Ksr, Prasanth" <Prasanth.Ksr@dell.com>
To: Hans de Goede <hdegoede@redhat.com>,
Mark Gross <mgross@linux.intel.com>,
Andy Shevchenko <andy@infradead.org>
Cc: "Limonciello, Mario" <Mario.Limonciello@dell.com>,
"Bharathi, Divya" <Divya.Bharathi@Dell.com>,
Alexander Naumann <alexandernaumann@gmx.de>,
"platform-driver-x86@vger.kernel.org"
<platform-driver-x86@vger.kernel.org>,
"Yuan, Perry" <Perry.Yuan@dell.com>
Subject: RE: [PATCH v2] platform/x86: dell-wmi-sysman: Make init_bios_attributes() ACPI object parsing more robust
Date: Mon, 29 Mar 2021 16:00:33 +0000 [thread overview]
Message-ID: <DM6PR19MB3676CB7EC5181819843A75DF827E9@DM6PR19MB3676.namprd19.prod.outlook.com> (raw)
In-Reply-To: <69d0c17a-a24b-6cf3-9acc-e6c4398c9a9c@redhat.com>
Hi,
> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Friday, March 26, 2021 10:14 PM
> To: Ksr, Prasanth; Mark Gross; Andy Shevchenko
> Cc: Limonciello, Mario; Bharathi, Divya; Alexander Naumann; platform-driver-
> x86@vger.kernel.org; Yuan, Perry
> Subject: Re: [PATCH v2] platform/x86: dell-wmi-sysman: Make
> init_bios_attributes() ACPI object parsing more robust
>
>
> [EXTERNAL EMAIL]
>
> Hi,
>
> On 3/26/21 4:40 PM, Ksr, Prasanth wrote:
> >
> >
> > Hi,
> >
> > [EXTERNAL EMAIL]
> >
> >> Hi,
> >>
> >> On 3/21/21 1:16 PM, Hans de Goede wrote:
> >>> Make init_bios_attributes() ACPI object parsing more robust:
> >>> 1. Always check that the type of the return ACPI object is package,
> rather
> >>> then only checking this for instance_id == 0 2. Check that the
> >>> package has the minimum amount of elements which will
> >>> be consumed by the populate_foo_data() for the attr_type
> >>>
> >>> Note/TODO: The populate_foo_data() functions should also be made
> >>> more robust. The should check the type of each of the elements
> >>> matches the type which they expect and in case of
> >>> populate_enum_data()
> >>> obj->package.count should be passed to it as an argument and it
> >>> obj->should
> >>> re-check this itself since it consume a variable number of elements.
> >>>
> >>> Fixes: e8a60aa7404b ("platform/x86: Introduce support for Systems
> >>> Management Driver over WMI for Dell Systems")
> >>> Cc: Divya Bharathi <Divya_Bharathi@dell.com>
> >>> Cc: Mario Limonciello <mario.limonciello@dell.com>
> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>> ---
> >>> Changes in v2:
> >>> - Restore behavior of returning -ENODEV when the
> get_wmiobj_pointer() call
> >>> for instance_id == 0 returns NULL. Otherwise
> >>> /sys/class/firmware-attributes/dell-wmi-sysman will get created with
> an
> >>> empty attributes dir (empty except for pending_reboot and
> >>> reset_bios)
> >
> >> So the reason for this change in v2 is that testing on a Dell Latitude E5570:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1936171
> >>
> >> With v1 of this patch results in an empty attributes dir (empty except for
> pending_reboot and reset_bios), interestingly enough there are both
> System and Admin dirs created under Authentication, so the BIOS of this
> device seems to have the GUIDs for both the attributes and the
> authentication interfaces; and it >has the 2 standard authentication objects,
> theoretically allowing changing the BIOS passwords from within Linux, but it
> does not advertise any attributes which can be changed.
> >>
> >> With the new v2 patch, the driver will now simply refuse to load (and it
> should no longer crash during cleanup because of the other patches).
> >>
> >> But I wonder what is actually the right thing to do here ? Arguably being
> able to change the BIOS passwords has some value on its own ?
> >>
> >> Mario, what is your take on this?
> >
> > Ideally we should not be hitting this code path at all. I am working with
> perry to see the results after applying the patches on the system.
> > If this is behavior we consistently see on older system BIOS then we will
> patch the code to avoid code path and bail out early not loading the driver.
>
> With v2 of my patches (which have been merged) we do bail out as soon as it
> is clear that there is no enum-type (*) attribute with instance_id 0. That is
> pretty-much the earliest that we can bail and that works fine.
>
> What I was wondering is if that is the right thing to do though. On the E5570
> there seem to be no enum/int/str attributes at all but what if there are enum
> + int attributes + no str attributes for example ?
>
It would be right thing to do because this scenario happens because of some BIOS issue.
> Also maybe the BIOS password interface on its own, without having any
> attributes at all has some value by itself ?
>
> *) enums are the first type we check for
>
No value will be added because of this and more over i am sure even that is because of a buggy BIOS revision
and the interface might not work as expected.
> > Also I was not cc'ed on the mail threads where the bugs were discussed
> > so going further please add me and Divya for reporting any issues on
> > the driver
>
> Yes that was my mistake, sorry about that. I'll make sure to Cc Divya and you
> on future emails about the dell-wmi-sysman driver.
>
> Regards,
>
> Hans
>
>
>
>
> >>> ---
> >>> .../x86/dell/dell-wmi-sysman/sysman.c | 32 ++++++++++++++++---
> >>> 1 file changed, 28 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c
> >>> b/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c
> >>> index 7410ccae650c..a90ae6ba4a73 100644
> >>> --- a/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c
> >>> +++ b/drivers/platform/x86/dell/dell-wmi-sysman/sysman.c
> >>> @@ -399,6 +399,7 @@ static int init_bios_attributes(int attr_type, const
> char *guid)
> >>> union acpi_object *obj = NULL;
> >>> union acpi_object *elements;
> >>> struct kset *tmp_set;
> >>> + int min_elements;
> >>>
> >>> /* instance_id needs to be reset for each type GUID
> >>> * also, instance IDs are unique within GUID but not across @@
> >>> -409,14 +410,38 @@ static int init_bios_attributes(int attr_type, const
> char *guid)
> >>> retval = alloc_attributes_data(attr_type);
> >>> if (retval)
> >>> return retval;
> >>> +
> >>> + switch (attr_type) {
> >>> + case ENUM: min_elements = 8; break;
> >>> + case INT: min_elements = 9; break;
> >>> + case STR: min_elements = 8; break;
> >>> + case PO: min_elements = 4; break;
> >>> + default:
> >>> + pr_err("Error: Unknown attr_type: %d\n", attr_type);
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> /* need to use specific instance_id and guid combination to get right
> data */
> >>> obj = get_wmiobj_pointer(instance_id, guid);
> >>> - if (!obj || obj->type != ACPI_TYPE_PACKAGE)
> >>> + if (!obj)
> >>> return -ENODEV;
> >>> - elements = obj->package.elements;
> >>>
> >>> mutex_lock(&wmi_priv.mutex);
> >>> - while (elements) {
> >>> + while (obj) {
> >>> + if (obj->type != ACPI_TYPE_PACKAGE) {
> >>> + pr_err("Error: Expected ACPI-package type, got:
> %d\n", obj->type);
> >>> + retval = -EIO;
> >>> + goto err_attr_init;
> >>> + }
> >>> +
> >>> + if (obj->package.count < min_elements) {
> >>> + pr_err("Error: ACPI-package does not have enough
> elements: %d < %d\n",
> >>> + obj->package.count, min_elements);
> >>> + goto nextobj;
> >>> + }
> >>> +
> >>> + elements = obj->package.elements;
> >>> +
> >>> /* sanity checking */
> >>> if (elements[ATTR_NAME].type != ACPI_TYPE_STRING) {
> >>> pr_debug("incorrect element type\n"); @@ -481,7
> +506,6 @@ static
> >>> int init_bios_attributes(int attr_type, const char *guid)
> >>> kfree(obj);
> >>> instance_id++;
> >>> obj = get_wmiobj_pointer(instance_id, guid);
> >>> - elements = obj ? obj->package.elements : NULL;
> >>> }
> >>>
> >>> mutex_unlock(&wmi_priv.mutex);
> >>>
> >
next prev parent reply other threads:[~2021-03-29 16:01 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-21 12:16 [PATCH v2] platform/x86: dell-wmi-sysman: Make init_bios_attributes() ACPI object parsing more robust Hans de Goede
2021-03-21 12:36 ` Hans de Goede
2021-03-26 15:40 ` Ksr, Prasanth
2021-03-26 16:43 ` Hans de Goede
2021-03-29 16:00 ` Ksr, Prasanth [this message]
2021-03-30 7:53 ` Hans de Goede
2021-04-01 16:45 ` Ksr, Prasanth
2021-04-07 11:32 ` Hans de Goede
2021-04-08 1:40 ` Ksr, Prasanth
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DM6PR19MB3676CB7EC5181819843A75DF827E9@DM6PR19MB3676.namprd19.prod.outlook.com \
--to=prasanth.ksr@dell.com \
--cc=Divya.Bharathi@Dell.com \
--cc=Mario.Limonciello@dell.com \
--cc=Perry.Yuan@dell.com \
--cc=alexandernaumann@gmx.de \
--cc=andy@infradead.org \
--cc=hdegoede@redhat.com \
--cc=mgross@linux.intel.com \
--cc=platform-driver-x86@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).