RE: [PATCH v2] platform/x86: dell-wmi-sysman: Make init_bios_attributes() ACPI object parsing more robust

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);
> >>>
> >