Re: [PATCH v2] ACPI: GTDT: simplify acpi_gtdt_init() implementation

[Zheng Zengkai <zhengzengkai@huawei.com>]

在 2024/10/9 21:10, Lorenzo Pieralisi 写道:
> On Wed, Oct 09, 2024 at 12:33:35PM +0100, Marc Zyngier wrote:
>> On Tue, 08 Oct 2024 15:04:52 +0100,
>> Zheng Zengkai <zhengzengkai@huawei.com> wrote:
>>>
>>> 在 2024/10/8 16:55, Marc Zyngier 写道:
>>>> On Tue, 08 Oct 2024 09:24:29 +0100,
>>>> Zheng Zengkai <zhengzengkai@huawei.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.
>> That's not my reading if it. Where do you see another validity check
>> that makes this one superfluous?
>>
>>>>> 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.
>> You are not changing it, you are getting rid of it, and I don't see
>> you replacing it with anything else.
>>
>>>>> 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@huawei.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 ?
>> That's how it is supposed to be done indeed.
> AFAICS I think first we need to check whether the platform_timer pointer
> is within gtdt bounds (< gtdt_end) before de-referencing what it points
> at to detect the (first) GT entry length and check that it is within
> gtdt_end too. We do that only in next_platform_timer() for subsequent
> GT blocks.
>
> I agree with Marc, current check is fine, we should add to it, not
> remove it.


OK,  we need to tighten the check, not get rid of it.

Thanks!


> Thanks,
> Lorenzo
> .
>