Re: [PATCH] ACPI: FPDT: break out of the loop if record length is zero

[Vasily Khoruzhick]

On Mon, Sep 25, 2023 at 10:03 PM Zhang, Rui <rui.zhang@xxxxxxxxx> wrote:
>
> On Mon, 2023-09-25 at 14:40 -0700, Vasily Khoruzhick wrote:
> > Buggy BIOSes may have zero-length records in FPDT, table, as a result
> s/FPDT, table/FPDT table
>
>
> > fpdt_process_subtable() spins in eternal loop.
> >
> > Break out of the loop if record length is zero.
> >
> >
> > Fixes: d1eb86e59be0 ("ACPI: tables: introduce support for FPDT
> > table")
> > Cc: stable@xxxxxxxxxxxxxxx
> >
> > Signed-off-by: Vasily Khoruzhick <anarsoul@xxxxxxxxx>
>
> Is there a bugzilla or something where such a buggy BIOS is reported?

I'm not aware of a bug filed a kernel bugzilla, however I found
mentions of the issue online:
https://forum.proxmox.com/threads/acpi-fpdt-duplicate-resume-performance-record-found.114530/

> > ---
> >  drivers/acpi/acpi_fpdt.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/acpi/acpi_fpdt.c b/drivers/acpi/acpi_fpdt.c
> > index a2056c4c8cb7..53d8f9601a55 100644
> > --- a/drivers/acpi/acpi_fpdt.c
> > +++ b/drivers/acpi/acpi_fpdt.c
> > @@ -194,6 +194,11 @@ static int fpdt_process_subtable(u64 address,
> > u32 subtable_type)
> >                 record_header = (void *)subtable_header + offset;
> >                 offset += record_header->length;
> >
> > +               if (!record_header->length) {
> > +                       pr_info(FW_BUG "Zero-length record
> > found.\n");
> > +                       break;
>
> For cases like this, it implies the FPDT table is far from usable and
> we should stop processing on such platforms.

Here's FPDT dump:

00000000: 4650 4454 4400 0000 018c 414c 4153 4b41  FPDTD.....ALASKA
00000010: 4120 4d20 4920 0000 0920 0701 414d 4920  A M I ... ..AMI
00000020: 1300 0100 0100 1001 0000 0000 30fe 207f  ............0. .
00000030: 0000 0000 0000 1001 0000 0000 54fe 207f  ............T. .
00000040: 0000 0000                                ....

S3PT at 0x7f20fe30:

7F20FE30: 53 33 50 54 24 00 00 00-00 00 00 00 00 00 18 01  *S3PT$...........*
7F20FE40: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00  *................*
7F20FE50: 00 00 00 00

FBPT at 0x7f20fe54:

7F20FE50: xx xx xx xx 46 42 50 54-3C 00 00 00 46 42 50 54  *....FBPT<...FBPT*
7F20FE60: 02 00 30 02 00 00 00 00-00 00 00 00 00 00 00 00  *..0.............*
7F20FE70: 2A A6 BC 6E 0B 00 00 00-1A 44 41 70 0B 00 00 00  **..n.....DAp....*
7F20FE80: 00 00 00 00 00 00 00 00-00 00 00 00 00 00 00 00  *................*

It looks like subtables are not usable. S3PT subtable has the first
record with zero len, and FBPT has its signature again instead of the
first record header.

So yeah, I agree that FPDT is not usabled in this case, and it
shouldn't be processed further.

> So, IMO, it is better to
> 1. return an error here rather than break and return 0.
> 2. add the error handling for fpdt_process_subtable() failures.
>
> what do you think?

Agree, I'll implement it in v2.

Regards,
Vasily