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

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

[Hans de Goede]

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 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)
---
 .../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);
-- 
2.30.2

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

[Hans de Goede]

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 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?

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

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

[Ksr, Prasanth]

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 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.

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

Regards,
Prasanth K S R


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

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

[Hans de Goede]

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 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 ?

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

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

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

[Ksr, Prasanth]

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

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

[Hans de Goede]

Hi,

On 3/29/21 6:00 PM, Ksr, Prasanth wrote:
> 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.

Right, this is the right thing to do on machines such as the Latitude E5570.

My question is more, what if, in the future some machine does not have say
string bios-attributes, but it does have enum and int attributes ?

The current code will then still refuse to bind / load, which seems wrong.

Note that on the E5570 there are no enum and no string and no int attributes,
so we could also delay the return -ENODEV until after we have enumerated all 3
types and if all 3 have a wmi_priv.foo_instances_count of 0 then return -ENODEV
that seems like the logical thing to do here to me.

Regards,

Hans

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

[Ksr, Prasanth]

Hi,

> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Tuesday, March 30, 2021 1:24 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/29/21 6:00 PM, Ksr, Prasanth wrote:
> > 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.
> 
> Right, this is the right thing to do on machines such as the Latitude E5570.
> 
> My question is more, what if, in the future some machine does not have say
> string bios-attributes, but it does have enum and int attributes ?
> 
> The current code will then still refuse to bind / load, which seems wrong.
> 
> Note that on the E5570 there are no enum and no string and no int
> attributes, so we could also delay the return -ENODEV until after we have
> enumerated all 3 types and if all 3 have a wmi_priv.foo_instances_count of 0
> then return -ENODEV that seems like the logical thing to do here to me.

It is not only with Latitude E5570 and this device is an example of one such case which we are mentioning
We expect all 3 categories to be present always for supported platforms. Even a system with minimal BIOS attributes will have at least few mandatory attributes in each of the three categories.
If BIOS don't have any one category GUID exposed then it will be faulty BIOS and our driver must refuse to bind/load. Hence, we believe that current code has right logic.

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

[Hans de Goede]

Hi,

On 4/1/21 6:45 PM, Ksr, Prasanth wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: Tuesday, March 30, 2021 1:24 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/29/21 6:00 PM, Ksr, Prasanth wrote:
>>> 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.
>>
>> Right, this is the right thing to do on machines such as the Latitude E5570.
>>
>> My question is more, what if, in the future some machine does not have say
>> string bios-attributes, but it does have enum and int attributes ?
>>
>> The current code will then still refuse to bind / load, which seems wrong.
>>
>> Note that on the E5570 there are no enum and no string and no int
>> attributes, so we could also delay the return -ENODEV until after we have
>> enumerated all 3 types and if all 3 have a wmi_priv.foo_instances_count of 0
>> then return -ENODEV that seems like the logical thing to do here to me.
> 
> It is not only with Latitude E5570 and this device is an example of one such case which we are mentioning
> We expect all 3 categories to be present always for supported platforms. Even a system with minimal BIOS attributes will have at least few mandatory attributes in each of the three categories.
> If BIOS don't have any one category GUID exposed then it will be faulty BIOS and our driver must refuse to bind/load. Hence, we believe that current code has right logic.

Ok, so that means that this v2 is doing the right thing, as this version returns -ENODEV if any of the types are missing, just as before.

Since this adds a couple of useful robustness checks I'm going to merge this now.

As mentioned before, can you (Prasanth and/or Divya) please take care of the TODO from the commit message:

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 should
re-check this itself since it consume a variable number of elements.

Regards,

Hans

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

[Ksr, Prasanth]

Hi,

> -----Original Message-----
> From: Hans de Goede <hdegoede@redhat.com>
> Sent: Wednesday, April 7, 2021 5:02 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: RE: [PATCH v2] platform/x86: dell-wmi-sysman: Make
> init_bios_attributes() ACPI object parsing more robust
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi,
> 
> On 4/1/21 6:45 PM, Ksr, Prasanth wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Hans de Goede <hdegoede@redhat.com>
> >> Sent: Tuesday, March 30, 2021 1:24 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/29/21 6:00 PM, Ksr, Prasanth wrote:
> >>> 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://urldefense.com/v3/__https://bugzilla.redhat.com/show_bug.
> >>>>>>
> cgi?id=1936171__;!!LpKI!1eYVxhE0Cyt6JtzgTBadfIvZLQQ7LImJYSQns0D_A
> >>>>>> Yl6M_p00yMgV6NTFBoL4zMU$ [bugzilla[.]redhat[.]com]
> >>>>>>
> >>>>>> 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.
> >>
> >> Right, this is the right thing to do on machines such as the Latitude E5570.
> >>
> >> My question is more, what if, in the future some machine does not
> >> have say string bios-attributes, but it does have enum and int attributes ?
> >>
> >> The current code will then still refuse to bind / load, which seems wrong.
> >>
> >> Note that on the E5570 there are no enum and no string and no int
> >> attributes, so we could also delay the return -ENODEV until after we
> >> have enumerated all 3 types and if all 3 have a
> >> wmi_priv.foo_instances_count of 0 then return -ENODEV that seems like
> the logical thing to do here to me.
> >
> > It is not only with Latitude E5570 and this device is an example of
> > one such case which we are mentioning We expect all 3 categories to be
> present always for supported platforms. Even a system with minimal BIOS
> attributes will have at least few mandatory attributes in each of the three
> categories.
> > If BIOS don't have any one category GUID exposed then it will be faulty
> BIOS and our driver must refuse to bind/load. Hence, we believe that current
> code has right logic.
> 
> Ok, so that means that this v2 is doing the right thing, as this version returns -
> ENODEV if any of the types are missing, just as before.
> 
> Since this adds a couple of useful robustness checks I'm going to merge this
> now.
> 
> As mentioned before, can you (Prasanth and/or Divya) please take care of
> the TODO from the commit message:
> 
> 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 should
> re-check this itself since it consume a variable number of elements.

Ack. Will work on this TODO and send the patches soon.