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

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

[Vasily Khoruzhick]

Buggy BIOSes may have zero-length records in FPDT, table, as a result
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@vger.kernel.org

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 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;
+		}
+
 		switch (record_header->type) {
 		case RECORD_S3_RESUME:
 			if (subtable_type != SUBTABLE_S3PT) {
-- 
2.42.0

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

[Zhang, Rui]

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@vger.kernel.org
> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>

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) {

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@intel.com> 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@vger.kernel.org
> >
> > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
>
> 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