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

[Marc Zyngier]

On Tue, 08 Oct 2024 15:04:52 +0100,
Zheng Zengkai <zhengzengkai@xxxxxxxxxx> wrote:
> 
> 
> 在 2024/10/8 16:55, Marc Zyngier 写道:
> > On Tue, 08 Oct 2024 09:24:29 +0100,
> > Zheng Zengkai <zhengzengkai@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >> Changes in v2:
> >> - initialize 'ret' in gtdt_sbsa_gwdt_init() to silence build warning
> >> 
> >> v1: https://lore.kernel.org/all/20240930030716.179992-1-zhengzengkai@xxxxxxxxxx/
> >> ---
> >>   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.

	M.

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