Messages in this thread | | | Subject | Re: [PATCH 2/4] ACPI: battery: Fix buffer overread if not NUL-terminated | From | Armin Wolf <> | Date | Tue, 17 Jan 2023 22:01:17 +0100 |
| |
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) ? >> --
| |