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

[Hans de Goede <hdegoede@redhat.com>]

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