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, causing a buffer overread later.
Luckely, 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 deals with this issues, while the second patch fixes
another problem which could happen when handling buffers. The third
patch adds a minor improvement to the string handling code and the
fourth 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.
Armin Wolf (4):
ACPI: battery: Fix missing NUL-termination with large strings
ACPI: battery: Fix buffer overread if not NUL-terminated
ACPI: battery: Replace strncpy() with strscpy()
ACPI: battery: Increase maximum string length
drivers/acpi/battery.c | 36 +++++++++++++++++++++++-------------
1 file changed, 23 insertions(+), 13 deletions(-)
--
2.30.2
Currently, strncpy() and manual NUL-termination is used
when copying integers. Switch to strscpy() which takes care
of NUL-terminating the result.
Signed-off-by: Armin Wolf <[email protected]>
---
drivers/acpi/battery.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 9f6daa9f2010..b39b84b8f3ae 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -449,8 +449,7 @@ static int extract_package(struct acpi_battery *battery,
break;
case ACPI_TYPE_INTEGER:
- strncpy(ptr, (u8 *)&element->integer.value, sizeof(u64));
- ptr[sizeof(u64)] = 0;
+ strscpy(ptr, (u8 *)&element->integer.value, sizeof(u64) + 1);
break;
default:
--
2.30.2
If the buffer containing string data 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 use '\0' instead of a plain 0.
Signed-off-by: Armin Wolf <[email protected]>
---
drivers/acpi/battery.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index fb64bd217d82..9f6daa9f2010 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -438,15 +438,24 @@ static int extract_package(struct acpi_battery *battery,
if (offsets[i].mode) {
u8 *ptr = (u8 *)battery + offsets[i].offset;
- if (element->type == ACPI_TYPE_STRING ||
- element->type == ACPI_TYPE_BUFFER)
+ switch (element->type) {
+ case ACPI_TYPE_STRING:
strscpy(ptr, element->string.pointer, 32);
- else if (element->type == ACPI_TYPE_INTEGER) {
- strncpy(ptr, (u8 *)&element->integer.value,
- sizeof(u64));
+
+ break;
+ case ACPI_TYPE_BUFFER:
+ strscpy(ptr, element->buffer.pointer,
+ min_t(u32, element->buffer.length + 1, 32));
+
+ break;
+ case ACPI_TYPE_INTEGER:
+ strncpy(ptr, (u8 *)&element->integer.value, sizeof(u64));
ptr[sizeof(u64)] = 0;
- } else
- *ptr = 0; /* don't have value */
+
+ 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
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 <[email protected]>
---
drivers/acpi/battery.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index b39b84b8f3ae..9f4c31484adc 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 <[email protected]>");
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;
@@ -440,12 +442,12 @@ static int extract_package(struct acpi_battery *battery,
switch (element->type) {
case ACPI_TYPE_STRING:
- strscpy(ptr, element->string.pointer, 32);
+ strscpy(ptr, element->string.pointer, MAX_STRING_LENGTH);
break;
case ACPI_TYPE_BUFFER:
strscpy(ptr, element->buffer.pointer,
- min_t(u32, element->buffer.length + 1, 32));
+ min_t(u32, element->buffer.length + 1, MAX_STRING_LENGTH));
break;
case ACPI_TYPE_INTEGER:
--
2.30.2
When encountering a string bigger than the destination buffer (32 bytes),
the string is not properly NUL-terminated, causing buffer overreads later.
This for example happens on the Inspiron 3505, where the battery
model name is larger than 32 bytes, which leads to sysfs showing
the model name together with the serial number string (which is
NUL-terminated and thus prevents worse).
Fix this by using strscpy() which ensures that the result is
always NUL-terminated.
Signed-off-by: Armin Wolf <[email protected]>
---
drivers/acpi/battery.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index f4badcdde76e..fb64bd217d82 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -440,7 +440,7 @@ static int extract_package(struct acpi_battery *battery,
if (element->type == ACPI_TYPE_STRING ||
element->type == ACPI_TYPE_BUFFER)
- strncpy(ptr, element->string.pointer, 32);
+ strscpy(ptr, element->string.pointer, 32);
else if (element->type == ACPI_TYPE_INTEGER) {
strncpy(ptr, (u8 *)&element->integer.value,
sizeof(u64));
--
2.30.2
On Sat, Jan 14, 2023 at 9:51 AM Armin Wolf <[email protected]> wrote:
>
> If the buffer containing string data is not NUL-terminated
> (which is perfectly legal according to the ACPI specification),
> the acpi battery driver might not honor its length.
Note that this is about extracting package entries of type ACPI_TYPE_BUFFER.
And please spell ACPI in capitals.
> 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.
OK
> Also use '\0' instead of a plain 0.
Why? It's a u8, not a char.
> Signed-off-by: Armin Wolf <[email protected]>
> ---
> drivers/acpi/battery.c | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index fb64bd217d82..9f6daa9f2010 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -438,15 +438,24 @@ static int extract_package(struct acpi_battery *battery,
> if (offsets[i].mode) {
> u8 *ptr = (u8 *)battery + offsets[i].offset;
I would add
u32 len = 32;
>
> - if (element->type == ACPI_TYPE_STRING ||
> - element->type == ACPI_TYPE_BUFFER)
> + switch (element->type) {
And here I would do
case ACPI_TYPE_BUFFER:
if (len > element->buffer.length + 1)
len = element->buffer.length + 1;
fallthrough;
case ACPI_TYPE_STRING:
strscpy(ptr, element->buffer.pointer, len);
break;
case ACPI_TYPE_INTEGER:
and so on.
> + case ACPI_TYPE_STRING:
> strscpy(ptr, element->string.pointer, 32);
> - else if (element->type == ACPI_TYPE_INTEGER) {
> - strncpy(ptr, (u8 *)&element->integer.value,
> - sizeof(u64));
> +
> + break;
> + case ACPI_TYPE_BUFFER:
> + strscpy(ptr, element->buffer.pointer,
> + min_t(u32, element->buffer.length + 1, 32));
> +
> + break;
> + case ACPI_TYPE_INTEGER:
> + strncpy(ptr, (u8 *)&element->integer.value, sizeof(u64));
> ptr[sizeof(u64)] = 0;
> - } else
> - *ptr = 0; /* don't have value */
> +
> + break;
> + default:
> + *ptr = '\0'; /* don't have value */
> + }
> } else {
> int *x = (int *)((u8 *)battery + offsets[i].offset);
> *x = (element->type == ACPI_TYPE_INTEGER) ?
> --
On Sat, Jan 14, 2023 at 9:51 AM Armin Wolf <[email protected]> wrote:
>
> Currently, strncpy() and manual NUL-termination is used
> when copying integers. Switch to strscpy() which takes care
> of NUL-terminating the result.
>
> Signed-off-by: Armin Wolf <[email protected]>
> ---
> drivers/acpi/battery.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 9f6daa9f2010..b39b84b8f3ae 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -449,8 +449,7 @@ static int extract_package(struct acpi_battery *battery,
>
> break;
> case ACPI_TYPE_INTEGER:
> - strncpy(ptr, (u8 *)&element->integer.value, sizeof(u64));
> - ptr[sizeof(u64)] = 0;
> + strscpy(ptr, (u8 *)&element->integer.value, sizeof(u64) + 1);
>
> break;
> default:
> --
This can be folded into the previous patch.
Otherwise you're patching your own patch which isn't particularly clean.
Am 17.01.23 um 15:42 schrieb Rafael J. Wysocki:
> On Sat, Jan 14, 2023 at 9:51 AM Armin Wolf <[email protected]> wrote:
>> If the buffer containing string data is not NUL-terminated
>> (which is perfectly legal according to the ACPI specification),
>> the acpi battery driver might not honor its length.
> Note that this is about extracting package entries of type ACPI_TYPE_BUFFER.
>
> And please spell ACPI in capitals.
>
>> 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.
> OK
>
>> Also use '\0' instead of a plain 0.
> Why? It's a u8, not a char.
>
>> Signed-off-by: Armin Wolf <[email protected]>
>> ---
>> drivers/acpi/battery.c | 23 ++++++++++++++++-------
>> 1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>> index fb64bd217d82..9f6daa9f2010 100644
>> --- a/drivers/acpi/battery.c
>> +++ b/drivers/acpi/battery.c
>> @@ -438,15 +438,24 @@ static int extract_package(struct acpi_battery *battery,
>> if (offsets[i].mode) {
>> u8 *ptr = (u8 *)battery + offsets[i].offset;
> I would add
>
> u32 len = 32;
>
>> - if (element->type == ACPI_TYPE_STRING ||
>> - element->type == ACPI_TYPE_BUFFER)
>> + switch (element->type) {
> And here I would do
>
> case ACPI_TYPE_BUFFER:
> if (len > element->buffer.length + 1)
> len = element->buffer.length + 1;
>
> fallthrough;
> case ACPI_TYPE_STRING:
> strscpy(ptr, element->buffer.pointer, len);
> break;
> case ACPI_TYPE_INTEGER:
>
> and so on.
But wouldn't this cause the ACPI string object to be accessed the wrong way
(buffer.pointer instead of string.pointer)?
Armin Wolf
>> + case ACPI_TYPE_STRING:
>> strscpy(ptr, element->string.pointer, 32);
>> - else if (element->type == ACPI_TYPE_INTEGER) {
>> - strncpy(ptr, (u8 *)&element->integer.value,
>> - sizeof(u64));
>> +
>> + break;
>> + case ACPI_TYPE_BUFFER:
>> + strscpy(ptr, element->buffer.pointer,
>> + min_t(u32, element->buffer.length + 1, 32));
>> +
>> + break;
>> + case ACPI_TYPE_INTEGER:
>> + strncpy(ptr, (u8 *)&element->integer.value, sizeof(u64));
>> ptr[sizeof(u64)] = 0;
>> - } else
>> - *ptr = 0; /* don't have value */
>> +
>> + break;
>> + default:
>> + *ptr = '\0'; /* don't have value */
>> + }
>> } else {
>> int *x = (int *)((u8 *)battery + offsets[i].offset);
>> *x = (element->type == ACPI_TYPE_INTEGER) ?
>> --
On Tue, Jan 17, 2023 at 10:01 PM Armin Wolf <[email protected]> wrote:
>
> Am 17.01.23 um 15:42 schrieb Rafael J. Wysocki:
>
> > On Sat, Jan 14, 2023 at 9:51 AM Armin Wolf <[email protected]> wrote:
> >> If the buffer containing string data is not NUL-terminated
> >> (which is perfectly legal according to the ACPI specification),
> >> the acpi battery driver might not honor its length.
> > Note that this is about extracting package entries of type ACPI_TYPE_BUFFER.
> >
> > And please spell ACPI in capitals.
> >
> >> 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.
> > OK
> >
> >> Also use '\0' instead of a plain 0.
> > Why? It's a u8, not a char.
> >
> >> Signed-off-by: Armin Wolf <[email protected]>
> >> ---
> >> drivers/acpi/battery.c | 23 ++++++++++++++++-------
> >> 1 file changed, 16 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> >> index fb64bd217d82..9f6daa9f2010 100644
> >> --- a/drivers/acpi/battery.c
> >> +++ b/drivers/acpi/battery.c
> >> @@ -438,15 +438,24 @@ static int extract_package(struct acpi_battery *battery,
> >> if (offsets[i].mode) {
> >> u8 *ptr = (u8 *)battery + offsets[i].offset;
> > I would add
> >
> > u32 len = 32;
> >
> >> - if (element->type == ACPI_TYPE_STRING ||
> >> - element->type == ACPI_TYPE_BUFFER)
> >> + switch (element->type) {
> > And here I would do
> >
> > case ACPI_TYPE_BUFFER:
> > if (len > element->buffer.length + 1)
> > len = element->buffer.length + 1;
> >
> > fallthrough;
> > case ACPI_TYPE_STRING:
> > strscpy(ptr, element->buffer.pointer, len);
> > break;
> > case ACPI_TYPE_INTEGER:
> >
> > and so on.
>
> But wouldn't this cause the ACPI string object to be accessed the wrong way
> (buffer.pointer instead of string.pointer)?
I meant string.pointer, like in the original code, but this doesn't
matter really, because the value of the pointer is the same.
On Sat, Jan 14, 2023 at 9:51 AM Armin Wolf <[email protected]> wrote:
>
> When encountering a string bigger than the destination buffer (32 bytes),
> the string is not properly NUL-terminated, causing buffer overreads later.
>
> This for example happens on the Inspiron 3505, where the battery
> model name is larger than 32 bytes, which leads to sysfs showing
> the model name together with the serial number string (which is
> NUL-terminated and thus prevents worse).
>
> Fix this by using strscpy() which ensures that the result is
> always NUL-terminated.
>
> Signed-off-by: Armin Wolf <[email protected]>
> ---
> drivers/acpi/battery.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index f4badcdde76e..fb64bd217d82 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -440,7 +440,7 @@ static int extract_package(struct acpi_battery *battery,
>
> if (element->type == ACPI_TYPE_STRING ||
> element->type == ACPI_TYPE_BUFFER)
> - strncpy(ptr, element->string.pointer, 32);
> + strscpy(ptr, element->string.pointer, 32);
> else if (element->type == ACPI_TYPE_INTEGER) {
> strncpy(ptr, (u8 *)&element->integer.value,
> sizeof(u64));
> --
Applied as 6.3 material, thanks!
Please do not include this one in the next version of the series.