From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id BEB6F1917FB; Wed, 7 May 2025 16:38:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1746635914; cv=none; b=jFykYXXNoxHB7bN8U2sNqV+xM0ZgyleMlf71ul891B3MwyVemWC216s2N1AkS68qonWQk4/CCerqU7OwE6msrOsJRLwXBVJj2cW43+s5QjxG+iKUqvy6uXWQOAY6qv/SygjFa1oqmp1GKRlcx8bu+IqaFaj7O1EDGpqh816iU1Y= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1746635914; c=relaxed/simple; bh=pPRh0xmgP+OZka+vsxhg1EUacKsXWMQf1hLtzjiubm0=; h=Message-ID:Date:MIME-Version:Subject:From:To:Cc:References: In-Reply-To:Content-Type; b=fmw8tzE1NFxx8YwxWoTmvkc/Eg/lDt4EYhoFwXvkoO4/tDh/frCfLtkB5udtnPGaoQNt5IbEXdxyY0T3v+hw+ofoZ1eVJfmjFS3795J9dE2lcxVn22y2OFT5ZVSVoCr3H8H74bEdJUWZrWfxouYEN7jDjzNi5lwT2uaaQndS+KU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 055B216F2; Wed, 7 May 2025 09:38:22 -0700 (PDT) Received: from [192.168.20.57] (usa-sjc-mx-foss1.foss.arm.com [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D0D163F58B; Wed, 7 May 2025 09:38:29 -0700 (PDT) Message-ID: Date: Wed, 7 May 2025 11:38:27 -0500 Precedence: bulk X-Mailing-List: linux-acpi@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] ACPI/PPTT: fix off-by-one error From: Jeremy Linton To: "Rafael J. Wysocki" Cc: "Heyne, Maximilian" , "stable@vger.kernel.org" , Len Brown , Sudeep Holla , Ard Biesheuvel , Catalin Marinas , "linux-acpi@vger.kernel.org" , "linux-kernel@vger.kernel.org" References: <20250506-draco-taped-15f475cd@mheyne-amazon> <214c2a2d-e0ea-4ec6-9925-05e39319e813@arm.com> <1911d3b6-f328-40a6-aa03-cde3d79554de@arm.com> Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit On 5/7/25 11:31 AM, Jeremy Linton wrote: > On 5/7/25 11:12 AM, Rafael J. Wysocki wrote: >> On Wed, May 7, 2025 at 5:51 PM Jeremy Linton >> wrote: >>> >>> On 5/7/25 10:42 AM, Rafael J. Wysocki wrote: >>>> On Wed, May 7, 2025 at 5:25 PM Jeremy Linton >>>> 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@vger.kernel.org >>>>>> Signed-off-by: Maximilian Heyne >>>>>> --- >>>>>>     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. > > Right, but then there is a clarity because really its walking the > table+subtypes looking for the cpu node. Exiting early because its not > big enough for a cpu node makes sense but you still need the cpu node > check to avoid a variation on the original bug. > > > >> >> Roughly something like this: >> >> proc_sz = sizeof(struct acpi_pptt_processor); >> >> while ((unsigned long)entry + entry->length <= table_end) { > > Here your reading the entry, without knowing its long enough. For the > leaf check just using struct acpi_pptt_processor is fine, but for the > acpi_find_processor_node(): > > proc_sz = sizeof(struct acpi_subtable_type); Although, maybe I just wrote code that justifies using acpi_pptt_processor here because the entry->num_of_priv_resources length check isn't being made without it. So ok, use proc_sz = sizeof(struct acpi_subtable_type) and assume that we don't care if the subtable type is less than proc_sz. > > while ((unsigned long)entry + proc_sz <= table_end) { >  if (entry->type == ACPI_PPTT_TYPE_PROCESSOR && >  entry->length == sizeof(struct acpi_pptt_processor) + >     entry->number_of_priv_resources * sizeof(u32) && >  entry + entry->length <= table_end && >  acpi_pptt_leaf_node(...)) >     return (...)entry; > > > Although at this point the while loops entry + proc_sz could just be < > table_end under the assumption that entry->length will be > 0 but > whichever makes more sense. > > >