[PATCH v2 0/2] ACPI: battery: Fix various string handling issues

[PATCH v2 0/2] ACPI: battery: Fix various string handling issues

[Armin Wolf]

On my Dell Inspiron 3505, the battery model name was displayed
differently than when running Windows. While i first suspected an
ACPI issue, it turned out that the real reason was the ACPI battery
driver failing to handle strings larger than 32 bytes.

This caused the model name of the battery (35 bytes long, hex string)
to miss proper NUL-termination, resulting in a buffer overread later.
Luckily, a valid string was stored right after the now invalid string,
appending only the battery serial number to the original model name.

The first patch fixes a potential buffer overread then handling buffers,
while the second patch finally increases the maximum string length to
avoid truncating such larger strings.

The patch series was tested on a Dell Inspiron 3505 and appears
to work properly.
---
Changes in v2:
- Drop first patch since it was already applied
- combine the second and third patch
- do not replace 0 with '\0'
- spell ACPI in capitals
- rework the buffer length hdanling

Armin Wolf (2):
  ACPI: battery: Fix buffer overread if not NUL-terminated
  ACPI: battery: Increase maximum string length

 drivers/acpi/battery.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

--
2.30.2

[PATCH v2 1/2] ACPI: battery: Fix buffer overread if not NUL-terminated

[Armin Wolf]

If a buffer containing ASCII characters is not NUL-terminated
(which is perfectly legal according to the ACPI specification),
the ACPI battery driver might not honor its length.
Fix this by limiting the amount of data to be copied to
the buffer length while also using strscpy() to make sure
that the resulting string is always NUL-terminated.
Also replace strncpy() vs strscpy().

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 drivers/acpi/battery.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index fb64bd217d82..0ec12a7dbcca 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -437,16 +437,25 @@ static int extract_package(struct acpi_battery *battery,
 		element = &package->package.elements[i];
 		if (offsets[i].mode) {
 			u8 *ptr = (u8 *)battery + offsets[i].offset;
+			u32 len = 32;

-			if (element->type == ACPI_TYPE_STRING ||
-			    element->type == ACPI_TYPE_BUFFER)
-				strscpy(ptr, element->string.pointer, 32);
-			else if (element->type == ACPI_TYPE_INTEGER) {
-				strncpy(ptr, (u8 *)&element->integer.value,
-					sizeof(u64));
-				ptr[sizeof(u64)] = 0;
-			} else
+			switch (element->type) {
+			case ACPI_TYPE_BUFFER:
+				if (len > element->buffer.length + 1)
+					len = element->buffer.length + 1;
+
+				fallthrough;
+			case ACPI_TYPE_STRING:
+				strscpy(ptr, element->string.pointer, len);
+
+				break;
+			case ACPI_TYPE_INTEGER:
+				strscpy(ptr, (u8 *)&element->integer.value, sizeof(u64) + 1);
+
+				break;
+			default:
 				*ptr = 0; /* don't have value */
+			}
 		} else {
 			int *x = (int *)((u8 *)battery + offsets[i].offset);
 			*x = (element->type == ACPI_TYPE_INTEGER) ?
--
2.30.2

[PATCH v2 2/2] ACPI: battery: Increase maximum string length

[Armin Wolf]

On the Dell Inspiron 3505, the battery model name
is represented as a hex string containing seven numbers,
causing it to be larger than the current maximum string
length (32).
Increase this length to 64 to avoid truncating the string
in such cases. Also introduce a common define for the length.

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

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 0ec12a7dbcca..9c67ed02d797 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -42,6 +42,8 @@
 #define ACPI_BATTERY_STATE_CHARGING	0x2
 #define ACPI_BATTERY_STATE_CRITICAL	0x4

+#define MAX_STRING_LENGTH	64
+
 MODULE_AUTHOR("Paul Diefenbaugh");
 MODULE_AUTHOR("Alexey Starikovskiy <astarikovskiy@suse.de>");
 MODULE_DESCRIPTION("ACPI Battery Driver");
@@ -118,10 +120,10 @@ struct acpi_battery {
 	int capacity_granularity_1;
 	int capacity_granularity_2;
 	int alarm;
-	char model_number[32];
-	char serial_number[32];
-	char type[32];
-	char oem_info[32];
+	char model_number[MAX_STRING_LENGTH];
+	char serial_number[MAX_STRING_LENGTH];
+	char type[MAX_STRING_LENGTH];
+	char oem_info[MAX_STRING_LENGTH];
 	int state;
 	int power_unit;
 	unsigned long flags;
@@ -437,7 +439,7 @@ static int extract_package(struct acpi_battery *battery,
 		element = &package->package.elements[i];
 		if (offsets[i].mode) {
 			u8 *ptr = (u8 *)battery + offsets[i].offset;
-			u32 len = 32;
+			u32 len = MAX_STRING_LENGTH;

 			switch (element->type) {
 			case ACPI_TYPE_BUFFER:
--
2.30.2

Re: [PATCH v2 0/2] ACPI: battery: Fix various string handling issues

[Armin Wolf]

Am 19.01.23 um 15:21 schrieb Armin Wolf:

> On my Dell Inspiron 3505, the battery model name was displayed
> differently than when running Windows. While i first suspected an
> ACPI issue, it turned out that the real reason was the ACPI battery
> driver failing to handle strings larger than 32 bytes.
>
> This caused the model name of the battery (35 bytes long, hex string)
> to miss proper NUL-termination, resulting in a buffer overread later.
> Luckily, a valid string was stored right after the now invalid string,
> appending only the battery serial number to the original model name.
>
> The first patch fixes a potential buffer overread then handling buffers,
> while the second patch finally increases the maximum string length to
> avoid truncating such larger strings.
>
> The patch series was tested on a Dell Inspiron 3505 and appears
> to work properly.

Are there any outstanding issues with the patch series which need
to be fixed for mainline inclusion?

Armin Wolf

> ---
> Changes in v2:
> - Drop first patch since it was already applied
> - combine the second and third patch
> - do not replace 0 with '\0'
> - spell ACPI in capitals
> - rework the buffer length hdanling
>
> Armin Wolf (2):
>    ACPI: battery: Fix buffer overread if not NUL-terminated
>    ACPI: battery: Increase maximum string length
>
>   drivers/acpi/battery.c | 35 +++++++++++++++++++++++------------
>   1 file changed, 23 insertions(+), 12 deletions(-)
>
> --
> 2.30.2
>
>

Re: [PATCH v2 0/2] ACPI: battery: Fix various string handling issues

[Rafael J. Wysocki]

On Mon, Jan 30, 2023 at 10:14 AM Armin Wolf <W_Armin@gmx.de> wrote:
>
> Am 19.01.23 um 15:21 schrieb Armin Wolf:
>
> > On my Dell Inspiron 3505, the battery model name was displayed
> > differently than when running Windows. While i first suspected an
> > ACPI issue, it turned out that the real reason was the ACPI battery
> > driver failing to handle strings larger than 32 bytes.
> >
> > This caused the model name of the battery (35 bytes long, hex string)
> > to miss proper NUL-termination, resulting in a buffer overread later.
> > Luckily, a valid string was stored right after the now invalid string,
> > appending only the battery serial number to the original model name.
> >
> > The first patch fixes a potential buffer overread then handling buffers,
> > while the second patch finally increases the maximum string length to
> > avoid truncating such larger strings.
> >
> > The patch series was tested on a Dell Inspiron 3505 and appears
> > to work properly.
>
> Are there any outstanding issues with the patch series which need
> to be fixed for mainline inclusion?

I'll have a look shortly and let you know.

Thanks!

> > ---
> > Changes in v2:
> > - Drop first patch since it was already applied
> > - combine the second and third patch
> > - do not replace 0 with '\0'
> > - spell ACPI in capitals
> > - rework the buffer length hdanling
> >
> > Armin Wolf (2):
> >    ACPI: battery: Fix buffer overread if not NUL-terminated
> >    ACPI: battery: Increase maximum string length
> >
> >   drivers/acpi/battery.c | 35 +++++++++++++++++++++++------------
> >   1 file changed, 23 insertions(+), 12 deletions(-)
> >
> > --

Re: [PATCH v2 0/2] ACPI: battery: Fix various string handling issues

[Rafael J. Wysocki]

On Mon, Jan 30, 2023 at 1:29 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Jan 30, 2023 at 10:14 AM Armin Wolf <W_Armin@gmx.de> wrote:
> >
> > Am 19.01.23 um 15:21 schrieb Armin Wolf:
> >
> > > On my Dell Inspiron 3505, the battery model name was displayed
> > > differently than when running Windows. While i first suspected an
> > > ACPI issue, it turned out that the real reason was the ACPI battery
> > > driver failing to handle strings larger than 32 bytes.
> > >
> > > This caused the model name of the battery (35 bytes long, hex string)
> > > to miss proper NUL-termination, resulting in a buffer overread later.
> > > Luckily, a valid string was stored right after the now invalid string,
> > > appending only the battery serial number to the original model name.
> > >
> > > The first patch fixes a potential buffer overread then handling buffers,
> > > while the second patch finally increases the maximum string length to
> > > avoid truncating such larger strings.
> > >
> > > The patch series was tested on a Dell Inspiron 3505 and appears
> > > to work properly.
> >
> > Are there any outstanding issues with the patch series which need
> > to be fixed for mainline inclusion?
>
> I'll have a look shortly and let you know.

I've queued up the patches for 6.3, thanks!