When the firmware ROM supplies a FACS table with garbage, and the
firmware code does not clear the global_lock field before booting to a
loader/OS, the garbage bytes in that field (like 0xffffffff) can
indicate that the lock is taken when it is not, thereby preventing
obtaining said lock even though it is otherwise perfectly usable if
the field were not prepopulated with garbage.
Reset the lock to a known good state upon ACPI initialization.
References: https://bugzilla.kernel.org/show_bug.cgi?id=206553
Signed-off-by: Jan Engelhardt <[email protected]>
---
drivers/acpi/acpica/tbutils.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
index c5f0b8ec70cc..26bdbc585d7e 100644
--- a/drivers/acpi/acpica/tbutils.c
+++ b/drivers/acpi/acpica/tbutils.c
@@ -56,6 +56,9 @@ acpi_status acpi_tb_initialize_facs(void)
&facs));
acpi_gbl_FACS = facs;
}
+ /* Clear potential garbage from the initial FACS table. */
+ if (facs != NULL)
+ facs->global_lock &= ~0x3;
/* If there is no FACS, just continue. There was already an error msg */
--
2.26.0
On Mon, Mar 30, 2020 at 10:58 AM Jan Engelhardt <[email protected]> wrote:
>
> When the firmware ROM supplies a FACS table with garbage, and the
> firmware code does not clear the global_lock field before booting to a
> loader/OS, the garbage bytes in that field (like 0xffffffff) can
> indicate that the lock is taken when it is not, thereby preventing
> obtaining said lock even though it is otherwise perfectly usable if
> the field were not prepopulated with garbage.
>
> Reset the lock to a known good state upon ACPI initialization.
>
> References: https://bugzilla.kernel.org/show_bug.cgi?id=206553
> Signed-off-by: Jan Engelhardt <[email protected]>
Bob, Erik, please let me know if you want me to apply this directly or
you prefer to route it through the upstream.
> ---
>
> drivers/acpi/acpica/tbutils.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/acpi/acpica/tbutils.c b/drivers/acpi/acpica/tbutils.c
> index c5f0b8ec70cc..26bdbc585d7e 100644
> --- a/drivers/acpi/acpica/tbutils.c
> +++ b/drivers/acpi/acpica/tbutils.c
> @@ -56,6 +56,9 @@ acpi_status acpi_tb_initialize_facs(void)
> &facs));
> acpi_gbl_FACS = facs;
> }
> + /* Clear potential garbage from the initial FACS table. */
> + if (facs != NULL)
> + facs->global_lock &= ~0x3;
>
> /* If there is no FACS, just continue. There was already an error msg */
>
> --
> 2.26.0
>
> -----Original Message-----
> From: [email protected] <linux-acpi-
> [email protected]> On Behalf Of Rafael J. Wysocki
> Sent: Wednesday, April 1, 2020 2:12 AM
> To: Jan Engelhardt <[email protected]>; Kaneda, Erik
> <[email protected]>; Moore, Robert <[email protected]>
> Cc: Wysocki, Rafael J <[email protected]>; ACPI Devel Maling List
> <[email protected]>; Linux Kernel Mailing List <linux-
> [email protected]>
> Subject: Re: [PATCH] acpica: clear global_lock bits at FACS initialization
>
> On Mon, Mar 30, 2020 at 10:58 AM Jan Engelhardt <[email protected]> wrote:
> >
Hi Jan,
I've been reading the ACPI spec and there's nothing stated about what the
initial state of the lock should be... This patch is assuming that the lock should
be free when the FACS is being initialized and I don't think this is a safe
assumption to make.
What if this is a legitimate acquisition by an SMI handler very early in OS boot?
> > When the firmware ROM supplies a FACS table with garbage, and the
> > firmware code does not clear the global_lock field before booting to a
> > loader/OS, the garbage bytes in that field (like 0xffffffff) can
> > indicate that the lock is taken when it is not, thereby preventing
> > obtaining said lock even though it is otherwise perfectly usable if
> > the field were not prepopulated with garbage.
How do we know that the lock is taken when it is not?
Erik
> >
> > Reset the lock to a known good state upon ACPI initialization.
> >
> > References: https://bugzilla.kernel.org/show_bug.cgi?id=206553
> > Signed-off-by: Jan Engelhardt <[email protected]>
>
> Bob, Erik, please let me know if you want me to apply this directly or you
> prefer to route it through the upstream.
>
> > ---
> >
> > drivers/acpi/acpica/tbutils.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/acpi/acpica/tbutils.c
> > b/drivers/acpi/acpica/tbutils.c index c5f0b8ec70cc..26bdbc585d7e
> > 100644
> > --- a/drivers/acpi/acpica/tbutils.c
> > +++ b/drivers/acpi/acpica/tbutils.c
> > @@ -56,6 +56,9 @@ acpi_status acpi_tb_initialize_facs(void)
> > &facs));
> > acpi_gbl_FACS = facs;
> > }
> > + /* Clear potential garbage from the initial FACS table. */
> > + if (facs != NULL)
> > + facs->global_lock &= ~0x3;
> >
> > /* If there is no FACS, just continue. There was already an
> > error msg */
> >
> > --
> > 2.26.0
> >
On Wednesday 2020-04-01 23:55, Kaneda, Erik wrote:
>
>I've been reading the ACPI spec and there's nothing stated about what the
>initial state of the lock should be... This patch is assuming that the lock should
>be free when the FACS is being initialized and I don't think this is a safe
>assumption to make.
>
>What if this is a legitimate acquisition by an SMI handler very early in OS boot?
Before the OS has initialized ACPI (which, to me, is best recognized by what
action the power button will cause - either instant-off or ACPI event),
I would imagine that there are no SMI handlers that try to make use of ACPI
features like the FACS lock.
Furthermore, if the OS has taken the FACS lock and an SMI happens,
what would the SMI do if it cannot obtain the lock? It certainly can't
busywait for the OS, because that's interrupted..
>> > When the firmware ROM supplies a FACS table with garbage, and the
>> > firmware code does not clear the global_lock field before booting to a
>> > loader/OS, the garbage bytes in that field (like 0xffffffff) can
>> > indicate that the lock is taken when it is not, thereby preventing
>> > obtaining said lock even though it is otherwise perfectly usable if
>> > the field were not prepopulated with garbage.
>
>How do we know that the lock is taken when it is not?
We don't. ACPI does not make itself look good in this instance I am afraid.
Hi Jan,
url: https://github.com/0day-ci/linux/commits/Jan-Engelhardt/acpica-clear-global_lock-bits-at-FACS-initialization/20200330-183705
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
Reported-by: Dan Carpenter <[email protected]>
smatch warnings:
drivers/acpi/acpica/tbutils.c:60 acpi_tb_initialize_facs() error: uninitialized symbol 'facs'.
# https://github.com/0day-ci/linux/commit/cc0fd9e263391ff230ac700aa76dbcf7195c8c42
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout cc0fd9e263391ff230ac700aa76dbcf7195c8c42
vim +/facs +60 drivers/acpi/acpica/tbutils.c
009c4cbe99bea2 drivers/acpi/tables/tbutils.c Bob Moore 2008-11-12 35 acpi_status acpi_tb_initialize_facs(void)
009c4cbe99bea2 drivers/acpi/tables/tbutils.c Bob Moore 2008-11-12 36 {
7484619bff495c drivers/acpi/acpica/tbutils.c Lv Zheng 2015-08-25 37 struct acpi_table_facs *facs;
009c4cbe99bea2 drivers/acpi/tables/tbutils.c Bob Moore 2008-11-12 38
22e5b40ab21fca drivers/acpi/acpica/tbutils.c Bob Moore 2011-11-16 39 /* If Hardware Reduced flag is set, there is no FACS */
22e5b40ab21fca drivers/acpi/acpica/tbutils.c Bob Moore 2011-11-16 40
22e5b40ab21fca drivers/acpi/acpica/tbutils.c Bob Moore 2011-11-16 41 if (acpi_gbl_reduced_hardware) {
22e5b40ab21fca drivers/acpi/acpica/tbutils.c Bob Moore 2011-11-16 42 acpi_gbl_FACS = NULL;
22e5b40ab21fca drivers/acpi/acpica/tbutils.c Bob Moore 2011-11-16 43 return (AE_OK);
8ec3f459073e67 drivers/acpi/acpica/tbutils.c Lv Zheng 2015-08-25 44 } else if (acpi_gbl_FADT.Xfacs &&
8ec3f459073e67 drivers/acpi/acpica/tbutils.c Lv Zheng 2015-08-25 45 (!acpi_gbl_FADT.facs
8ec3f459073e67 drivers/acpi/acpica/tbutils.c Lv Zheng 2015-08-25 46 || !acpi_gbl_use32_bit_facs_addresses)) {
8ec3f459073e67 drivers/acpi/acpica/tbutils.c Lv Zheng 2015-08-25 47 (void)acpi_get_table_by_index(acpi_gbl_xfacs_index,
c04e1fb4396d27 drivers/acpi/acpica/tbutils.c Lv Zheng 2015-07-01 48 ACPI_CAST_INDIRECT_PTR(struct
c04e1fb4396d27 drivers/acpi/acpica/tbutils.c Lv Zheng 2015-07-01 49 acpi_table_header,
7484619bff495c drivers/acpi/acpica/tbutils.c Lv Zheng 2015-08-25 50 &facs));
7484619bff495c drivers/acpi/acpica/tbutils.c Lv Zheng 2015-08-25 51 acpi_gbl_FACS = facs;
8ec3f459073e67 drivers/acpi/acpica/tbutils.c Lv Zheng 2015-08-25 52 } else if (acpi_gbl_FADT.facs) {
^^^^^^^
8ec3f459073e67 drivers/acpi/acpica/tbutils.c Lv Zheng 2015-08-25 53 (void)acpi_get_table_by_index(acpi_gbl_facs_index,
009c4cbe99bea2 drivers/acpi/tables/tbutils.c Bob Moore 2008-11-12 54 ACPI_CAST_INDIRECT_PTR(struct
009c4cbe99bea2 drivers/acpi/tables/tbutils.c Bob Moore 2008-11-12 55 acpi_table_header,
7484619bff495c drivers/acpi/acpica/tbutils.c Lv Zheng 2015-08-25 56 &facs));
7484619bff495c drivers/acpi/acpica/tbutils.c Lv Zheng 2015-08-25 57 acpi_gbl_FACS = facs;
c04e1fb4396d27 drivers/acpi/acpica/tbutils.c Lv Zheng 2015-07-01 58 }
There is no else path, only else if paths.
cc0fd9e263391f drivers/acpi/acpica/tbutils.c Jan Engelhardt 2020-03-30 59 /* Clear potential garbage from the initial FACS table. */
cc0fd9e263391f drivers/acpi/acpica/tbutils.c Jan Engelhardt 2020-03-30 @60 if (facs != NULL)
cc0fd9e263391f drivers/acpi/acpica/tbutils.c Jan Engelhardt 2020-03-30 61 facs->global_lock &= ~0x3;
c04e1fb4396d27 drivers/acpi/acpica/tbutils.c Lv Zheng 2015-07-01 62
f06147f9fbf134 drivers/acpi/acpica/tbutils.c Lv Zheng 2015-07-01 63 /* If there is no FACS, just continue. There was already an error msg */
f06147f9fbf134 drivers/acpi/acpica/tbutils.c Lv Zheng 2015-07-01 64
c04e1fb4396d27 drivers/acpi/acpica/tbutils.c Lv Zheng 2015-07-01 65 return (AE_OK);
009c4cbe99bea2 drivers/acpi/tables/tbutils.c Bob Moore 2008-11-12 66 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]