Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [High] Out-of-bounds read in add_dcr() due to unchecked sizeof_dcr() calculation. - [High] Out-of-bounds read in add_platform_cap() due to insufficient subtable length validation. - [High] ACPI notify handlers leak when DIMM registration fails, leading to Use-After-Free. - [High] Time-Of-Check to Time-Of-Use (TOCTOU) Heap Buffer Overflow in add_idt() and add_flush(). -- --- Patch [3]: [PATCH v2 3/5] ACPI: NFIT: validate interleave and flush array extents --- commit 5f0d0836358fb697d133a3d8f0839ed8e9b8fd8d Author: Pengpeng Hou <[email protected]> ACPI: NFIT: validate interleave and flush array extents Interleave and flush subtables carry a fixed header followed by arrays whose element counts come from firmware. sizeof_idt() and sizeof_flush() derive the copy size from those counts but do not require the result to fit in the subtable's advertised length. > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c > index f68edfe649528..bf4ddc56494e3 100644 > --- a/drivers/acpi/nfit/core.c > +++ b/drivers/acpi/nfit/core.c > @@ -882,9 +882,16 @@ static bool add_bdw(struct acpi_nfit_desc *acpi_desc, > > static size_t sizeof_idt(struct acpi_nfit_interleave *idt) > { > + size_t size; > + > if (idt->header.length < sizeof(*idt)) > return 0; > - return sizeof(*idt) + sizeof(u32) * idt->line_count; > + > + size = struct_size(idt, line_offset, idt->line_count); > + if (size > idt->header.length) > + return 0; > + > + return size; > } [Severity: High] This is a pre-existing issue, but does fetching the size fields directly from mapped memory here and in sizeof_flush() create a time-of-check to time-of-use race condition? Looking at add_idt(), the size is evaluated twice: static bool add_idt(struct acpi_nfit_desc *acpi_desc, ...) { ... nfit_idt = devm_kzalloc(dev, sizeof(*nfit_idt) + sizeof_idt(idt), GFP_KERNEL); ... memcpy(nfit_idt->idt, idt, sizeof_idt(idt)); ... } If firmware or a hypervisor alters line_count concurrently between the allocation and the memcpy, could this lead to a heap buffer overflow? [ ... ] > > static size_t sizeof_flush(struct acpi_nfit_flush_address *flush) > { > + size_t size; > + > if (flush->header.length < sizeof(*flush)) > return 0; > - return struct_size(flush, hint_address, flush->hint_count); > + > + size = struct_size(flush, hint_address, flush->hint_count); > + if (size > flush->header.length) > + return 0; > + > + return size; > } [Severity: High] This isn't a bug introduced by this patch, but while we are improving bounds checking, could sizeof_dcr() also read out of bounds? If a control region subtable has a short length but dcr->windows is non-zero, sizeof_dcr() returns the full size: static size_t sizeof_dcr(struct acpi_nfit_control_region *dcr) { ... if (dcr->windows) return sizeof(*dcr); ... } Will this cause memcmp() to read out of bounds in add_dcr() when the derived size is not validated against the actual subtable length? [Severity: High] This is a pre-existing issue, but does add_platform_cap() similarly lack length validation? When add_table() processes ACPI_NFIT_TYPE_CAPABILITIES, it unconditionally dispatches the function: case ACPI_NFIT_TYPE_CAPABILITIES: if (!add_platform_cap(acpi_desc, table)) return err; break; If the table length is short (e.g., 4 bytes), could the dereferences in add_platform_cap() cause an out-of-bounds read? static bool add_platform_cap(struct acpi_nfit_desc *acpi_desc, struct acpi_nfit_capabilities *pcap) { ... mask = (1 << (pcap->highest_capability + 1)) - 1; acpi_desc->platform_cap = pcap->capabilities & mask; ... } [Severity: High] This isn't a bug introduced by this patch, but does acpi_nfit_register_dimms() leak notify handlers on failure? During dimm registration, if memory allocation fails for nvdimm, the function returns early: static int acpi_nfit_register_dimms(struct acpi_nfit_desc *acpi_desc) { ... nvdimm = __nvdimm_create(...); if (!nvdimm) return -ENOMEM; ... return devm_add_action_or_reset(acpi_desc->dev, shutdown_dimm_notify, acpi_desc); } Since shutdown_dimm_notify is only queued for cleanup at the very end of the function, do we leave dangling notify handlers registered if the loop aborts early? Could a subsequent event then trigger a use-after-free on the freed nfit_mem structures? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
