Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
MIME-Version: 1.0
References: <20230925214046.1051350-1-anarsoul@gmail.com> <b0786bf5571050397b5075bac7c1e9023bfa5aec.camel@intel.com>
In-Reply-To: <b0786bf5571050397b5075bac7c1e9023bfa5aec.camel@intel.com>
From:   Vasily Khoruzhick <anarsoul@gmail.com>
Date:   Tue, 26 Sep 2023 18:47:24 -0700
Message-ID: <CA+E=qVc424VJsqGRR+SZbDmDdtdVmx+Ag6vt_brhZsNv1JTCRw@mail.gmail.com>
Subject: Re: [PATCH] ACPI: FPDT: break out of the loop if record length is zero
To:     "Zhang, Rui" <rui.zhang@intel.com>
Cc:     "rafael@kernel.org" <rafael@kernel.org>,
        "lenb@kernel.org" <lenb@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>,
        "stable@vger.kernel.org" <stable@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Sep 25, 2023 at 10:03=E2=80=AFPM Zhang, Rui <rui.zhang@intel.com> w=
rote:
>
> 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-re=
cord-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 =3D (void *)subtable_header + offset;
> >                 offset +=3D 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<...FBP=
T*
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