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

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