[PATCH v2 0/5] ACPI: NFIT: harden subtable extent validation

[PATCH v2 0/5] ACPI: NFIT: harden subtable extent validation

[Pengpeng Hou]

NFIT parsing has several independent extent contracts: the outer table
must contain each advertised subtable, and variable or versioned records
must fit inside that subtable.  The original patch mixed those contracts
and also derived the SPA structure length from the location-cookie-valid
flag.

Split the checks by record type.  Patch 1 validates the generic subtable
header and outer extent.  Patch 2 accepts both defined SPA layouts while
treating the cookie flag as a validity bit, not a size selector.  Patches
3 and 4 validate the variable arrays and the control-region extent.
Patch 5 bounds the 32-bit capability mask without rejecting future
firmware that advertises a higher capability index.

Changes since v1:
https://lore.kernel.org/all/20260722041701.21078-1-pengpeng@iscas.ac.cn/
- split the generic and type-specific contracts into separate patches
- handle both 56-byte and 64-byte SPA structures
- add the control-region extent check identified during review
- clamp the implemented capability mask instead of rejecting future bits
- describe the exact fields protected by each check

The series was reviewed statically against the current tree. I did not
run an NFIT firmware fault-injection test.

Pengpeng Hou (5):
  ACPI: NFIT: validate subtable extents before dispatch
  ACPI: NFIT: accept SPA structures with an unused location cookie
  ACPI: NFIT: validate interleave and flush array extents
  ACPI: NFIT: validate control region extent
  ACPI: NFIT: bound the platform capability mask

 drivers/acpi/nfit/core.c | 77 ++++++++++++++++++++++++++++++++--------
 1 file changed, 62 insertions(+), 15 deletions(-)

base-commit: f5bbbfec59b4e2fb7520a91de3df8a6174325d6a

-- 
2.50.1 (Apple Git-155)

[PATCH v2 1/5] ACPI: NFIT: validate subtable extents before dispatch

[Pengpeng Hou]

add_table() checks only that the subtable cursor is before the end of
the NFIT before reading the two-field subtable header. A cursor with
fewer than sizeof(struct acpi_nfit_header) bytes remaining therefore
makes the header read cross the mapped table.

The function also advances by the firmware-provided length without
checking that the advertised subtable fits in the enclosing NFIT. A
malformed length can move the parser beyond the table and make the next
iteration read unrelated memory.

Require a complete header and bind the advertised length to both the
header size and the bytes remaining in the NFIT before dispatching the
subtable.

Fixes: b94d5230d06e ("libnvdimm, nfit: initial libnvdimm infrastructure and NFIT support")
Assisted-by: Codex:gpt-5
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/acpi/nfit/core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index cb771d9cadb2..4428adb6a1ab 100644
--- 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;
 
 	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;
 	}
 
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2 1/5] ACPI: NFIT: validate subtable extents before dispatch

[sashiko-bot]

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 <pengpeng@iscas.ac.cn>

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/20260813150948.17239-1-pengpeng@iscas.ac.cn?part=1

[PATCH v2 2/5] ACPI: NFIT: accept SPA structures with an unused location cookie

[Pengpeng Hou]

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.

sizeof_spa() instead derives the expected length from the flag. It
therefore rejects a valid 64-byte ACPI 6.4 structure when the cookie is
present but not valid. It can also compare 64 bytes against a previously
saved 56-byte structure without first checking that the saved allocation
has the same length.

Accept either the legacy 56-byte layout or the ACPI 6.4 64-byte layout.
Require the cookie-valid flag to be clear for the legacy layout, and
compare saved entries only when their validated lengths match.

Fixes: e9cfd259c6d3 ("ACPI: NFIT: Fix support for variable 'SPA' structure size")
Assisted-by: Codex:gpt-5
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/acpi/nfit/core.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 4428adb6a1ab..f68edfe64952 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;
 }
 
 static bool add_spa(struct acpi_nfit_desc *acpi_desc,
@@ -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;
 
 	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;
 		}
 	}
 
-	nfit_spa = devm_kzalloc(dev, sizeof(*nfit_spa) + sizeof_spa(spa),
-			GFP_KERNEL);
+	nfit_spa = devm_kzalloc(dev, sizeof(*nfit_spa) + size, GFP_KERNEL);
 	if (!nfit_spa)
 		return false;
 	INIT_LIST_HEAD(&nfit_spa->list);
-	memcpy(nfit_spa->spa, spa, sizeof_spa(spa));
+	memcpy(nfit_spa->spa, spa, size);
 	list_add_tail(&nfit_spa->list, &acpi_desc->spas);
 	dev_dbg(dev, "spa index: %d type: %s\n",
 			spa->range_index,
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2 2/5] ACPI: NFIT: accept SPA structures with an unused location cookie

[sashiko-bot]

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 <pengpeng@iscas.ac.cn>

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/20260813150948.17239-1-pengpeng@iscas.ac.cn?part=2

[PATCH v2 3/5] ACPI: NFIT: validate interleave and flush array extents

[Pengpeng Hou]

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.

A short subtable with a large line_count or hint_count can consequently
make add_idt() or add_flush() copy beyond the subtable. The outer NFIT
extent check cannot catch this because the following subtable bytes are
still inside the enclosing ACPI table.

Use struct_size() for both variable arrays and reject a derived size that
exceeds the current subtable.

Fixes: b94d5230d06e ("libnvdimm, nfit: initial libnvdimm infrastructure and NFIT support")
Assisted-by: Codex:gpt-5
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/acpi/nfit/core.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index f68edfe64952..bf4ddc56494e 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;
 }
 
 static bool add_idt(struct acpi_nfit_desc *acpi_desc,
@@ -921,9 +928,16 @@ static bool add_idt(struct acpi_nfit_desc *acpi_desc,
 
 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;
 }
 
 static bool add_flush(struct acpi_nfit_desc *acpi_desc,
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2 3/5] ACPI: NFIT: validate interleave and flush array extents

[sashiko-bot]

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 <pengpeng@iscas.ac.cn>

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/20260813150948.17239-1-pengpeng@iscas.ac.cn?part=3

[PATCH v2 4/5] ACPI: NFIT: validate control region extent

[Pengpeng Hou]

A control region with no block windows may legally stop before the
window-size fields, so sizeof_dcr() first reads the windows count from
the shortened fixed portion. When the count is nonzero, however, the
helper returns sizeof(*dcr) without checking that the advertised
subtable contains the remaining fields.

add_dcr() then copies that full size. A malformed subtable that ends
immediately after a nonzero windows count can therefore make the copy
read beyond the control region.

Compute the required size from the windows count and require that size
to fit in the advertised subtable before returning it.

Fixes: b94d5230d06e ("libnvdimm, nfit: initial libnvdimm infrastructure and NFIT support")
Assisted-by: Codex:gpt-5
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/acpi/nfit/core.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index bf4ddc56494e..84c70f1941c5 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;
+
+	return size;
 }
 
 static bool add_dcr(struct acpi_nfit_desc *acpi_desc,
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2 4/5] ACPI: NFIT: validate control region extent

[sashiko-bot]

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 <pengpeng@iscas.ac.cn>

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/20260813150948.17239-1-pengpeng@iscas.ac.cn?part=4

[PATCH v2 5/5] ACPI: NFIT: bound the platform capability mask

[Pengpeng Hou]

The platform capability subtable parser reads highest_capability and
capabilities without first requiring the fixed structure to be present.
It also shifts a signed integer by highest_capability + 1. Firmware can
therefore trigger an out-of-bounds read with a short subtable and an
undefined shift with a value at or above 31.

Require the fixed structure before reading either field. For capability
indices that cover all bits representable by the current u32 field, use
U32_MAX. This ignores capabilities beyond the implemented 32-bit field
rather than rejecting future firmware that advertises a higher index.

Fixes: 06e8ccdab15f ("acpi: nfit: Add support for detect platform CPU cache flush on power loss")
Assisted-by: Codex:gpt-5
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/acpi/nfit/core.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 84c70f1941c5..7638d4dc8c6d 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -987,7 +987,14 @@ static bool add_platform_cap(struct acpi_nfit_desc *acpi_desc,
 	struct device *dev = acpi_desc->dev;
 	u32 mask;
 
-	mask = (1 << (pcap->highest_capability + 1)) - 1;
+	if (pcap->header.length < sizeof(*pcap))
+		return false;
+
+	if (pcap->highest_capability >= 31)
+		mask = U32_MAX;
+	else
+		mask = (1U << (pcap->highest_capability + 1)) - 1;
+
 	acpi_desc->platform_cap = pcap->capabilities & mask;
 	dev_dbg(dev, "cap: %#x\n", acpi_desc->platform_cap);
 	return true;
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2 5/5] ACPI: NFIT: bound the platform capability mask

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The `acpi_nfit_init` function fails to restore list elements on error paths, leading to memory leaks and complete state corruption for existing NVDIMM devices during hot-updates.
--

--- Patch [5]: [PATCH v2 5/5] ACPI: NFIT: bound the platform capability mask ---
commit bbbfd46d3c5bf4ec32e49479ec84620dc0f64973
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

ACPI: NFIT: bound the platform capability mask

The platform capability subtable parser reads highest_capability and
capabilities without first requiring the fixed structure to be present.
This commit adds checks to prevent out-of-bounds reads and undefined shifts.

> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 84c70f1941c57..7638d4dc8c6da 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -987,7 +987,14 @@ static bool add_platform_cap(struct acpi_nfit_desc *acpi_desc,
>  	struct device *dev = acpi_desc->dev;
>  	u32 mask;
>  
> -	mask = (1 << (pcap->highest_capability + 1)) - 1;
> +	if (pcap->header.length < sizeof(*pcap))
> +		return false;

[Severity: High]
This is a pre-existing issue, but does adding this new early return expose
a state corruption bug during hot-updates?

In acpi_nfit_init(), existing NFIT resources are moved to a local stack
variable before parsing:

acpi_nfit_init() {
        ...
        list_cut_position(&prev.spas, &acpi_desc->spas,
                          acpi_desc->spas.prev);
        ...

If the newly added capability length check fails, it appears the table
parsing aborts:

        if (IS_ERR(data)) {
                dev_dbg(dev, "nfit table parsing error: %ld\n", PTR_ERR(data));
                rc = PTR_ERR(data);
                goto out_unlock;
        }

Which jumps directly to the exit label:

out_unlock:
        mutex_unlock(&acpi_desc->init_mutex);
        return rc;
}

Since the elements in the local prev lists are never restored to
acpi_desc on this error path, do these devm-managed items become permanently
leaked with dangling list pointers when prev goes out of scope?

> +
> +	if (pcap->highest_capability >= 31)
> +		mask = U32_MAX;
> +	else
> +		mask = (1U << (pcap->highest_capability + 1)) - 1;
> +
>  	acpi_desc->platform_cap = pcap->capabilities & mask;
>  	dev_dbg(dev, "cap: %#x\n", acpi_desc->platform_cap);
>  	return true;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813150948.17239-1-pengpeng@iscas.ac.cn?part=5