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 AA773CF2572 for ; Sat, 12 Oct 2024 17:35:49 +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-Type:MIME-Version: References:In-Reply-To:Subject:Cc:To:From:Message-ID:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=m4jFs4LZyOC9opmfW6CH6a5HRXKsf5B8811GYmTGIWk=; b=3Tv04AFoqZBj2I2zlm6+wfWOuy Nr5fpt6e3ASfe8mXt8QsMLYwDK+zpRLCul0f3iY1aowod1r2WvHRn7ALIiYwdF3CwSdpzKYPMwf+r ia+eQo/q9JF5Hs7s53uDaZ1RixPV4wl+6Q4fgXMi18+HQQrQRb8xImvkgWwsSKoCBfjh/9CeZpLQc 8XA79e2OnNGPSIrVWEgGggdH+sA5um7DhvtlTvk9z6JgjGQF8kF2eaA3Y1dJHuOnKuiqynp4zRrDG XByOJeIVJ4D4MErJErc9076TPQOxlAiErKo9TeBDZVf735yhLCvQLFnrxKSGQcUo02pdfu7T7m7H8 IePz16wQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98 #2 (Red Hat Linux)) id 1szg1h-00000001YVh-0uzx; Sat, 12 Oct 2024 17:35:37 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.98 #2 (Red Hat Linux)) id 1szg0I-00000001YNn-3wFU for linux-arm-kernel@lists.infradead.org; Sat, 12 Oct 2024 17:34:12 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 2412E5C53A4; Sat, 12 Oct 2024 17:34:05 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id ECD0DC4CEC6; Sat, 12 Oct 2024 17:34:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1728754449; bh=WxZIcq+iQPIXa3sSTYV2bGd68tYlonJCyY0WBpuj98k=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=peKxWGrwzr8FxSfY/QjwmIXCleiviobuhU6FBPH6JimLjIRSOKW8nsVz+QhJogUNl NW3dr6z8Amgs2NA2o5ldu5ceftEkVPF9gK/ecQWuDjcptM/rPIuTaGMrbr0qMkyKu0 fA+oIn1vbGG8lC4HwII7COJkw0grH4C8pe37PZpLBoGx/jgqPCEbLq77yCO1iNDCUD Wo/Ae+6RroBPYFERuRNReTYTk5QQTURrPjAM7QXBAZa4ePgtJljzkYAvk1Oht6MU/5 Vj/IC1g/DEaVri3Zxk2CAhr7rX9kaUtJAyDD6D9lGU0FYfF5YJjae/QAm21ZktYync SqEmyp0aj0Gcg== Received: from sofa.misterjones.org ([185.219.108.64] helo=wait-a-minute.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 1szg0E-002uES-98; Sat, 12 Oct 2024 18:34:06 +0100 Date: Sat, 12 Oct 2024 18:34:05 +0100 Message-ID: <8734l1usxe.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: <20241012085343.6594-1-zhengzengkai@huawei.com> References: <20241012085343.6594-1-zhengzengkai@huawei.com> 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 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII 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-20241012_103411_420009_12918FDD X-CRM114-Status: GOOD ( 37.42 ) 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 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 failed. > > Add check against table length (gtdt_end) for each element of platform > timer array in acpi_gtdt_init() early, making sure that both their entry > and length actually fit in the table. > > For the first platform timer, keep the check against the end of the > acpi_table_gtdt struct, it is unnecessary for subsequent platform timer. Really? > > 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 > > v1: https://lore.kernel.org/all/20241010144703.113728-1-zhengzengkai@huawei.com/ > --- > drivers/acpi/arm64/gtdt.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > 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 = container_of(table, struct acpi_table_gtdt, header); > acpi_gtdt_desc.gtdt = gtdt; > @@ -177,11 +179,20 @@ int __init acpi_gtdt_init(struct acpi_table_header *table, > } > > platform_timer = (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 = (void *)table + sizeof(struct acpi_table_gtdt); > + for (int i = 0; i < gtdt->platform_timer_count; i++) { > + gh = platform_timer; > + if (((i == 0 && platform_timer >= struct_end) || i != 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? > + platform_timer < acpi_gtdt_desc.gtdt_end && > + platform_timer + gh->length <= 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? Again, you cannot trust *anything* you find in the ACPI table. > + platform_timer += gh->length; You are also reinventing the wheel, and repeating some of the worse constructs in this code. It would be much better to build on (and augment) the existing primitives to make the code *readable* instead of being this pointer soup. Believe it or not, there is some value in abstracting things. I came up with the patchlet below, very lightly tested on my Synquacer. It may not be optimal, but given that this is used exactly once per boot, I'm sure we can afford a few extra comparisons. It makes the iterator robust, and then uses that to implement the checks. M. diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c index c0e77c1c8e09d..dca814183cc5c 100644 --- a/drivers/acpi/arm64/gtdt.c +++ b/drivers/acpi/arm64/gtdt.c @@ -36,15 +36,24 @@ struct acpi_gtdt_descriptor { static struct acpi_gtdt_descriptor acpi_gtdt_desc __initdata; -static inline __init void *next_platform_timer(void *platform_timer) +static __init bool platform_timer_valid(void *platform_timer) { struct acpi_gtdt_header *gh = platform_timer; - platform_timer += gh->length; - if (platform_timer < acpi_gtdt_desc.gtdt_end) - return platform_timer; + return (gh->length != 0 && + platform_timer >= (void *)(acpi_gtdt_desc.gtdt + 1) && + platform_timer + gh->length <= acpi_gtdt_desc.gtdt_end); +} + +static __init void *next_platform_timer(void *platform_timer) +{ + struct acpi_gtdt_header *gh = platform_timer; - return NULL; + if (!platform_timer_valid(platform_timer) || + !platform_timer_valid(platform_timer + gh->length)) + return NULL; + + return platform_timer + gh->length; } #define for_each_platform_timer(_g) \ @@ -155,8 +164,9 @@ bool __init acpi_gtdt_c3stop(int type) int __init acpi_gtdt_init(struct acpi_table_header *table, int *platform_timer_count) { - void *platform_timer; + void *platform_timer, *tmp; struct acpi_table_gtdt *gtdt; + int cnt = 0; gtdt = container_of(table, struct acpi_table_gtdt, header); acpi_gtdt_desc.gtdt = gtdt; @@ -177,7 +187,12 @@ int __init acpi_gtdt_init(struct acpi_table_header *table, } platform_timer = (void *)gtdt + gtdt->platform_timer_offset; - if (platform_timer < (void *)table + sizeof(struct acpi_table_gtdt)) { + for (tmp = platform_timer; + tmp && platform_timer_valid(tmp); + tmp = next_platform_timer(tmp)) + cnt++; + + if (cnt != gtdt->platform_timer_count) { pr_err(FW_BUG "invalid timer data.\n"); return -EINVAL; } -- Without deviation from the norm, progress is not possible.