| Message ID | 9445401f-7cfb-bbb5-e25c-28f578efa212@valinux.co.jp (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | ACPI: tables: Fix NULL dereference by acpi_os_map_memory() | expand |
On Wed, Jul 26, 2023 at 6:53 AM Kiwamu Okabe <okabe@valinux.co.jp> wrote: > > The Infer static analyzer https://fbinfer.com/ reports following > NULL poinster dereference by the acpi_os_map_memory() function. > I believe this patch does fix the issue without any panic. Please demonstrate to me that the NULL pointer dereference can actually happen in this code. Thanks! > Signed-off-by: Kiwamu Okabe <okabe@valinux.co.jp> > --- > drivers/acpi/tables.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c > index 8ab0a82b4da4..ae7b7343bacf 100644 > --- a/drivers/acpi/tables.c > +++ b/drivers/acpi/tables.c > @@ -717,6 +717,9 @@ acpi_table_initrd_override(struct acpi_table_header *existing_table, > while (table_offset + ACPI_HEADER_SIZE <= all_tables_size) { > table = acpi_os_map_memory(acpi_tables_addr + table_offset, > ACPI_HEADER_SIZE); > + if (WARN_ON(!table)) { > + return AE_OK; > + } > if (table_offset + table->length > all_tables_size) { > acpi_os_unmap_memory(table, ACPI_HEADER_SIZE); > WARN_ON(1); > @@ -772,6 +775,9 @@ static void __init acpi_table_initrd_scan(void) > while (table_offset + ACPI_HEADER_SIZE <= all_tables_size) { > table = acpi_os_map_memory(acpi_tables_addr + table_offset, > ACPI_HEADER_SIZE); > + if (WARN_ON(!table)) { > + return; > + } > if (table_offset + table->length > all_tables_size) { > acpi_os_unmap_memory(table, ACPI_HEADER_SIZE); > WARN_ON(1); > -- > 2.39.2 >
Dear Rafael, On 7/26/23 23:35, Rafael J. Wysocki wrote: > On Wed, Jul 26, 2023 at 6:53 AM Kiwamu Okabe <okabe@valinux.co.jp> wrote: >> >> The Infer static analyzer https://fbinfer.com/ reports following >> NULL poinster dereference by the acpi_os_map_memory() function. >> I believe this patch does fix the issue without any panic. > > Please demonstrate to me that the NULL pointer dereference can > actually happen in this code. The `acpi_table_initrd_override()` function potentially occurs NULL pointer dereference on `table->length`, ``` while (table_offset + ACPI_HEADER_SIZE <= all_tables_size) { table = acpi_os_map_memory(acpi_tables_addr + table_offset, ACPI_HEADER_SIZE); if (table_offset + table->length > all_tables_size) { acpi_os_unmap_memory(table, ACPI_HEADER_SIZE); WARN_ON(1); return AE_OK; } ``` because the acpi_os_map_memory() function potentially returns NULL, ``` void __iomem __ref *acpi_os_map_iomem(acpi_physical_address phys, acpi_size size) { --snip-- map = kzalloc(sizeof(*map), GFP_KERNEL); if (!map) { mutex_unlock(&acpi_ioremap_lock); return NULL; } --snip-- void *__ref acpi_os_map_memory(acpi_physical_address phys, acpi_size size) { return (void *)acpi_os_map_iomem(phys, size); } ``` because the `kzalloc()` potentially returns NULL. And also, the other code have NULL check to call `acpi_os_map_memory()` as following. ``` subtable_header = acpi_os_map_memory(address, sizeof(*subtable_header)); if (!subtable_header) return -ENOMEM; --snip-- rsdp = acpi_os_map_memory(rsdp_address, sizeof(struct acpi_table_rsdp)); if (!rsdp) { return_ACPI_STATUS(AE_NO_MEMORY); } ``` >> Signed-off-by: Kiwamu Okabe <okabe@valinux.co.jp> >> --- >> drivers/acpi/tables.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c >> index 8ab0a82b4da4..ae7b7343bacf 100644 >> --- a/drivers/acpi/tables.c >> +++ b/drivers/acpi/tables.c >> @@ -717,6 +717,9 @@ acpi_table_initrd_override(struct acpi_table_header *existing_table, >> while (table_offset + ACPI_HEADER_SIZE <= all_tables_size) { >> table = acpi_os_map_memory(acpi_tables_addr + table_offset, >> ACPI_HEADER_SIZE); >> + if (WARN_ON(!table)) { >> + return AE_OK; >> + } >> if (table_offset + table->length > all_tables_size) { >> acpi_os_unmap_memory(table, ACPI_HEADER_SIZE); >> WARN_ON(1); >> @@ -772,6 +775,9 @@ static void __init acpi_table_initrd_scan(void) >> while (table_offset + ACPI_HEADER_SIZE <= all_tables_size) { >> table = acpi_os_map_memory(acpi_tables_addr + table_offset, >> ACPI_HEADER_SIZE); >> + if (WARN_ON(!table)) { >> + return; >> + } >> if (table_offset + table->length > all_tables_size) { >> acpi_os_unmap_memory(table, ACPI_HEADER_SIZE); >> WARN_ON(1); >> -- >> 2.39.2 >> > Best Regards,
diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index 8ab0a82b4da4..ae7b7343bacf 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -717,6 +717,9 @@ acpi_table_initrd_override(struct acpi_table_header *existing_table, while (table_offset + ACPI_HEADER_SIZE <= all_tables_size) { table = acpi_os_map_memory(acpi_tables_addr + table_offset, ACPI_HEADER_SIZE); + if (WARN_ON(!table)) { + return AE_OK; + } if (table_offset + table->length > all_tables_size) { acpi_os_unmap_memory(table, ACPI_HEADER_SIZE); WARN_ON(1); @@ -772,6 +775,9 @@ static void __init acpi_table_initrd_scan(void) while (table_offset + ACPI_HEADER_SIZE <= all_tables_size) { table = acpi_os_map_memory(acpi_tables_addr + table_offset, ACPI_HEADER_SIZE); + if (WARN_ON(!table)) { + return; + } if (table_offset + table->length > all_tables_size) { acpi_os_unmap_memory(table, ACPI_HEADER_SIZE); WARN_ON(1);
The Infer static analyzer https://fbinfer.com/ reports following NULL poinster dereference by the acpi_os_map_memory() function. I believe this patch does fix the issue without any panic. Signed-off-by: Kiwamu Okabe <okabe@valinux.co.jp> --- drivers/acpi/tables.c | 6 ++++++ 1 file changed, 6 insertions(+)