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

[Zheng Zengkai]

Hi Marc,

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