| 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
| ||
|
Message-ID: <20130725120414.GA22964@konrad-lan.dumpdata.com>
Date: Thu, 25 Jul 2013 08:04:14 -0400
From: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
To: "Zheng, Lv" <lv.zheng@...el.com>
Cc: Ben Guthro <benjamin.guthro@...rix.com>,
"Moore, Robert" <robert.moore@...el.com>,
Jan Beulich <jbeulich@...e.com>,
"Rafael J . Wysocki" <rjw@...k.pl>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
"xen-devel@...ts.xen.org" <xen-devel@...ts.xen.org>
Subject: Re: [PATCH v3 1/3] acpi: Call acpi_os_prepare_sleep hook in reduced
hardware sleep path
CC-ing some of the tboot maintainers.
> As what I've said, it's up to the others to determine if the patch is OK.
> I just need to make my concerns visible in the community. :-)
If I understand your concerns you don't want the hook to depend on any
of the bit manipulations the existing code does for the pm1 values. The
hook should do it itself case it needs to tweak them or what not.
And it also frees you from altering the ACPICA code without having to
worry about being dependent on what the input values the hook requires?
Is this what you had in mind? (not compile tested nor tested).
I am not even sure if outside the drivers/acpi you can call
acpi_hw_get_bit_register_info ..
And since the Xen bits would do the same exact bit manipulation it
probably could use a library to do pm1* stuff so both tboot and Xen
can use it.
diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c
index f84fe00..59570b1 100644
--- a/arch/x86/kernel/tboot.c
+++ b/arch/x86/kernel/tboot.c
@@ -273,20 +273,75 @@ static void tboot_copy_fadt(const struct acpi_table_fadt *fadt)
offsetof(struct acpi_table_facs, firmware_waking_vector);
}
-static int tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control)
+static int tboot_get_pm_control(bool legacy)
+{
+ u32 pm1a_control;
+ u32 pm1b_control;
+ u32 in_value;
+ acpi_status status;
+ struct acpi_bit_register_info *sleep_type_reg_info;
+ struct acpi_bit_register_info *sleep_enable_reg_info;
+
+ if (!legacy)
+ return -ENOSPC;
+
+ status = acpi_hw_register_read(ACPI_REGISTER_PM1_CONTROL,
+ &pm1a_control);
+ if (ACPI_FAILURE(status)) {
+ return -EXXX /* something */;
+ }
+ sleep_type_reg_info = acpi_hw_get_bit_register_info(ACPI_BITREG_SLEEP_TYPE);
+ sleep_enable_reg_info = acpi_hw_get_bit_register_info(ACPI_BITREG_SLEEP_ENABLE);
+ /* Clear the SLP_EN and SLP_TYP fields */
+
+ pm1a_control &= ~(sleep_type_reg_info->access_bit_mask |
+ sleep_enable_reg_info->access_bit_mask);
+ pm1b_control = pm1a_control;
+
+ /* Insert the SLP_TYP bits */
+
+ pm1a_control |=
+ (acpi_gbl_sleep_type_a << sleep_type_reg_info->bit_position);
+ pm1b_control |=
+ (acpi_gbl_sleep_type_b << sleep_type_reg_info->bit_position);
+
+ /*
+ * We split the writes of SLP_TYP and SLP_EN to workaround
+ * poorly implemented hardware.
+ */
+
+ /* Write #1: write the SLP_TYP data to the PM1 Control registers */
+
+ status = acpi_hw_write_pm1_control(pm1a_control, pm1b_control);
+ if (ACPI_FAILURE(status)) {
+ return -EXXX /* something */;
+ }
+
+ /* Insert the sleep enable (SLP_EN) bit */
+
+ pm1a_control |= sleep_enable_reg_info->access_bit_mask;
+ pm1b_control |= sleep_enable_reg_info->access_bit_mask;
+ tboot->acpi_sinfo.pm1a_cnt_val = pm1a_control;
+ tboot->acpi_sinfo.pm1b_cnt_val = pm1b_control;
+ return 0;
+}
+static int tboot_sleep(u8 sleep_state);
{
static u32 acpi_shutdown_map[ACPI_S_STATE_COUNT] = {
/* S0,1,2: */ -1, -1, -1,
/* S3: */ TB_SHUTDOWN_S3,
/* S4: */ TB_SHUTDOWN_S4,
/* S5: */ TB_SHUTDOWN_S5 };
+ int rc;
if (!tboot_enabled())
return 0;
tboot_copy_fadt(&acpi_gbl_FADT);
- tboot->acpi_sinfo.pm1a_cnt_val = pm1a_control;
- tboot->acpi_sinfo.pm1b_cnt_val = pm1b_control;
+
+ rc = tboot_get_pm_control();
+ if (rc < 0)
+ return -1;
/* we always use the 32b wakeup vector */
tboot->acpi_sinfo.vector_width = 32;
diff --git a/drivers/acpi/acpica/hwesleep.c b/drivers/acpi/acpica/hwesleep.c
index 5e5f762..a8e98f9 100644
--- a/drivers/acpi/acpica/hwesleep.c
+++ b/drivers/acpi/acpica/hwesleep.c
@@ -113,6 +113,15 @@ acpi_status acpi_hw_extended_sleep(u8 sleep_state)
!acpi_gbl_FADT.sleep_status.address) {
return_ACPI_STATUS(AE_NOT_EXIST);
}
+ /*
+ * If using tboot or other platforms that need tweaks then
+ * do them here, and also bail out if neccessary.
+ */
+ status = acpi_os_prepare_sleep(sleep_state);
+ if (ACPI_SKIP(status))
+ return_ACPI_STATUS(AE_OK);
+ if (ACPI_FAILURE(status))
+ return_ACPI_STATUS(status);
/* Clear wake status (WAK_STS) */
diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
index e3828cc..909b23b 100644
--- a/drivers/acpi/acpica/hwsleep.c
+++ b/drivers/acpi/acpica/hwsleep.c
@@ -108,6 +108,16 @@ acpi_status acpi_hw_legacy_sleep(u8 sleep_state)
return_ACPI_STATUS(status);
}
+ /*
+ * If using tboot or other platforms that need tweaks then
+ * do them here, and also bail out if neccessary.
+ */
+ status = acpi_os_prepare_sleep(sleep_state);
+ if (ACPI_SKIP(status))
+ return_ACPI_STATUS(AE_OK);
+ if (ACPI_FAILURE(status))
+ return_ACPI_STATUS(status);
+
/* Get current value of PM1A control */
status = acpi_hw_register_read(ACPI_REGISTER_PM1_CONTROL,
@@ -152,12 +162,6 @@ acpi_status acpi_hw_legacy_sleep(u8 sleep_state)
ACPI_FLUSH_CPU_CACHE();
- status = acpi_os_prepare_sleep(sleep_state, pm1a_control,
- pm1b_control);
- if (ACPI_SKIP(status))
- return_ACPI_STATUS(AE_OK);
- if (ACPI_FAILURE(status))
- return_ACPI_STATUS(status);
/* Write #2: Write both SLP_TYP + SLP_EN */
status = acpi_hw_write_pm1_control(pm1a_control, pm1b_control);
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index e721863..ffcc364 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -77,8 +77,7 @@ EXPORT_SYMBOL(acpi_in_debugger);
extern char line_buf[80];
#endif /*ENABLE_DEBUGGER */
-static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 pm1a_ctrl,
- u32 pm1b_ctrl);
+static int (*__acpi_os_prepare_sleep)(u8 sleep_state);
static acpi_osd_handler acpi_irq_handler;
static void *acpi_irq_context;
@@ -1757,13 +1756,11 @@ acpi_status acpi_os_terminate(void)
return AE_OK;
}
-acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 pm1a_control,
- u32 pm1b_control)
+acpi_status acpi_os_prepare_sleep(u8 sleep_state)
{
int rc = 0;
if (__acpi_os_prepare_sleep)
- rc = __acpi_os_prepare_sleep(sleep_state,
- pm1a_control, pm1b_control);
+ rc = __acpi_os_prepare_sleep(sleep_state);
if (rc < 0)
return AE_ERROR;
else if (rc > 0)
@@ -1772,8 +1769,7 @@ acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 pm1a_control,
return AE_OK;
}
-void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
- u32 pm1a_ctrl, u32 pm1b_ctrl))
+void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state))
{
__acpi_os_prepare_sleep = func;
}
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 17b5b59..8de1043 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -480,8 +480,7 @@ static inline bool acpi_driver_match_device(struct device *dev,
void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
u32 pm1a_ctrl, u32 pm1b_ctrl));
-acpi_status acpi_os_prepare_sleep(u8 sleep_state,
- u32 pm1a_control, u32 pm1b_control);
+acpi_status acpi_os_prepare_sleep(u8 sleep_state);
#ifdef CONFIG_X86
void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
#else
.. massive snip..
> > > On 07/24/2013 10:38 AM, Moore, Robert wrote:
> > > > I haven't found a high-level description of "acpi_os_prepare_sleep",
> > perhaps I missed it.
> > > >
> > > > Can someone point me to the overall description of this change and why it is
> > being considered?
> > >
> > > Hi Bob,
> > >
> > > For this series, the v6 of this series does a decent job of what it is
> > > trying to accomplish:
> > > https://lkml.org/lkml/2013/7/1/205
> > >
> > > However, I recognize that this does not really describe *why*
> > > acpi_os_prepare_sleep is necessary to begin with. For that, we need to
> > > go back a little more.
> > >
> > > The summary for the series that introduced it is a good description,
> > > of the reasons it is necessary:
> > > http://lkml.indiana.edu/hypermail/linux/kernel/1112.2/00450.html
> > >
> > > In summary though - in the case of Xen (and I believe this is also
> > > true in tboot) a value inappropriate for a VM (which dom0 is a special
> > > case
> > > of) was being written to cr3, and the physical machine would never
> > > come out of S3.
> > >
> > > This mechanism gives an os specific hook to do something else down at
> > > the lower levels, while still being able to take advantage of the
> > > large amount of OS independent code in ACPICA.
> >
> > It might be also prudent to look at original 'hook' that was added by Intel in the
> > Linux code to support TXT:
> >
> >
> > commit 86886e55b273f565935491816c7c96b82469d4f8
> > Author: Joseph Cihula <joseph.cihula@...el.com>
> > Date: Tue Jun 30 19:31:07 2009 -0700
> >
> > x86, intel_txt: Intel TXT Sx shutdown support
> >
> > Support for graceful handling of sleep states (S3/S4/S5) after an Intel(R)
> > TXT launch.
> >
> > Without this patch, attempting to place the system in one of the ACPI
> > sleep
> > states (S3/S4/S5) will cause the TXT hardware to treat this as an attack
> > and
> > will cause a system reset, with memory locked. Not only may the
> > subsequent
> > memory scrub take some time, but the platform will be unable to enter
> > the
> > requested power state.
> >
> > This patch calls back into the tboot so that it may properly and securely
> > clean
> > up system state and clear the secrets-in-memory flag, after which it will
> > place
> > the system into the requested sleep state using ACPI information passed
> > by the kernel.
> >
> > arch/x86/kernel/smpboot.c | 2 ++
> > drivers/acpi/acpica/hwsleep.c | 3 +++
> > kernel/cpu.c | 7 ++++++-
> > 3 files changed, 11 insertions(+), 1 deletion(-)
> >
> > Signed-off-by: Joseph Cihula <joseph.cihula@...el.com>
> > Signed-off-by: Shane Wang <shane.wang@...el.com>
> > Signed-off-by: H. Peter Anvin <hpa@...or.com>
> >
> > I suspect that if tboot was used with a different OS (Solaris?) it would hit the
> > same case and a similar hook would be needed.
> >
> > Said 'hook' (which was a call to tboot_sleep) was converted to be a more
> > neutral 'acpi_os_prepare_sleep' which tboot can use (and incidently Xen too).
> >
> > I think what Bob is saying that if said hook is neccessary (and I believe it is - and
> > Intel TXT maintainer thinks so too since he added it in the first place), then the
> > right way of adding it is via the ACPICA tree.
> >
> > Should the discussion for this be moved there ? (https://acpica.org/community)
> > and an generic 'os_prepare_sleep' patch added in
> > git://github.com/acpica/acpica.git?
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body
> > of a message to majordomo@...r.kernel.org More majordomo info at
> > http://vger.kernel.org/majordomo-info.html
> --
> 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/
>
--
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