Received-SPF: pass (google.com: domain of linux-kernel+bounces-58717-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <b1a8dbe8-a5c1-4462-993e-ba0888c5b23c@linux.intel.com>
Date: Thu, 8 Feb 2024 13:24:15 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] platform/x86: wmi: Make input buffer madatory when
 evaulating methods
Content-Language: en-US
To: Armin Wolf <W_Armin@gmx.de>, hdegoede@redhat.com,
 ilpo.jarvinen@linux.intel.com
Cc: platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240207202012.3506-1-W_Armin@gmx.de>
 <6360f90f-1aca-4355-aa19-661c2925dd24@linux.intel.com>
 <d82f47fa-a126-4242-b4c3-83ba2c37fd95@gmx.de>
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
In-Reply-To: <d82f47fa-a126-4242-b4c3-83ba2c37fd95@gmx.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


On 2/8/24 10:22 AM, Armin Wolf wrote:
> Am 08.02.24 um 03:41 schrieb Kuppuswamy Sathyanarayanan:
>
>> On 2/7/24 12:20 PM, Armin Wolf wrote:
>>> The ACPI-WMI specification declares that a WMxx control method takes
>>> 3 arguments: instance, method id and argument buffer. This is also
>>> the case even when the underlying WMI method does not have any
>>> input arguments.
>> It would be better if you include specification version and section
>> title for reference.
>
> The ACPI-WMI specification is not part of the ACPI specification. It
> consists of a single whitepaper published by Microsoft:
>
> https://github.com/microsoft/Windows-driver-samples/blob/main/wmi/wmiacpi/wmi-acpi.htm
>
> The section describing the ACPI control methods is called
> "ACPI Control Method Naming Conventions and Functionality for Windows 2000 Instrumentation".
>
> I do not thing that mentioning this in the commit message would be of any use.

I think it is better to include this detail. But, since it is not a standard specification,
I will let maintainer make this call.

>
>>> So if a WMI driver evaluates a WMI method without passing an input
>>> buffer, ACPICA will log a warning complaining that the third argument
>>> is missing.
>> I assume it is a compile warning. Can you copy the warning message?
>
> Its not a compiler warning.
>
> The ACPI control method is being evaluated at runtime, so the warning message
> will also be generated at runtime (when interpreting the AML bytecode).
>

Got it.

> Thanks,
> Armin Wolf
>
>>> Prevent this by checking that a input buffer was passed, and return
>>> an error if this was not the case.
>>>
>>> Tested on a Asus PRIME B650-Plus.
>>>
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> ---
>>
>> With above fixed,
>>
>> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>>>   drivers/platform/x86/wmi.c | 21 ++++++++++-----------
>>>   1 file changed, 10 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
>>> index 63906fdd0abf..f9e23d491dd9 100644
>>> --- a/drivers/platform/x86/wmi.c
>>> +++ b/drivers/platform/x86/wmi.c
>>> @@ -296,7 +296,7 @@ EXPORT_SYMBOL_GPL(wmidev_instance_count);
>>>    * @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
>>>    * @instance: Instance index
>>>    * @method_id: Method ID to call
>>> - * @in: Buffer containing input for the method call
>>> + * @in: Mandatory buffer containing input for the method call
>>>    * @out: Empty buffer to return the method results
>>>    *
>>>    * Call an ACPI-WMI method, the caller must free @out.
>>> @@ -326,7 +326,7 @@ EXPORT_SYMBOL_GPL(wmi_evaluate_method);
>>>    * @wdev: A wmi bus device from a driver
>>>    * @instance: Instance index
>>>    * @method_id: Method ID to call
>>> - * @in: Buffer containing input for the method call
>>> + * @in: Mandatory buffer containing input for the method call
>>>    * @out: Empty buffer to return the method results
>>>    *
>>>    * Call an ACPI-WMI method, the caller must free @out.
>>> @@ -347,26 +347,25 @@ acpi_status wmidev_evaluate_method(struct wmi_device *wdev, u8 instance, u32 met
>>>       block = &wblock->gblock;
>>>       handle = wblock->acpi_device->handle;
>>>
>>> +    if (!in)
>>> +        return AE_BAD_DATA;
>>> +
>>>       if (!(block->flags & ACPI_WMI_METHOD))
>>>           return AE_BAD_DATA;
>>>
>>>       if (block->instance_count <= instance)
>>>           return AE_BAD_PARAMETER;
>>>
>>> -    input.count = 2;
>>> +    input.count = 3;
>>>       input.pointer = params;
>>> +
>>>       params[0].type = ACPI_TYPE_INTEGER;
>>>       params[0].integer.value = instance;
>>>       params[1].type = ACPI_TYPE_INTEGER;
>>>       params[1].integer.value = method_id;
>>> -
>>> -    if (in) {
>>> -        input.count = 3;
>>> -
>>> -        params[2].type = get_param_acpi_type(wblock);
>>> -        params[2].buffer.length = in->length;
>>> -        params[2].buffer.pointer = in->pointer;
>>> -    }
>>> +    params[2].type = get_param_acpi_type(wblock);
>>> +    params[2].buffer.length = in->length;
>>> +    params[2].buffer.pointer = in->pointer;
>>>
>>>       get_acpi_method_name(wblock, 'M', method);
>>>
>>> -- 
>>> 2.39.2
>>>
>>>
>
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer