linux-kernel - Re: [RFC][RFT][PATCH] ACPI: Protection from suspending in the middle of EC transaction

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <201001312331.50321.rjw@sisk.pl>
Date:	Sun, 31 Jan 2010 23:31:50 +0100
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Maxim Levitsky <maximlevitsky@...il.com>
Cc:	ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
	Alexey Starikovskiy <astarikovskiy@...e.de>,
	Len Brown <lenb@...nel.org>,
	pm list <linux-pm@...ts.linux-foundation.org>,
	Thomas Renninger <trenn@...e.de>,
	Matthew Garrett <mjg59@...f.ucam.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC][RFT][PATCH] ACPI: Protection from suspending in the middle of EC transaction

On Sunday 31 January 2010, Rafael J. Wysocki wrote:
> On Sunday 31 January 2010, Maxim Levitsky wrote:
> > On Sun, 2010-01-31 at 00:29 +0100, Rafael J. Wysocki wrote: 
> ...
> > >  
> > >  static int acpi_ec_resume(struct acpi_device *device)
> > >  {
> > >  	struct acpi_ec *ec = acpi_driver_data(device);
> > > +
> > > +	mutex_lock(&ec->lock);
> > >  	/* Enable use of GPE back */
> > >  	acpi_enable_gpe(NULL, ec->gpe);
> > > +	/* Allow transactions to happen again */
> > > +	set_bit(EC_FLAGS_SUSPENDED, &ec->flags);
> >           ^^^^^^^^^^^^
> > Thats why it doesn't work here....
> > Will retest now.
> 
> Ouch, sorry.

Fixed version is appended, for the record.

Rafael

---
From: Rafael J. Wysocki <rjw@...k.pl>
Subject: ACPI / EC: Add protection from suspending in the middle of EC transaction

There is a race between the suspend process and the EC driver that
may result in suspending in the middle of an EC transaction in
progress, which may lead to unpredictable behavior of the platform.

To remove that race condition, make acpi_ec_suspend() and
acpi_ec_resume() acquire ec->lock and introduce a new EC flag,
EC_FLAGS_SUSPENDED, set in acpi_ec_suspend() and cleared in
acpi_ec_resume(), that will cause acpi_ec_transaction() to return
error code (-EBUSY) if set.

Signed-off-by: Rafael J. Wysocki <rjw@...k.pl>
---
 drivers/acpi/ec.c |   17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/acpi/ec.c
===================================================================
--- linux-2.6.orig/drivers/acpi/ec.c
+++ linux-2.6/drivers/acpi/ec.c
@@ -76,8 +76,9 @@ enum ec_command {
 enum {
 	EC_FLAGS_QUERY_PENDING,		/* Query is pending */
 	EC_FLAGS_GPE_STORM,		/* GPE storm detected */
-	EC_FLAGS_HANDLERS_INSTALLED	/* Handlers for GPE and
+	EC_FLAGS_HANDLERS_INSTALLED,	/* Handlers for GPE and
 					 * OpReg are installed */
+	EC_FLAGS_SUSPENDED,		/* Driver is suspended */
 };
 
 /* If we find an EC via the ECDT, we need to keep a ptr to its context */
@@ -291,6 +292,10 @@ static int acpi_ec_transaction(struct ac
 	if (t->rdata)
 		memset(t->rdata, 0, t->rlen);
 	mutex_lock(&ec->lock);
+	if (test_bit(EC_FLAGS_SUSPENDED, &ec->flags)) {
+		status = -EBUSY;
+		goto unlock;
+	}
 	if (ec->global_lock) {
 		status = acpi_acquire_global_lock(ACPI_EC_UDELAY_GLK, &glk);
 		if (ACPI_FAILURE(status)) {
@@ -1059,16 +1064,26 @@ error:
 static int acpi_ec_suspend(struct acpi_device *device, pm_message_t state)
 {
 	struct acpi_ec *ec = acpi_driver_data(device);
+
+	mutex_lock(&ec->lock);
+	/* Prevent transactions from happening while suspended */
+	set_bit(EC_FLAGS_SUSPENDED, &ec->flags);
 	/* Stop using GPE */
 	acpi_disable_gpe(NULL, ec->gpe);
+	mutex_unlock(&ec->lock);
 	return 0;
 }
 
 static int acpi_ec_resume(struct acpi_device *device)
 {
 	struct acpi_ec *ec = acpi_driver_data(device);
+
+	mutex_lock(&ec->lock);
 	/* Enable use of GPE back */
 	acpi_enable_gpe(NULL, ec->gpe);
+	/* Allow transactions to happen again */
+	clear_bit(EC_FLAGS_SUSPENDED, &ec->flags);
+	mutex_unlock(&ec->lock);
 	return 0;
 }
 
--
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/