Hi,
> -----Original Message-----
> From: Hans de Goede <hdegoede@xxxxxxxxxx>
> 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@xxxxxxxxxxxxxxx; 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@xxxxxxxx>
> >>> Cc: Mario Limonciello <mario.limonciello@xxxxxxxx>
> >>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >>> ---
> >>> 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);
> >>>
> >
