linux-kernel - Re: [PATCH v2] ACPI: GTDT: Tighten the check for the array of platform timer structures

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <868quq69ro.wl-maz@kernel.org>
Date: Mon, 14 Oct 2024 15:26:19 +0100
From: Marc Zyngier <maz@...nel.org>
To: Zheng Zengkai <zhengzengkai@...wei.com>
Cc: <lpieralisi@...nel.org>,
	<guohanjun@...wei.com>,
	<sudeep.holla@....com>,
	<mark.rutland@....com>,
	<rafael@...nel.org>,
	<lenb@...nel.org>,
	<daniel.lezcano@...aro.org>,
	<tglx@...utronix.de>,
	<linux-acpi@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] ACPI: GTDT: Tighten the check for the array of platform timer structures

On Mon, 14 Oct 2024 13:22:26 +0100,
Zheng Zengkai <zhengzengkai@...wei.com> wrote:
> 
> Hi Marc,
> 
> 在 2024/10/13 1:34, Marc Zyngier 写道:
> > On Sat, 12 Oct 2024 09:53:43 +0100,
> > Zheng Zengkai <zhengzengkai@...wei.com> 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 <maz@...nel.org>
> >> Suggested-by: Lorenzo Pieralisi <lpieralisi@...nel.org>
> >> Signed-off-by: Zheng Zengkai <zhengzengkai@...wei.com>
> >> ---
> >> 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?
> 
> 
> 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.

> 
> 
> >> +			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?
> 
> 
> 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.

-- 
Without deviation from the norm, progress is not possible.