Re: 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 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