| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 25 Jun 2015 01:57:03 +0200
From: "Rafael J. Wysocki" <rjw@...ysocki.net>
To: Lv Zheng <lv.zheng@...el.com>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
Len Brown <len.brown@...el.com>, Lv Zheng <zetalog@...il.com>,
linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
Bob Moore <robert.moore@...el.com>
Subject: Re: [PATCH v2 05/28] ACPICA: Hardware: Enable firmware waking vector for both 32-bit and 64-bit FACS.
On Wednesday, June 24, 2015 11:02:54 AM Lv Zheng wrote:
> ACPICA commit 368eb60778b27b6ae94d3658ddc902ca1342a963
> ACPICA commit 70f62a80d65515e1285fdeeb50d94ee6f07df4bd
>
> The following commit is reported to have broken s2ram on some platforms:
> Commit: 0249ed2444d65d65fc3f3f64f398f1ad0b7e54cd
> ACPICA: Add option to favor 32-bit FADT addresses.
> The platform reports 2 FACS tables (which is not allowed by ACPI
> specification) and the new 32-bit address favor rule forces OSPMs to use
> the FACS table reported via FADT's X_FIRMWARE_CTRL field.
>
> The root cause of the reported bug might be one of the followings:
> 1. BIOS may favor the 64-bit firmware waking vector address when the
> version of the FACS is greater than 0 and Linux currently only supports
> resuming from the real mode, so the 64-bit firmware waking vector has
> never been set and might be invalid to BIOS while the commit enables
> higher version FACS.
> 2. BIOS may favor the FACS reported via the "FIRMWARE_CTRL" field in the
> FADT while the commit doesn't set the firmware waking vector address of
> the FACS reported by "FIRMWARE_CTRL", it only sets the firware waking
> vector address of the FACS reported by "X_FIRMWARE_CTRL".
>
> This patch excludes the cases that can trigger the bugs caused by the root
> cause 2.
>
> There is no handshaking mechanism can be used by OSPM to tell BIOS which
> FACS is currently used. Thus the FACS reported by "FIRMWARE_CTRL" may still
> be used by BIOS and the 0 value of the 32-bit firmware waking vector might
> trigger such failure.
>
> This patch enables the firmware waking vectors for both 32bit/64bit FACS
> tables in order to ensure we can exclude the cases that trigger the bugs
> caused by the root cause 2. The exclusion is split into 2 commits so that
> if it turns out not to be necessary, this single commit can be reverted
> without affecting the useful one. Lv Zheng, Bob Moore.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=74021
> Link: https://github.com/acpica/acpica/commit/368eb607
> Link: https://github.com/acpica/acpica/commit/70f62a80
> Reported-and-tested-by: Oswald Buddenhagen <ossi@....org>
> Signed-off-by: Lv Zheng <lv.zheng@...el.com>
> Signed-off-by: Bob Moore <robert.moore@...el.com>
> ---
> drivers/acpi/acpica/acglobal.h | 2 ++
> drivers/acpi/acpica/hwxfsleep.c | 74 ++++++++++++++++++++++++++++++++-------
> drivers/acpi/acpica/tbutils.c | 14 ++++----
> 3 files changed, 71 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/acpi/acpica/acglobal.h b/drivers/acpi/acpica/acglobal.h
> index a0c4787..53f96a3 100644
> --- a/drivers/acpi/acpica/acglobal.h
> +++ b/drivers/acpi/acpica/acglobal.h
> @@ -61,6 +61,8 @@ ACPI_GLOBAL(struct acpi_table_header, acpi_gbl_original_dsdt_header);
>
> #if (!ACPI_REDUCED_HARDWARE)
> ACPI_GLOBAL(struct acpi_table_facs *, acpi_gbl_FACS);
> +ACPI_GLOBAL(struct acpi_table_facs *, acpi_gbl_facs32);
> +ACPI_GLOBAL(struct acpi_table_facs *, acpi_gbl_facs64);
>
> #endif /* !ACPI_REDUCED_HARDWARE */
>
> diff --git a/drivers/acpi/acpica/hwxfsleep.c b/drivers/acpi/acpica/hwxfsleep.c
> index c67cd32..e273b2e 100644
> --- a/drivers/acpi/acpica/hwxfsleep.c
> +++ b/drivers/acpi/acpica/hwxfsleep.c
> @@ -50,6 +50,13 @@
> ACPI_MODULE_NAME("hwxfsleep")
>
> /* Local prototypes */
> +#if (!ACPI_REDUCED_HARDWARE)
> +static acpi_status
> +acpi_hw_set_firmware_waking_vector(struct acpi_table_facs *facs,
> + acpi_physical_address physical_address,
> + acpi_physical_address physical_address64);
> +#endif
> +
> static acpi_status acpi_hw_sleep_dispatch(u8 sleep_state, u32 function_id);
>
> /*
> @@ -79,9 +86,10 @@ static struct acpi_sleep_functions acpi_sleep_dispatch[] = {
> #if (!ACPI_REDUCED_HARDWARE)
> /*******************************************************************************
> *
> - * FUNCTION: acpi_set_firmware_waking_vector
> + * FUNCTION: acpi_hw_set_firmware_waking_vector
> *
> - * PARAMETERS: physical_address - 32-bit physical address of ACPI real mode
> + * PARAMETERS: facs - Pointer to FACS table
> + * physical_address - 32-bit physical address of ACPI real mode
> * entry point
> * physical_address64 - 64-bit physical address of ACPI protected
> * entry point
> @@ -92,11 +100,12 @@ static struct acpi_sleep_functions acpi_sleep_dispatch[] = {
> *
> ******************************************************************************/
>
> -acpi_status
> -acpi_set_firmware_waking_vector(acpi_physical_address physical_address,
> - acpi_physical_address physical_address64)
> +static acpi_status
> +acpi_hw_set_firmware_waking_vector(struct acpi_table_facs *facs,
> + acpi_physical_address physical_address,
> + acpi_physical_address physical_address64)
> {
> - ACPI_FUNCTION_TRACE(acpi_set_firmware_waking_vector);
> + ACPI_FUNCTION_TRACE(acpi_hw_set_firmware_waking_vector);
>
>
> /*
> @@ -109,25 +118,66 @@ acpi_set_firmware_waking_vector(acpi_physical_address physical_address,
>
> /* Set the 32-bit vector */
>
> - acpi_gbl_FACS->firmware_waking_vector = (u32)physical_address;
> + facs->firmware_waking_vector = (u32)physical_address;
>
> - if (acpi_gbl_FACS->length > 32) {
> - if (acpi_gbl_FACS->version >= 1) {
> + if (facs->length > 32) {
> + if (facs->version >= 1) {
>
> /* Set the 64-bit vector */
>
> - acpi_gbl_FACS->xfirmware_waking_vector =
> - physical_address64;
> + facs->xfirmware_waking_vector = physical_address64;
> } else {
> /* Clear the 64-bit vector if it exists */
>
> - acpi_gbl_FACS->xfirmware_waking_vector = 0;
> + facs->xfirmware_waking_vector = 0;
> }
> }
>
> return_ACPI_STATUS(AE_OK);
> }
>
> +/*******************************************************************************
> + *
> + * FUNCTION: acpi_set_firmware_waking_vector
> + *
> + * PARAMETERS: physical_address - 32-bit physical address of ACPI real mode
> + * entry point
> + * physical_address64 - 64-bit physical address of ACPI protected
> + * entry point
> + *
> + * RETURN: Status
> + *
> + * DESCRIPTION: Sets the firmware_waking_vector fields of the FACS
> + *
> + ******************************************************************************/
> +
> +acpi_status
> +acpi_set_firmware_waking_vector(acpi_physical_address physical_address,
> + acpi_physical_address physical_address64)
The question here is: Why does the host OS need to care about the second
argument of this function that will always be 0? Why didn't you keep the
old header of acpi_set_firmware_waking_vector() as a one-argument function
taking a u32 and why didn't you add something like
acpi_status acpi_set_firmware_waking_vector_full(u32 real_mode_address,
acpi_physical_address high_address)
and why didn't you redefine acpi_set_firmware_waking_vector() as
acpi_status acpi_set_firmware_waking_vector(u32 real_mode_address)
{
return acpi_set_firmware_waking_vector_full(real_mode_address, 0);
}
?
If you did that, there wouldn't be any need to touch the code in
drivers/acpi/sleep.c and the arch headers, so can you please explain to me
why *exactly* you didn't do that?
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists