Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
Pre-existing issues: - [High] sizeof_spa() accesses spa->flags before validating the subtable length, causing an out-of-bounds read. - [High] add_platform_cap() reads capability fields without verifying the subtable length, causing an out-of-bounds read. - [High] Early termination in acpi_nfit_init() leaves internal device lists stranded on the stack, corrupting the device state and leaking resources. - [High] sizeof_idt() calculates a size that can exceed the verified subtable length, causing an out-of-bounds read in add_idt(). - [High] sizeof_dcr() can return a size exceeding the verified subtable length when dcr->windows is set, causing an out-of-bounds read. - [Medium] Bitwise shift operation in add_platform_cap() can cause undefined behavior if highest_capability >= 31. -- --- Patch [1]: [PATCH v2 1/5] ACPI: NFIT: validate subtable extents before dispatch --- commit 7124d3c49e2df0b214c3037c798664d5e4c0f3d3 Author: Pengpeng Hou <[email protected]> ACPI: NFIT: validate subtable extents before dispatch This commit strengthens NFIT table parsing by requiring a complete header and validating that the advertised subtable length fits within the remaining bytes in the NFIT before dispatching it. > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c > --- a/drivers/acpi/nfit/core.c > +++ b/drivers/acpi/nfit/core.c > @@ -963,14 +963,18 @@ static void *add_table(struct acpi_nfit_desc *acpi_desc, > struct device *dev = acpi_desc->dev; > struct acpi_nfit_header *hdr; > void *err = ERR_PTR(-ENOMEM); > + size_t table_len; > > if (table >= end) > return NULL; > + table_len = end - table; > + if (table_len < sizeof(*hdr)) > + return NULL; > [Severity: High] This isn't a bug introduced by this patch, but returning NULL here can cause device state corruption during runtime updates. If add_table() returns NULL early while parsing an ACPI update in acpi_nfit_init(), the function jumps to out_unlock: drivers/acpi/nfit/core.c:acpi_nfit_init() { ... goto out_unlock; ... } At that point, it exits without restoring nodes from the stack list prev back to acpi_desc, which permanently empties the device's operational lists. Does acpi_nfit_init() need to be updated to handle this early return safely? > hdr = table; > - if (!hdr->length) { > - dev_warn(dev, "found a zero length table '%d' parsing nfit\n", > - hdr->type); > + if (hdr->length < sizeof(*hdr) || hdr->length > table_len) { > + dev_warn(dev, "invalid table length %u for type %u parsing > nfit\n", > + hdr->length, hdr->type); > return NULL; > } [Severity: High] This is a pre-existing issue, but while we are strengthening generic table length checks here, does sizeof_spa() also need bounds checking? If a malformed table provides a System Physical Address (SPA) subtable with a length of 4 or 5, add_table() will accept it because hdr->length >= 4. However, sizeof_spa() in drivers/acpi/nfit/core.c reads spa->flags before verifying the subtable length is at least 8 bytes: drivers/acpi/nfit/core.c:sizeof_spa() { if (spa->flags & ACPI_NFIT_LOCATION_COOKIE_VALID) ... } Can this lead to an out-of-bounds read if the table length is too small? [Severity: High] This is a pre-existing issue, but does add_platform_cap() similarly need length validation? A capabilities subtable might pass the generic length check here, but add_platform_cap() in drivers/acpi/nfit/core.c reads capabilities without verifying the subtable length is at least 16 bytes: drivers/acpi/nfit/core.c:add_platform_cap() { mask = (1 << (pcap->highest_capability + 1)) - 1; acpi_desc->platform_cap = pcap->capabilities & mask; } Could this result in an out-of-bounds read? [Severity: Medium] This is also a pre-existing issue, but in the same code snippet above, if highest_capability is 31 or greater, would the bitwise shift: drivers/acpi/nfit/core.c:add_platform_cap() { mask = (1 << (pcap->highest_capability + 1)) - 1; } invoke undefined behavior by shifting a 32-bit signed integer by 32 or more? [Severity: High] This is a pre-existing issue, but does sizeof_idt() also need length bounds checking to prevent out-of-bounds reads? If a malformed table has a small hdr->length but a large line_count, sizeof_idt() in drivers/acpi/nfit/core.c calculates a required size ignoring hdr->length: drivers/acpi/nfit/core.c:sizeof_idt() { return sizeof(*idt) + sizeof(u32) * idt->line_count; } Then add_idt() passes this unvalidated size to memcpy: drivers/acpi/nfit/core.c:add_idt() { memcpy(nfit_idt->idt, idt, sizeof_idt(idt)); } Could this copy past the mapped ACPI table boundary? [Severity: High] This is a pre-existing issue, but could sizeof_dcr() return a size exceeding the verified subtable length? If dcr->windows is set, sizeof_dcr() in drivers/acpi/nfit/core.c blindly returns 80 bytes: drivers/acpi/nfit/core.c:sizeof_dcr() { if (dcr->windows) return sizeof(*dcr); } Then add_dcr() copies the memory: drivers/acpi/nfit/core.c:add_dcr() { memcpy(nfit_dcr->dcr, dcr, sizeof_dcr(dcr)); } If the firmware provided less than 80 bytes for the control region table, does this cause an out-of-bounds read? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
