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: [email protected]
>
> Signed-off-by: Vasily Khoruzhick <[email protected]>
Is there a bugzilla or something where such a buggy BIOS is reported?
> ---
> 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.
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?
thanks,
rui
> + }
> +
> switch (record_header->type) {
> case RECORD_S3_RESUME:
> if (subtable_type != SUBTABLE_S3PT) {
On Mon, Sep 25, 2023 at 10:03 PM Zhang, Rui <[email protected]> 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: [email protected]
> >
> > Signed-off-by: Vasily Khoruzhick <[email protected]>
>
> 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