linux-kernel - Re: [PATCH] ACPI/PPTT: fix off-by-one error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0ii9HLfqfgcp=1qRRX6M1yThf7ZPNkSLVc5GGFhv=N-Lg@mail.gmail.com>
Date: Wed, 7 May 2025 18:12:40 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Jeremy Linton <jeremy.linton@....com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, "Heyne, Maximilian" <mheyne@...zon.de>, 
	"stable@...r.kernel.org" <stable@...r.kernel.org>, Len Brown <lenb@...nel.org>, 
	Sudeep Holla <sudeep.holla@....com>, Ard Biesheuvel <ardb@...nel.org>, 
	Catalin Marinas <catalin.marinas@....com>, 
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ACPI/PPTT: fix off-by-one error

On Wed, May 7, 2025 at 5:51 PM Jeremy Linton <jeremy.linton@....com> wrote:
>
> On 5/7/25 10:42 AM, Rafael J. Wysocki wrote:
> > On Wed, May 7, 2025 at 5:25 PM Jeremy Linton <jeremy.linton@....com> wrote:
> >>
> >> Hi,
> >>
> >> On 5/6/25 8:13 AM, Heyne, Maximilian wrote:
> >>> Commit 7ab4f0e37a0f ("ACPI PPTT: Fix coding mistakes in a couple of
> >>> sizeof() calls") corrects the processer entry size but unmasked a longer
> >>> standing bug where the last entry in the structure can get skipped due
> >>> to an off-by-one mistake if the last entry ends exactly at the end of
> >>> the ACPI subtable.
> >>>
> >>> The error manifests for instance on EC2 Graviton Metal instances with
> >>>
> >>>     ACPI PPTT: PPTT table found, but unable to locate core 63 (63)
> >>>     [...]
> >>>     ACPI: SPE must be homogeneous
> >>>
> >>> Fixes: 2bd00bcd73e5 ("ACPI/PPTT: Add Processor Properties Topology Table parsing")
> >>> Cc: stable@...r.kernel.org
> >>> Signed-off-by: Maximilian Heyne <mheyne@...zon.de>
> >>> ---
> >>>    drivers/acpi/pptt.c | 4 ++--
> >>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
> >>> index f73ce6e13065d..4364da90902e5 100644
> >>> --- a/drivers/acpi/pptt.c
> >>> +++ b/drivers/acpi/pptt.c
> >>> @@ -231,7 +231,7 @@ static int acpi_pptt_leaf_node(struct acpi_table_header *table_hdr,
> >>>                             sizeof(struct acpi_table_pptt));
> >>>        proc_sz = sizeof(struct acpi_pptt_processor);
> >>
> >> This isn't really right, it should be struct acpi_subtable_header, then
> >> once the header is safe, pull the length from it.
> >>
> >> But then, really if we are trying to fix the original bug that the table
> >> could be shorter than the data in it suggests, the struct
> >> acpi_pptt_processor length plus its resources needs to be checked once
> >> the subtype is known to be a processor node.
> >>
> >> Otherwise the original sizeof * change isn't really fixing anything.
> >
> > Sorry, what sense did it make to do
> >
> > proc_sz = sizeof(struct acpi_pptt_processor *);
> >
> > here?  As much as proc_sz = 0 I suppose?
>
> No, I agree, I think the original checks were simplified along the way
> to that. It wasn't 'right' either.
>
> The problem is that there are three subtypes of which processor is only
> one, and that struct acpi_pptt_processor doesn't necessarily reflect the
> actual size of the processor structure in the table because it has
> optional private resources tagged onto the end.

Right.

> So if the bug being fixed is that the length check is validating that
> the table length is less than the data in the table, that's still a
> problem because its only validating the processor node without resources.

Admittedly, it is not my code, but I understand this check as a
termination condition for the loop: If there's not enough space in the
table to hold a thing that I'm looking for, I may as well bail out.

> AKA the return is still potentially returning a pointer to a structure
> which may not be entirely contained in the table.

Right, but this check should be made anyway before comparing
cpu_node->parent to node_entry, when it is known to be a CPU entry
because otherwise why bother.

Roughly something like this:

proc_sz = sizeof(struct acpi_pptt_processor);

while ((unsigned long)entry + entry->length <= table_end) {
        if (entry->type == ACPI_PPTT_TYPE_PROCESSOR &&
            entry->length >= proc_sz &&
            cpu_node->parent == node_entry)
                return 0;
...
}