Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] State corruption and list_head dangling pointers on NFIT table parsing error. - [High] Out-of-bounds read and undefined behavior via bit shift in add_platform_cap(). -- --- Patch [4]: [PATCH v2 4/5] ACPI: NFIT: validate control region extent --- commit 0bc177f6743d37dbd5255c0f7a268009d181d358 Author: Pengpeng Hou <[email protected]> ACPI: NFIT: validate control region extent This patch modifies sizeof_dcr() to ensure that the advertised subtable size is large enough to contain all remaining fields, preventing a potential out-of-bounds read during the memory copy in add_dcr(). It computes the required size based on the windows count and validates it against the provided subtable length. > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c > index bf4ddc56494e3..84c70f1941c57 100644 > --- a/drivers/acpi/nfit/core.c > +++ b/drivers/acpi/nfit/core.c > @@ -817,12 +817,20 @@ EXPORT_SYMBOL_GPL(nfit_get_smbios_id); > */ > static size_t sizeof_dcr(struct acpi_nfit_control_region *dcr) > { > + size_t size; > + > if (dcr->header.length < offsetof(struct acpi_nfit_control_region, > window_size)) > return 0; > if (dcr->windows) > - return sizeof(*dcr); > - return offsetof(struct acpi_nfit_control_region, window_size); > + size = sizeof(*dcr); > + else > + size = offsetof(struct acpi_nfit_control_region, window_size); > + > + if (size > dcr->header.length) > + return 0; [Severity: High] This isn't a bug introduced by this patch, but returning 0 here can trigger a state corruption and memory leak in the caller's error path. If sizeof_dcr() returns 0, add_dcr() returns false, which causes add_table() to fail and jump to the error path in acpi_nfit_init(): drivers/acpi/nfit/core.c:acpi_nfit_init() { ... while (!IS_ERR_OR_NULL(data)) data = add_table(acpi_desc, &prev, data, end); if (IS_ERR(data)) { dev_dbg(dev, "nfit table parsing error: %ld\n", PTR_ERR(data)); rc = PTR_ERR(data); goto out_unlock; } ... out_unlock: mutex_unlock(&acpi_desc->init_mutex); return rc; } When jumping to out_unlock, existing list elements that were temporarily moved to the stack variable prev are not spliced back to acpi_desc. They become orphaned, and their list_head pointers are left dangling to invalid stack memory. Does this need to be cleaned up or spliced back on error? > + > + return size; > } > > static bool add_dcr(struct acpi_nfit_desc *acpi_desc, [Severity: High] This is a pre-existing issue, but is there an out-of-bounds read and undefined behavior when parsing the Capabilities table? In add_table(), a table is only validated to be at least 4 bytes long (sizeof(struct acpi_nfit_header)). However, add_platform_cap() accesses fields beyond the header: drivers/acpi/nfit/core.c:add_platform_cap() { ... mask = (1 << (pcap->highest_capability + 1)) - 1; acpi_desc->platform_cap = pcap->capabilities & mask; ... } If a malformed Capabilities subtable is provided with a length of 4 bytes, could this result in an out-of-bounds read of pcap->highest_capability and pcap->capabilities? Additionally, if pcap->highest_capability is 30 or greater, does the bit shift result in signed integer overflow or shift-out-of-bounds undefined behavior? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
