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] [day] [month] [year] [list]

Message-ID: <4333e8bd-89ed-f1cc-2f1d-ec539362a704@huawei.com>
Date: Tue, 15 Oct 2024 22:50:14 +0800
From: Zheng Zengkai <zhengzengkai@...wei.com>
To: Marc Zyngier <maz@...nel.org>
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

Hi Marc,

在 2024/10/14 22:26, Marc Zyngier 写道:
> 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.


OK


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


OK,  I will do that.

Thanks!