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

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

[Armin Wolf]

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

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

[PATCH 2/2] platform/x86: dell-ddv: Warn if ePPID has a suspicious length

[Armin Wolf]

On some systems (like the Dell Inspiron 3505), the acpi operation
region holding the ePPID string is two bytes too short, causing
acpi functions like ToString() to omit the last two bytes.
This does not happen on Windows, supposedly due to their implementation
of ToString() ignoring buffer boundaries.
Inform users if the ePPID length differs from the Dell specification
so they can complain to Dell to fix their BIOS.

Tested on a Dell Inspiron 3505.

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

diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
index 1a001296e8c6..2bb449845d14 100644
--- a/drivers/platform/x86/dell/dell-wmi-ddv.c
+++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
@@ -10,12 +10,14 @@
 #include <linux/acpi.h>
 #include <linux/debugfs.h>
 #include <linux/device.h>
+#include <linux/dev_printk.h>
 #include <linux/kernel.h>
 #include <linux/kstrtox.h>
 #include <linux/math.h>
 #include <linux/module.h>
 #include <linux/limits.h>
 #include <linux/power_supply.h>
+#include <linux/printk.h>
 #include <linux/seq_file.h>
 #include <linux/sysfs.h>
 #include <linux/wmi.h>
@@ -27,6 +29,9 @@
 #define DELL_DDV_SUPPORTED_INTERFACE 2
 #define DELL_DDV_GUID	"8A42EA14-4F2A-FD45-6422-0087F7A7E608"

+#define DELL_EPPID_LENGTH	20
+#define DELL_EPPID_EXT_LENGTH	23
+
 enum dell_ddv_method {
 	DELL_DDV_BATTERY_DESIGN_CAPACITY	= 0x01,
 	DELL_DDV_BATTERY_FULL_CHARGE_CAPACITY	= 0x02,
@@ -196,6 +201,10 @@ static ssize_t eppid_show(struct device *dev, struct device_attribute *attr, cha
 	if (ret < 0)
 		return ret;

+	if (obj->string.length != DELL_EPPID_LENGTH && obj->string.length != DELL_EPPID_EXT_LENGTH)
+		dev_info_once(&data->wdev->dev, FW_INFO "Suspicious ePPID length (%d)\n",
+			      obj->string.length);
+
 	ret = sysfs_emit(buf, "%s\n", obj->string.pointer);

 	kfree(obj);
--
2.30.2

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

[Hans de Goede]

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
> 

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

[David E. Box]

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
> 

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

[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?
> 
>> 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

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

[Armin Wolf]

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