diff mbox series

[2/4] ACPI: battery: Fix buffer overread if not NUL-terminated

Message ID 20230114085053.72059-3-W_Armin@gmx.de
State New
Headers
Series ACPI: battery: Fix various string handling issues |

Commit Message

Armin Wolf Jan. 14, 2023, 8:50 a.m. UTC 
  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 <W_Armin@gmx.de>
---
 drivers/acpi/battery.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

--
2.30.2

Comments

Rafael J. Wysocki Jan. 17, 2023, 2:42 p.m. UTC | #1 
On Sat, Jan 14, 2023 at 9:51 AM Armin Wolf <W_Armin@gmx.de> 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 <W_Armin@gmx.de>
> ---
>  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) ?
> --
Armin Wolf Jan. 17, 2023, 9:01 p.m. UTC | #2 
Am 17.01.23 um 15:42 schrieb Rafael J. Wysocki:

> On Sat, Jan 14, 2023 at 9:51 AM Armin Wolf <W_Armin@gmx.de> 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 <W_Armin@gmx.de>
>> ---
>>   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) ?
>> --
Rafael J. Wysocki Jan. 18, 2023, 3 p.m. UTC | #3 
On Tue, Jan 17, 2023 at 10:01 PM Armin Wolf <W_Armin@gmx.de> wrote:
>
> Am 17.01.23 um 15:42 schrieb Rafael J. Wysocki:
>
> > On Sat, Jan 14, 2023 at 9:51 AM Armin Wolf <W_Armin@gmx.de> 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 <W_Armin@gmx.de>
> >> ---
> >>   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.
diff mbox series

Patch

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