From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 91CC1D1812A for ; Mon, 14 Oct 2024 15:35:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:MIME-Version:References:In-Reply-To:Subject:Cc:To:From: Message-ID:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=NrLQgo92JQMdwXslSg/nSLULlZq2XbUA9sELH2jp/KM=; b=DLIAKWGIDaIGDGLkPjE9FY97Ko b1FRq0cGDsRNWcmw5YJNMpve6Zv1/LDC/kgYMUxwuEFgnI9pdfRsco6B/OT+HTYgv9tpyMpuk7t/n isZlSBMi3rm+A4dWaZR9SBQVdBhOnz71Mmw26BBsRYgSGWJa3HCekHF909MtYX6oxfOVaYbTPHTNt hybq1Csr5ndIwb2tf3KbzF/+TphMpiGzesLTmsR7dmYeZHtBagOSU4eWyzXtiMzbmtQclrWhVCiwA 2s2sx1ibHruMQEyS80x9LwVfNlEC6XQmOdAGqMlYGWu5MhtBNeiz0fVo6utKNrGqFmS3bNLrXKmyT wa5BFPDQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1t0N6Y-00000005gHE-1Htz; Mon, 14 Oct 2024 15:35:30 +0000 Received: from nyc.source.kernel.org ([2604:1380:45d1:ec00::3]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1t0M1g-00000005So2-29j3 for linux-arm-kernel@lists.infradead.org; Mon, 14 Oct 2024 14:26:26 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 71E84A415D2; Mon, 14 Oct 2024 14:26:14 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 05B8EC4CECF; Mon, 14 Oct 2024 14:26:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1728915983; bh=htdeMYqzZVZ1IWH4JC74fCThAQafoLo4kvCLkF7l+Y4=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=OeUihHqb+rrucxLsD4KW4jlYjtEDlaTjvP8pzGsCOu0ve24Oj0oKsPiNE2LlPvFWy HdvJF1n26PPZ9q88Nr3CWzRABDvjUihArskESDdkKnuEuDaaoqOULk4uNmx4FCEZGs 8sywtn6RhmW98Hw6yz7SIqjQKodJ40Cf2Kv81AKWypkYazwsBDD7kqir7z2iUccnqb PmGHmsARMGBHemXWvv9REOPBkJxp/MHy9FgTBVTecVkS3yUOAIfBZWpYNDr9Lr/Tfd fdsR9LehB6jfBWP5ZAcQakA045u8IaKWNhZj0nzhXhfRb+Q08Hx/c1yLa5fcwg6teu GjH8W4oNxxTtQ== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1t0M1c-003Q59-PO; Mon, 14 Oct 2024 15:26:20 +0100 Date: Mon, 14 Oct 2024 15:26:19 +0100 Message-ID: <868quq69ro.wl-maz@kernel.org> From: Marc Zyngier To: Zheng Zengkai Cc: , , , , , , , , , , Subject: Re: [PATCH v2] ACPI: GTDT: Tighten the check for the array of platform timer structures In-Reply-To: References: <20241012085343.6594-1-zhengzengkai@huawei.com> <8734l1usxe.wl-maz@kernel.org> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/29.4 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: zhengzengkai@huawei.com, lpieralisi@kernel.org, guohanjun@huawei.com, sudeep.holla@arm.com, mark.rutland@arm.com, rafael@kernel.org, lenb@kernel.org, daniel.lezcano@linaro.org, tglx@linutronix.de, linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20241014_072624_728101_7BF95960 X-CRM114-Status: GOOD ( 34.93 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, 14 Oct 2024 13:22:26 +0100, Zheng Zengkai wrote: >=20 > Hi Marc, >=20 > =E5=9C=A8 2024/10/13 1:34, Marc Zyngier =E5=86=99=E9=81=93: > > On Sat, 12 Oct 2024 09:53:43 +0100, > > Zheng Zengkai wrote: > >> As suggested by Marc and Lorenzo, first we need to check whether the > >> platform_timer entry pointer is within gtdt bounds (< gtdt_end) before > >> de-referencing what it points at to detect the length of the platform > >> timer struct and then check that the length of current platform_timer > >> struct is within gtdt_end too. Now next_platform_timer() only checks > >> against gtdt_end for the entry of subsequent platform timer without > >> checking the length of it and will not report error if the check faile= d. > >>=20 > >> Add check against table length (gtdt_end) for each element of platform > >> timer array in acpi_gtdt_init() early, making sure that both their ent= ry > >> and length actually fit in the table. > >>=20 > >> For the first platform timer, keep the check against the end of the > >> acpi_table_gtdt struct, it is unnecessary for subsequent platform time= r. > > Really? > >=20 > >> Suggested-by: Marc Zyngier > >> Suggested-by: Lorenzo Pieralisi > >> Signed-off-by: Zheng Zengkai > >> --- > >> Changes in v2: > >> - Check against gtdt_end for both entry and len of each array element > >>=20 > >> v1: https://lore.kernel.org/all/20241010144703.113728-1-zhengzengkai@h= uawei.com/ > >> --- > >> drivers/acpi/arm64/gtdt.c | 19 +++++++++++++++---- > >> 1 file changed, 15 insertions(+), 4 deletions(-) > >>=20 > >> diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c > >> index c0e77c1c8e09..f5f62643899d 100644 > >> --- a/drivers/acpi/arm64/gtdt.c > >> +++ b/drivers/acpi/arm64/gtdt.c > >> @@ -157,6 +157,8 @@ int __init acpi_gtdt_init(struct acpi_table_header= *table, > >> { > >> void *platform_timer; > >> struct acpi_table_gtdt *gtdt; > >> + struct acpi_gtdt_header *gh; > >> + void *struct_end; > >> gtdt =3D container_of(table, struct acpi_table_gtdt, header); > >> acpi_gtdt_desc.gtdt =3D gtdt; > >> @@ -177,11 +179,20 @@ int __init acpi_gtdt_init(struct acpi_table_head= er *table, > >> } > >> platform_timer =3D (void *)gtdt + gtdt->platform_timer_offset; > >> - if (platform_timer < (void *)table + sizeof(struct acpi_table_gtdt))= { > >> - pr_err(FW_BUG "invalid timer data.\n"); > >> - return -EINVAL; > >> + struct_end =3D (void *)table + sizeof(struct acpi_table_gtdt); > >> + for (int i =3D 0; i < gtdt->platform_timer_count; i++) { > >> + gh =3D platform_timer; > >> + if (((i =3D=3D 0 && platform_timer >=3D struct_end) || i !=3D 0) && > > Why is only index 0 checked against the end of the table? Shouldn't > > int be an invariant that all timer descriptions must not intersect > > with the non-variable part of the GTDT table? >=20 >=20 > AFAICS, after checking against the end of the acpi_table_gtdt struct for = the > first platform timer, the subsequent platform_timer pointer value > computed via "platform_timer + gh->length" will also pass the check, > as the gh->length is of u16 type. But this is something that isn't obvious to the casual reader of this code, and you want to keep validation code simple and localised, with as few separate cases as you can. This isn't performance critical code, and there is nothing to be gained by "optimising" this. >=20 >=20 > >> + platform_timer < acpi_gtdt_desc.gtdt_end && > >> + platform_timer + gh->length <=3D acpi_gtdt_desc.gtdt_end) { > > Surely, assuming that length isn't zero, if the last term is true, the > > previous one also is? And what if it is 0? >=20 >=20 > Agree , the length should also be checked against 0, > but I think we should first check the platform_timer entry pointer, > then check the size of the same platform_timer structure, > not check them in the opposite order. Correct, that's something that needs fixing. Run with it. M. --=20 Without deviation from the norm, progress is not possible.