linux-kernel - Re: [PATCH v2] ACPI: GTDT: simplify acpi_gtdt

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: <57e9adb8-a34a-6d63-24b8-4ad0abb74bf9@huawei.com>
Date: Tue, 8 Oct 2024 22:04:52 +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: simplify acpi_gtdt_init() implementation


在 2024/10/8 16:55, Marc Zyngier 写道:
> On Tue, 08 Oct 2024 09:24:29 +0100,
> Zheng Zengkai <zhengzengkai@...wei.com> wrote:
>> According to GTDT Table Structure of ACPI specification, the result of
>> expression '(void *)gtdt + gtdt->platform_timer_offset' will be same
>> with the expression '(void *)table + sizeof(struct acpi_table_gtdt)'
> There is no such language in the spec. It simply says "Offset to the
> Platform Timer Structure[] array from the start of this table".
OK, I mean that in current code, the condition of this check is redundant.
>> in function acpi_gtdt_init(), so the condition of the "invalid timer
>> data" check will never be true, remove the EINVAL error check branch
>> and change to void return type for acpi_gtdt_init() to simplify the
>> function implementation and error handling by callers.
> And ACPI tables are well known to be always correct, right?
Not always, check is needed, but should be changed.
>> Besides, after commit c2743a36765d ("clocksource: arm_arch_timer: add
>> GTDT support for memory-mapped timer"), acpi_gtdt_init() currently will
>> not be called with parameter platform_timer_count set to NULL and we
>> can explicitly initialize the integer variable which is used for storing
>> the number of platform timers by caller to zero, so there is no need to
>> do null pointer check for platform_timer_count in acpi_gtdt_init(),
>> remove it to make code a bit more concise.
>>
>> Signed-off-by: Zheng Zengkai <zhengzengkai@...wei.com>
>> ---
>> Changes in v2:
>> - initialize 'ret' in gtdt_sbsa_gwdt_init() to silence build warning
>>
>> v1: https://lore.kernel.org/all/20240930030716.179992-1-zhengzengkai@huawei.com/
>> ---
>>   drivers/acpi/arm64/gtdt.c            | 31 +++++++---------------------
>>   drivers/clocksource/arm_arch_timer.c |  6 ++----
>>   include/linux/acpi.h                 |  2 +-
>>   3 files changed, 11 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c
>> index c0e77c1c8e09..7fe27c0edde7 100644
>> --- a/drivers/acpi/arm64/gtdt.c
>> +++ b/drivers/acpi/arm64/gtdt.c
>> @@ -147,45 +147,30 @@ bool __init acpi_gtdt_c3stop(int type)
>>    * @table:			The pointer to GTDT table.
>>    * @platform_timer_count:	It points to a integer variable which is used
>>    *				for storing the number of platform timers.
>> - *				This pointer could be NULL, if the caller
>> - *				doesn't need this info.
>> - *
>> - * Return: 0 if success, -EINVAL if error.
>>    */
>> -int __init acpi_gtdt_init(struct acpi_table_header *table,
>> +void __init acpi_gtdt_init(struct acpi_table_header *table,
>>   			  int *platform_timer_count)
>>   {
>> -	void *platform_timer;
>>   	struct acpi_table_gtdt *gtdt;
>>   
>>   	gtdt = container_of(table, struct acpi_table_gtdt, header);
>>   	acpi_gtdt_desc.gtdt = gtdt;
>>   	acpi_gtdt_desc.gtdt_end = (void *)table + table->length;
>>   	acpi_gtdt_desc.platform_timer = NULL;
>> -	if (platform_timer_count)
>> -		*platform_timer_count = 0;
>>   
>>   	if (table->revision < 2) {
>>   		pr_warn("Revision:%d doesn't support Platform Timers.\n",
>>   			table->revision);
>> -		return 0;
>> +		return;
>>   	}
>>   
>>   	if (!gtdt->platform_timer_count) {
>>   		pr_debug("No Platform Timer.\n");
>> -		return 0;
>> +		return;
>>   	}
>>   
>> -	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;
>> -	}
>> -	acpi_gtdt_desc.platform_timer = platform_timer;
>> -	if (platform_timer_count)
>> -		*platform_timer_count = gtdt->platform_timer_count;
>> -
>> -	return 0;
>> +	acpi_gtdt_desc.platform_timer = (void *)gtdt + gtdt->platform_timer_offset;
> And now you are trusting something that potentially points to some
> unexpected location, blindly using it. It is bad enough that the
> current checks are pretty poor (no check against the end of the
> table for the first timer entry), but you are making it worse.
>
> 	M.

Can I use the second and third bytes (the length) of platform timer 
structure to

check against the end of the table ?

Thanks!