Re: [PATCH v5 4/4] Changing bios_args.data to be dynamically allocated

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

Hi,

On 3/14/22 11:51, Hans de Goede wrote:
> Hi,
> 
> On 3/10/22 22:08, Jorge Lopez wrote:
>> The purpose of this patch is to remove 128 bytes buffer limitation
>> imposed in bios_args structure.
>>
>> A limiting factor discovered during this investigation was the struct
>> bios_args.data size restriction.  The data member size limits all
>> possible WMI commands to those requiring buffer size of 128 bytes or
>> less. Several WMI commands and queries require a buffer size larger
>> than 128 bytes hence limiting current and feature supported by the
>> driver. It is for this reason, struct bios_args.data changed and is
>> dynamically allocated.  hp_wmi_perform_query function changed to
>> handle the memory allocation and release of any required buffer size.
>>
>> All changes were validated on a HP ZBook Workstation notebook,
>> HP EliteBook x360, and HP EliteBook 850 G8.  Additional
>> validation was included in the test process to ensure no other
>> commands were incorrectly handled.
>>
>> Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com>
>>
>> ---
>> Based on the latest platform-drivers-x86.git/for-next
>> ---
>>  drivers/platform/x86/hp-wmi.c | 64 +++++++++++++++++++++++------------
>>  1 file changed, 42 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
>> index e76bd4bef6b5..cc5c4f637328 100644
>> --- a/drivers/platform/x86/hp-wmi.c
>> +++ b/drivers/platform/x86/hp-wmi.c
>> @@ -82,12 +82,17 @@ enum hp_wmi_event_ids {
>>  	HPWMI_BATTERY_CHARGE_PERIOD	= 0x10,
>>  };
>>  
>> +/**
>> + * struct bios_args buffer is dynamically allocated.  New WMI command types
>> + * were introduced that exceeds 128-byte data size.  Changes to handle
>> + * the data size allocation scheme were kept in hp_wmi_perform_qurey function.
>> + */
>>  struct bios_args {
>>  	u32 signature;
>>  	u32 command;
>>  	u32 commandtype;
>>  	u32 datasize;
>> -	u8 data[128];
>> +	u8 data[];
>>  };
>>  
>>  enum hp_wmi_commandtype {
>> @@ -268,34 +273,40 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>>  	int mid;
>>  	struct bios_return *bios_return;
>>  	int actual_outsize;
>> -	union acpi_object *obj;
>> -	struct bios_args args = {
>> -		.signature = 0x55434553,
>> -		.command = command,
>> -		.commandtype = query,
>> -		.datasize = insize,
>> -		.data = { 0 },
>> -	};
>> -	struct acpi_buffer input = { sizeof(struct bios_args), &args };
>> +	union acpi_object *obj = NULL;
>> +	struct bios_args *args = NULL;
>> +	size_t bios_args_size = struct_size(args, data, insize);
>> +	
>> +	struct acpi_buffer input;
>>  	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
>>  	int ret = 0;
>>  
>> -	mid = encode_outsize_for_pvsz(outsize);
>> -	if (WARN_ON(mid < 0))
>> -		return mid;
>> +	args = kmalloc(bios_args_size, GFP_KERNEL);
>> +	if (!args)
>> +		return -ENOMEM;
> 
> The variable declaration here again looks a bit messy, also
> there is no reason to move the block setting + checking mid,
> that just makes the diff unnecessarily large.
> 
> I've cleaned this up while mergin (no functional changes).
> 
> 
>>  
>> -	if (WARN_ON(insize > sizeof(args.data)))
>> -		return -EINVAL;
>> -	memcpy(&args.data[0], buffer, insize);
>> +	input.length = bios_args_size;
>> +	input.pointer = args;
>>  
>> -	wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
>> +	mid = encode_outsize_for_pvsz(outsize);
>> +	if (WARN_ON(mid < 0)) {
>> +		ret = mid;
>> +		goto out_free;
>> +	}
>>  
>> -	obj = output.pointer;
>> +	memcpy(args->data, buffer, flex_array_size(args, data, insize));
>>  
>> -	if (!obj)
>> -		return -EINVAL;
>> +	args->signature = 0x55434553;
>> +	args->command = command;
>> +	args->commandtype = query;
>> +	args->datasize = insize;
>>  
>> -	if (obj->type != ACPI_TYPE_BUFFER) {
>> +	ret = wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
>> +	if (ret)
>> +		goto out_free;
>> +
>> +	obj = output.pointer;
>> +	if (!obj) {
>>  		ret = -EINVAL;
>>  		goto out_free;
>>  	}
>> @@ -310,9 +321,17 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>>  		goto out_free;
>>  	}
>>  
>> +	if (obj->type != ACPI_TYPE_BUFFER) {
>> +		pr_warn("query 0x%x returned an invalid object 0x%x\n", query, ret);
>> +		ret = -EINVAL;
>> +		goto out_free;
>> +	}
>> +
> 
> You have now moved the obj->type != ACPI_TYPE_BUFFER check to after the:
> 	
> 	bios_return = (struct bios_return *)obj->buffer.pointer;
> 
> Statement, which dereferences the buffer member of the obj union.  That check
> MUST be done before looking at the buffer member, so I have moved it back to
> its old place, this also makes the diff/patch smaller.  Note this one is
> a functional change to your patch.
> 
> The changed initial block of the function now looks like this:
> 
> static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
> 				void *buffer, int insize, int outsize)
> {
> 	struct acpi_buffer input, output = { ACPI_ALLOCATE_BUFFER, NULL };
> 	size_t bios_args_size = struct_size(args, data, insize);

Ahum, there is a compile error here because args is not declared yet, so the final
version looks slightly different, see:

https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=review-hans&id=9f7e383ebdee6712bee02e3a6c2027cf287950fc

Regards,

Hans


> 	struct bios_return *bios_return;
> 	union acpi_object *obj = NULL;
> 	struct bios_args *args = NULL;
> 	int mid, actual_outsize, ret;
> 
> 	mid = encode_outsize_for_pvsz(outsize);
> 	if (WARN_ON(mid < 0))
> 		return mid;
> 
> 	args = kmalloc(bios_args_size, GFP_KERNEL);
> 	if (!args)
> 		return -ENOMEM;
> 
> 	input.length = bios_args_size;
> 	input.pointer = args;
> 
> 	args->signature = 0x55434553;
> 	args->command = command;
> 	args->commandtype = query;
> 	args->datasize = insize;
> 	memcpy(args->data, buffer, flex_array_size(args, data, insize));
> 
> 	ret = wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
> 	if (ret)
> 		goto out_free;
> 
> 	obj = output.pointer;
> 	if (!obj) {
> 		ret = -EINVAL;
> 		goto out_free;
> 	}
> 
> 	if (obj->type != ACPI_TYPE_BUFFER) {
> 		pr_warn("query 0x%x returned an invalid object 0x%x\n", query, ret);
> 		ret = -EINVAL;
> 		goto out_free;
> 	}
> 
> 	bios_return = (struct bios_return *)obj->buffer.pointer;
> 
> 
> And the new diff for this chunk of the patch now is:
> 
> @@ -266,37 +271,42 @@ static inline int encode_outsize_for_pvsz(int outsize)
>  static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>  				void *buffer, int insize, int outsize)
>  {
> -	int mid;
> +	struct acpi_buffer input, output = { ACPI_ALLOCATE_BUFFER, NULL };
> +	size_t bios_args_size = struct_size(args, data, insize);
>  	struct bios_return *bios_return;
> -	int actual_outsize;
> -	union acpi_object *obj;
> -	struct bios_args args = {
> -		.signature = 0x55434553,
> -		.command = command,
> -		.commandtype = query,
> -		.datasize = insize,
> -		.data = { 0 },
> -	};
> -	struct acpi_buffer input = { sizeof(struct bios_args), &args };
> -	struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> -	int ret = 0;
> +	union acpi_object *obj = NULL;
> +	struct bios_args *args = NULL;
> +	int mid, actual_outsize, ret;
>  
>  	mid = encode_outsize_for_pvsz(outsize);
>  	if (WARN_ON(mid < 0))
>  		return mid;
>  
> -	if (WARN_ON(insize > sizeof(args.data)))
> -		return -EINVAL;
> -	memcpy(&args.data[0], buffer, insize);
> +	args = kmalloc(bios_args_size, GFP_KERNEL);
> +	if (!args)
> +		return -ENOMEM;
>  
> -	wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
> +	input.length = bios_args_size;
> +	input.pointer = args;
>  
> -	obj = output.pointer;
> +	args->signature = 0x55434553;
> +	args->command = command;
> +	args->commandtype = query;
> +	args->datasize = insize;
> +	memcpy(args->data, buffer, flex_array_size(args, data, insize));
>  
> -	if (!obj)
> -		return -EINVAL;
> +	ret = wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
> +	if (ret)
> +		goto out_free;
> +
> +	obj = output.pointer;
> +	if (!obj) {
> +		ret = -EINVAL;
> +		goto out_free;
> +	}
>  
>  	if (obj->type != ACPI_TYPE_BUFFER) {
> +		pr_warn("query 0x%x returned an invalid object 0x%x\n", query, ret);
>  		ret = -EINVAL;
>  		goto out_free;
>  	}
> @@ ...
> 
> I've merged this patch with the above changes:
> 
> Thank you for your patch, I've applied this patch to my review-hans 
> branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
> 
> Note it will show up in my review-hans branch once I've pushed my
> local branch there, which might take a while.
> 
> Once I've run some tests on this branch the patches there will be
> added to the platform-drivers-x86/for-next branch and eventually
> will be included in the pdx86 pull-request to Linus for the next
> merge-window.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
> 
> 
>>  	/* Ignore output data of zero size */
>> -	if (!outsize)
>> +	if (!outsize) {
>> +		ret = 0;
>>  		goto out_free;
>> +	}
>>  
>>  	actual_outsize = min(outsize, (int)(obj->buffer.length - sizeof(*bios_return)));
>>  	memcpy(buffer, obj->buffer.pointer + sizeof(*bios_return), actual_outsize);
>> @@ -320,6 +339,7 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>>  
>>  out_free:
>>  	kfree(obj);
>> +	kfree(args);
>>  	return ret;
>>  }
>>  
> 
>