Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] If `acpi_nfit_init()` encounters an error during table parsing (e.g., memory allocation failure) or detects unsupported deletions, it returns without restoring the remaining elements left in the stack-allocated `prev` lists. This corrupts the driver's active state by permanently losing tracking of those elements. - [High] Pre-existing out-of-bounds reads in `add_dcr`, `add_flush`, and `add_idt` due to missing `header.length` validation against computed sizes. -- --- Patch [2]: [PATCH v2 2/5] ACPI: NFIT: accept SPA structures with an unused location cookie --- commit 1d01437bf3a7475500c2f72bf23fa46edef22d6c Author: Pengpeng Hou <[email protected]> ACPI: NFIT: accept SPA structures with an unused location cookie ACPI 6.4 extended the System Physical Address Range Structure from 56 to 64 bytes by appending an eight-byte location cookie. The cookie-valid flag describes whether that field contains usable data; it does not select the structure length. > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c > index 4428adb6a1ab6..f68edfe649528 100644 > --- a/drivers/acpi/nfit/core.c > +++ b/drivers/acpi/nfit/core.c > @@ -705,9 +705,20 @@ int nfit_spa_type(struct acpi_nfit_system_address *spa) > > static size_t sizeof_spa(struct acpi_nfit_system_address *spa) > { > + size_t legacy_size = offsetof(struct acpi_nfit_system_address, > + location_cookie); > + size_t size = spa->header.length; > + > + if (size == sizeof(*spa)) > + return size; > + > + if (size != legacy_size) > + return 0; > + > if (spa->flags & ACPI_NFIT_LOCATION_COOKIE_VALID) > - return sizeof(*spa); > - return sizeof(*spa) - 8; > + return 0; > + > + return size; > } [Severity: High] This is a pre-existing issue, but since you are fixing the length logic for SPA structures here, does similar code in sizeof_idt() and sizeof_flush() need bounds checking against the header length? In drivers/acpi/nfit/core.c:sizeof_idt(): static size_t sizeof_idt(struct acpi_nfit_interleave *idt) { if (idt->header.length < sizeof(*idt)) return 0; return sizeof(*idt) + sizeof(u32) * idt->line_count; } Can this lead to an out-of-bounds read if a malformed ACPI table provides an idt->line_count that makes the computed size larger than idt->header.length? The add_idt() function uses this unvalidated computed size directly for memcpy() and memcmp() on the ACPI buffer. Similar logic is present in sizeof_flush() and add_flush() with flush->hint_count. [ ... ] > @@ -716,23 +727,26 @@ static bool add_spa(struct acpi_nfit_desc *acpi_desc, > { > struct device *dev = acpi_desc->dev; > struct nfit_spa *nfit_spa; > + size_t size = sizeof_spa(spa); > > - if (spa->header.length != sizeof_spa(spa)) > + if (!size) > return false; [Severity: High] This isn't a bug introduced by this patch, but looking at acpi_nfit_init() which manages the prev list passed into these add_*() functions, is there a state corruption and memory leak on the error path? If a hotplug event removes regions and acpi_nfit_check_deletions() fails, it returns -ENXIO and jumps to out_unlock: rc = acpi_nfit_check_deletions(acpi_desc, &prev); if (rc) goto out_unlock; ... out_unlock: mutex_unlock(&acpi_desc->init_mutex); return rc; Since prev is allocated on the stack in acpi_nfit_init(), does jumping to out_unlock without splicing the remaining un-matched entries back onto the acpi_desc lists permanently lose track of those items? It appears this would drop them from the driver's tracking lists while leaving their list node pointers dangling to a destroyed stack frame. > list_for_each_entry(nfit_spa, &prev->spas, list) { > - if (memcmp(nfit_spa->spa, spa, sizeof_spa(spa)) == 0) { > + if (sizeof_spa(nfit_spa->spa) != size) > + continue; > + > + if (memcmp(nfit_spa->spa, spa, size) == 0) { > list_move_tail(&nfit_spa->list, &acpi_desc->spas); > return true; > } > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
