[1/2] platform/x86: dell-ddv: Improve buffer handling

  When the DDV interface returns a buffer, it actually
returns a acpi buffer containing an integer (buffer size)
and another acpi buffer (buffer content).
The size of the buffer may be smaller than the size of
the buffer content, which is perfectly valid and should not
be treated as an error.
Also use the buffer size instead of the buffer content size
when accessing the buffer to prevent accessing bogus data.

Tested on a Dell Inspiron 3505.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/platform/x86/dell/dell-wmi-ddv.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

--
2.30.2
  

Hi,

On 11/2/22 22:23, Armin Wolf wrote:
> When the DDV interface returns a buffer, it actually
> returns a acpi buffer containing an integer (buffer size)
> and another acpi buffer (buffer content).
> The size of the buffer may be smaller than the size of
> the buffer content, which is perfectly valid and should not
> be treated as an error.
> Also use the buffer size instead of the buffer content size
> when accessing the buffer to prevent accessing bogus data.
> 
> Tested on a Dell Inspiron 3505.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Thank you for your patch-series, I've applied the series 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



> ---
>  drivers/platform/x86/dell/dell-wmi-ddv.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
> index 699feae3c435..1a001296e8c6 100644
> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
> @@ -129,9 +129,9 @@ static int dell_wmi_ddv_query_buffer(struct wmi_device *wdev, enum dell_ddv_meth
>  	if (obj->package.elements[1].type != ACPI_TYPE_BUFFER)
>  		goto err_free;
> 
> -	if (buffer_size != obj->package.elements[1].buffer.length) {
> +	if (buffer_size > obj->package.elements[1].buffer.length) {
>  		dev_warn(&wdev->dev,
> -			 FW_WARN "ACPI buffer size (%llu) does not match WMI buffer size (%d)\n",
> +			 FW_WARN "WMI buffer size (%llu) exceeds ACPI buffer size (%d)\n",
>  			 buffer_size, obj->package.elements[1].buffer.length);
> 
>  		goto err_free;
> @@ -271,15 +271,17 @@ static int dell_wmi_ddv_buffer_read(struct seq_file *seq, enum dell_ddv_method m
>  	struct device *dev = seq->private;
>  	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
>  	union acpi_object *obj;
> -	union acpi_object buf;
> +	u64 size;
> +	u8 *buf;
>  	int ret;
> 
>  	ret = dell_wmi_ddv_query_buffer(data->wdev, method, 0, &obj);
>  	if (ret < 0)
>  		return ret;
> 
> -	buf = obj->package.elements[1];
> -	ret = seq_write(seq, buf.buffer.pointer, buf.buffer.length);
> +	size = obj->package.elements[0].integer.value;
> +	buf = obj->package.elements[1].buffer.pointer;
> +	ret = seq_write(seq, buf, size);
>  	kfree(obj);
> 
>  	return ret;
> --
> 2.30.2
>
  

On Wed, 2022-11-02 at 22:23 +0100, Armin Wolf wrote:
> When the DDV interface returns a buffer, it actually
> returns a acpi buffer containing an integer (buffer size)
> and another acpi buffer (buffer content).
> The size of the buffer may be smaller than the size of
> the buffer content, which is perfectly valid and should not
> be treated as an error.

Is there documentation for this that you can refer to?

> Also use the buffer size instead of the buffer content size
> when accessing the buffer to prevent accessing bogus data.
> 
> Tested on a Dell Inspiron 3505.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  drivers/platform/x86/dell/dell-wmi-ddv.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c
> b/drivers/platform/x86/dell/dell-wmi-ddv.c
> index 699feae3c435..1a001296e8c6 100644
> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
> @@ -129,9 +129,9 @@ static int dell_wmi_ddv_query_buffer(struct wmi_device
> *wdev, enum dell_ddv_meth
>  	if (obj->package.elements[1].type != ACPI_TYPE_BUFFER)
>  		goto err_free;
> 
> -	if (buffer_size != obj->package.elements[1].buffer.length) {
> +	if (buffer_size > obj->package.elements[1].buffer.length) {
>  		dev_warn(&wdev->dev,
> -			 FW_WARN "ACPI buffer size (%llu) does not match WMI
> buffer size (%d)\n",
> +			 FW_WARN "WMI buffer size (%llu) exceeds ACPI buffer
> size (%d)\n",
>  			 buffer_size, obj->package.elements[1].buffer.length);
> 
>  		goto err_free;
> @@ -271,15 +271,17 @@ static int dell_wmi_ddv_buffer_read(struct seq_file
> *seq, enum dell_ddv_method m
>  	struct device *dev = seq->private;
>  	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
>  	union acpi_object *obj;
> -	union acpi_object buf;
> +	u64 size;
> +	u8 *buf;
>  	int ret;
> 
>  	ret = dell_wmi_ddv_query_buffer(data->wdev, method, 0, &obj);
>  	if (ret < 0)
>  		return ret;
> 
> -	buf = obj->package.elements[1];
> -	ret = seq_write(seq, buf.buffer.pointer, buf.buffer.length);
> +	size = obj->package.elements[0].integer.value;
> +	buf = obj->package.elements[1].buffer.pointer;
> +	ret = seq_write(seq, buf, size);

Okay, so the buffer may provide more space than what should actually be used
according to the size field. This looks like a bug that should have a fixes tag
on the original commit.

David

>  	kfree(obj);
> 
>  	return ret;
> --
> 2.30.2
>
  

Hi,

On 11/7/22 19:54, David E. Box wrote:
> On Wed, 2022-11-02 at 22:23 +0100, Armin Wolf wrote:
>> When the DDV interface returns a buffer, it actually
>> returns a acpi buffer containing an integer (buffer size)
>> and another acpi buffer (buffer content).
>> The size of the buffer may be smaller than the size of
>> the buffer content, which is perfectly valid and should not
>> be treated as an error.
> 
> Is there documentation for this that you can refer to?
> 
>> Also use the buffer size instead of the buffer content size
>> when accessing the buffer to prevent accessing bogus data.
>>
>> Tested on a Dell Inspiron 3505.
>>
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>  drivers/platform/x86/dell/dell-wmi-ddv.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c
>> b/drivers/platform/x86/dell/dell-wmi-ddv.c
>> index 699feae3c435..1a001296e8c6 100644
>> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
>> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
>> @@ -129,9 +129,9 @@ static int dell_wmi_ddv_query_buffer(struct wmi_device
>> *wdev, enum dell_ddv_meth
>>  	if (obj->package.elements[1].type != ACPI_TYPE_BUFFER)
>>  		goto err_free;
>>
>> -	if (buffer_size != obj->package.elements[1].buffer.length) {
>> +	if (buffer_size > obj->package.elements[1].buffer.length) {
>>  		dev_warn(&wdev->dev,
>> -			 FW_WARN "ACPI buffer size (%llu) does not match WMI
>> buffer size (%d)\n",
>> +			 FW_WARN "WMI buffer size (%llu) exceeds ACPI buffer
>> size (%d)\n",
>>  			 buffer_size, obj->package.elements[1].buffer.length);
>>
>>  		goto err_free;
>> @@ -271,15 +271,17 @@ static int dell_wmi_ddv_buffer_read(struct seq_file
>> *seq, enum dell_ddv_method m
>>  	struct device *dev = seq->private;
>>  	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
>>  	union acpi_object *obj;
>> -	union acpi_object buf;
>> +	u64 size;
>> +	u8 *buf;
>>  	int ret;
>>
>>  	ret = dell_wmi_ddv_query_buffer(data->wdev, method, 0, &obj);
>>  	if (ret < 0)
>>  		return ret;
>>
>> -	buf = obj->package.elements[1];
>> -	ret = seq_write(seq, buf.buffer.pointer, buf.buffer.length);
>> +	size = obj->package.elements[0].integer.value;
>> +	buf = obj->package.elements[1].buffer.pointer;
>> +	ret = seq_write(seq, buf, size);
> 
> Okay, so the buffer may provide more space than what should actually be used
> according to the size field. This looks like a bug that should have a fixes tag
> on the original commit.

I have already merged this and both the original commit as well as
this fix will land in 6.2, so I don't think a Fixes commit is
really necessary in this case.

Also the old code checked that the 2 sizes matched, so it was more
strict and as such running only the original patch should not lead
to buffer overruns or anything like that.

Regards,

Hans
  

Am 07.11.22 um 20:49 schrieb Hans de Goede:

> Hi,
>
> On 11/7/22 19:54, David E. Box wrote:
>> On Wed, 2022-11-02 at 22:23 +0100, Armin Wolf wrote:
>>> When the DDV interface returns a buffer, it actually
>>> returns a acpi buffer containing an integer (buffer size)
>>> and another acpi buffer (buffer content).
>>> The size of the buffer may be smaller than the size of
>>> the buffer content, which is perfectly valid and should not
>>> be treated as an error.
>> Is there documentation for this that you can refer to?

Yes and no. With the bmf2mof tool, i was able to extract the following MOF-description of the WMI interface:

[WMI, Dynamic, Provider("WmiProv"), Locale("MS\\0x409"), Description("WMI Function"), guid("{8A42EA14-4F2A-FD45-6422-0087F7A7E608}")]
class DDVWmiMethodFunction {
   [key, read] string InstanceName;
   [read] boolean Active;

   [WmiMethodId(1), Implemented, read, write, Description("Return Battery Design Capacity.")] void BatteryDesignCapacity([in] uint32 arg2, [out] uint32 argr);
   [WmiMethodId(2), Implemented, read, write, Description("Return Battery Full Charge Capacity.")] void BatteryFullChargeCapacity([in] uint32 arg2, [out] uint32 argr);
   [WmiMethodId(3), Implemented, read, write, Description("Return Battery Manufacture Name.")] void BatteryManufactureName([in] uint32 arg2, [out] string argr);
   [WmiMethodId(4), Implemented, read, write, Description("Return Battery Manufacture Date.")] void BatteryManufactureDate([in] uint32 arg2, [out] uint32 argr);
   [WmiMethodId(5), Implemented, read, write, Description("Return Battery Serial Number.")] void BatterySerialNumber([in] uint32 arg2, [out] uint32 argr);
   [WmiMethodId(6), Implemented, read, write, Description("Return Battery Chemistry Value.")] void BatteryChemistryValue([in] uint32 arg2, [out] string argr);
   [WmiMethodId(7), Implemented, read, write, Description("Return Battery Temperature.")] void BatteryTemperature([in] uint32 arg2, [out] uint32 argr);
   [WmiMethodId(8), Implemented, read, write, Description("Return Battery Current.")] void BatteryCurrent([in] uint32 arg2, [out] uint32 argr);
   [WmiMethodId(9), Implemented, read, write, Description("Return Battery Voltage.")] void BatteryVoltage([in] uint32 arg2, [out] uint32 argr);
   [WmiMethodId(10), Implemented, read, write, Description("Return Battery Manufacture Access(MA code).")] void BatteryManufactureAceess([in] uint32 arg2, [out] uint32 argr);
   [WmiMethodId(11), Implemented, read, write, Description("Return Battery Relative State-Of-Charge.")] void BatteryRelativeStateOfCharge([in] uint32 arg2, [out] uint32 argr);
   [WmiMethodId(12), Implemented, read, write, Description("Return Battery Cycle Count")] void BatteryCycleCount([in] uint32 arg2, [out] uint32 argr);
   [WmiMethodId(13), Implemented, read, write, Description("Return Battery ePPID")] void BatteryePPID([in] uint32 arg2, [out] string argr);
   [WmiMethodId(14), Implemented, read, write, Description("Return Battery Raw Analytics Start")] void BatteryeRawAnalyticsStart([in] uint32 arg2, [out] uint32 argr);
   [WmiMethodId(15), Implemented, read, write, Description("Return Battery Raw Analytics")] void BatteryeRawAnalytics([in] uint32 arg2, [out] uint32 RawSize, [out, WmiSizeIs("RawSize") : ToInstance] uint8 RawData[]);
   [WmiMethodId(16), Implemented, read, write, Description("Return Battery Design Voltage.")] void BatteryDesignVoltage([in] uint32 arg2, [out] uint32 argr);
   [WmiMethodId(18), Implemented, read, write, Description("Return Version.")] void ReturnVersion([in] uint32 arg2, [out] uint32 argr);
   [WmiMethodId(32), Implemented, read, write, Description("Return Fan Sensor Information")] void FanSensorInformation([in] uint32 arg2, [out] uint32 RawSize, [out, WmiSizeIs("RawSize") : ToInstance] uint8 RawData[]);
   [WmiMethodId(34), Implemented, read, write, Description("Return Thermal Sensor Information")] void ThermalSensorInformation([in] uint32 arg2, [out] uint32 RawSize, [out, WmiSizeIs("RawSize") : ToInstance] uint8 RawData[]);
};

I also found out that an "empty" fan sensor information buffer still has a content buffer length of 1, but a size integer
of 0.

>>> Also use the buffer size instead of the buffer content size
>>> when accessing the buffer to prevent accessing bogus data.
>>>
>>> Tested on a Dell Inspiron 3505.
>>>
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> ---
>>>   drivers/platform/x86/dell/dell-wmi-ddv.c | 12 +++++++-----
>>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c
>>> b/drivers/platform/x86/dell/dell-wmi-ddv.c
>>> index 699feae3c435..1a001296e8c6 100644
>>> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
>>> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
>>> @@ -129,9 +129,9 @@ static int dell_wmi_ddv_query_buffer(struct wmi_device
>>> *wdev, enum dell_ddv_meth
>>>   	if (obj->package.elements[1].type != ACPI_TYPE_BUFFER)
>>>   		goto err_free;
>>>
>>> -	if (buffer_size != obj->package.elements[1].buffer.length) {
>>> +	if (buffer_size > obj->package.elements[1].buffer.length) {
>>>   		dev_warn(&wdev->dev,
>>> -			 FW_WARN "ACPI buffer size (%llu) does not match WMI
>>> buffer size (%d)\n",
>>> +			 FW_WARN "WMI buffer size (%llu) exceeds ACPI buffer
>>> size (%d)\n",
>>>   			 buffer_size, obj->package.elements[1].buffer.length);
>>>
>>>   		goto err_free;
>>> @@ -271,15 +271,17 @@ static int dell_wmi_ddv_buffer_read(struct seq_file
>>> *seq, enum dell_ddv_method m
>>>   	struct device *dev = seq->private;
>>>   	struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
>>>   	union acpi_object *obj;
>>> -	union acpi_object buf;
>>> +	u64 size;
>>> +	u8 *buf;
>>>   	int ret;
>>>
>>>   	ret = dell_wmi_ddv_query_buffer(data->wdev, method, 0, &obj);
>>>   	if (ret < 0)
>>>   		return ret;
>>>
>>> -	buf = obj->package.elements[1];
>>> -	ret = seq_write(seq, buf.buffer.pointer, buf.buffer.length);
>>> +	size = obj->package.elements[0].integer.value;
>>> +	buf = obj->package.elements[1].buffer.pointer;
>>> +	ret = seq_write(seq, buf, size);
>> Okay, so the buffer may provide more space than what should actually be used
>> according to the size field. This looks like a bug that should have a fixes tag
>> on the original commit.
> I have already merged this and both the original commit as well as
> this fix will land in 6.2, so I don't think a Fixes commit is
> really necessary in this case.
>
> Also the old code checked that the 2 sizes matched, so it was more
> strict and as such running only the original patch should not lead
> to buffer overruns or anything like that.
>
> Regards,
>
> Hans
>
>