Fix race in GenericSerialBus (I2C) and GPIO OpRegion parameter handling

The handling of the GenericSerialBus (I2C) and GPIO OpRegions in
AcpiEvAddressSpaceDispatch() passes a number of extra parameters to
the address-space handler through the address-space Context pointer
(instead of using more function parameters).

The Context is shared between threads, so if multiple threads try to call
the handler for the same address-space at the same time, then
a second thread could change the parameters of a first thread while the
handler is running for the first thread.

An example of this race hitting is the Lenovo Yoga Tablet2 1015L, where
there are both AttribBytes accesses and AttribByte accesses to the same
address-space. The AttribBytes access stores the number of bytes to
transfer in Context->AccessLength. Where as for the AttribByte access the
number of bytes to transfer is always 1 and FieldObj->Field.AccessLength
is unused (so 0). Both types of accesses racing from different threads
leads to the following problem:

1. Thread a. starts an AttribBytes access, stores a non 0 value
from FieldObj->Field.AccessLength in Context->AccessLength
2. Thread b. starts an AttribByte access, stores 0 in
Context->AccessLength
3. Thread a. calls i2c_acpi_space_handler() (under Linux). Which
sees that the access-type is ACPI_GSB_ACCESS_ATTRIB_MULTIBYTE and
calls acpi_gsb_i2c_read_bytes(..., Context->AccessLength)
4. At this point Context->AccessLength is 0 (set by thread b.)
rather then the FieldObj->Field.AccessLength value from thread a.
This 0 length reads leads to the following errors being logged:

 i2c i2c-0: adapter quirk: no zero length (addr 0x0078, size 0, read)
 i2c i2c-0: i2c read 0 bytes from client@0x78 starting at reg 0x0 failed, error: -95

Note this is just an example of the problems which this race can cause.
There are likely many more (sporadic) problems caused by this race.

This commit adds a new ContextMutex to acpi_object_addr_handler
and makes AcpiEvAddressSpaceDispatch() take that mutex when
using the shared Context to pass extra parameters to an address-space
handler, fixing this race.

Note the new mutex must be taken *after* exiting the interpreter,
therefor the existing AcpiExExitInterpreter() call is moved to above
the code which stores the extra parameters in the Context.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>

4 files changed, 59 insertions, 17 deletions

diff --git a/source/components/events/evhandler.c b/source/components/events/evhandler.c
index 9faccba37..39171014e 100644
--- a/source/components/events/evhandler.c
+++ b/source/components/events/evhandler.c
@@ -677,6 +677,13 @@ AcpiEvInstallSpaceHandler (
 
     /* Init handler obj */
 
+    Status = AcpiOsCreateMutex (&HandlerObj->AddressSpace.ContextMutex);
+    if (ACPI_FAILURE (Status))
+    {
+        AcpiUtRemoveReference (HandlerObj);
+        goto UnlockAndExit;
+    }
+
     HandlerObj->AddressSpace.SpaceId = (UINT8) SpaceId;
     HandlerObj->AddressSpace.HandlerFlags = Flags;
     HandlerObj->AddressSpace.RegionList = NULL;
diff --git a/source/components/events/evregion.c b/source/components/events/evregion.c
index 4f5bb7e0b..ad38439b2 100644
--- a/source/components/events/evregion.c
+++ b/source/components/events/evregion.c
@@ -270,6 +270,8 @@ AcpiEvAddressSpaceDispatch (
     ACPI_OPERAND_OBJECT     *RegionObj2;
     void                    *RegionContext = NULL;
     ACPI_CONNECTION_INFO    *Context;
+    ACPI_MUTEX              ContextMutex;
+    BOOLEAN                 ContextLocked;
     ACPI_PHYSICAL_ADDRESS   Address;
 
 
@@ -296,6 +298,8 @@ AcpiEvAddressSpaceDispatch (
     }
 
     Context = HandlerDesc->AddressSpace.Context;
+    ContextMutex = HandlerDesc->AddressSpace.ContextMutex;
+    ContextLocked = FALSE;
 
     /*
      * It may be the case that the region has never been initialized.
@@ -362,6 +366,23 @@ AcpiEvAddressSpaceDispatch (
     Handler = HandlerDesc->AddressSpace.Handler;
     Address = (RegionObj->Region.Address + RegionOffset);
 
+    ACPI_DEBUG_PRINT ((ACPI_DB_OPREGION,
+        "Handler %p (@%p) Address %8.8X%8.8X [%s]\n",
+        &RegionObj->Region.Handler->AddressSpace, Handler,
+        ACPI_FORMAT_UINT64 (Address),
+        AcpiUtGetRegionName (RegionObj->Region.SpaceId)));
+
+    if (!(HandlerDesc->AddressSpace.HandlerFlags &
+        ACPI_ADDR_HANDLER_DEFAULT_INSTALLED))
+    {
+        /*
+         * For handlers other than the default (supplied) handlers, we must
+         * exit the interpreter because the handler *might* block -- we don't
+         * know what it will do, so we can't hold the lock on the interpreter.
+         */
+        AcpiExExitInterpreter();
+    }
+
     /*
      * Special handling for GenericSerialBus and GeneralPurposeIo:
      * There are three extra parameters that must be passed to the
@@ -370,6 +391,11 @@ AcpiEvAddressSpaceDispatch (
      *   2) Length of the above buffer
      *   3) Actual access length from the AccessAs() op
      *
+     * Since we pass these extra parameters via the context, which is
+     * shared between threads, we must lock the context to avoid these
+     * parameters being changed from another thread before the handler
+     * has completed running.
+     *
      * In addition, for GeneralPurposeIo, the Address and BitWidth fields
      * are defined as follows:
      *   1) Address is the pin number index of the field (bit offset from
@@ -380,6 +406,15 @@ AcpiEvAddressSpaceDispatch (
         Context &&
         FieldObj)
     {
+
+        Status = AcpiOsAcquireMutex (ContextMutex, ACPI_WAIT_FOREVER);
+        if (ACPI_FAILURE (Status))
+        {
+            goto ReEnterInterpreter;
+        }
+
+        ContextLocked = TRUE;
+
         /* Get the Connection (ResourceTemplate) buffer */
 
         Context->Connection = FieldObj->Field.ResourceBuffer;
@@ -390,6 +425,15 @@ AcpiEvAddressSpaceDispatch (
         Context &&
         FieldObj)
     {
+
+        Status = AcpiOsAcquireMutex (ContextMutex, ACPI_WAIT_FOREVER);
+        if (ACPI_FAILURE (Status))
+        {
+            goto ReEnterInterpreter;
+        }
+
+        ContextLocked = TRUE;
+
         /* Get the Connection (ResourceTemplate) buffer */
 
         Context->Connection = FieldObj->Field.ResourceBuffer;
@@ -399,28 +443,16 @@ AcpiEvAddressSpaceDispatch (
         BitWidth = FieldObj->Field.BitLength;
     }
 
-    ACPI_DEBUG_PRINT ((ACPI_DB_OPREGION,
-        "Handler %p (@%p) Address %8.8X%8.8X [%s]\n",
-        &RegionObj->Region.Handler->AddressSpace, Handler,
-        ACPI_FORMAT_UINT64 (Address),
-        AcpiUtGetRegionName (RegionObj->Region.SpaceId)));
-
-    if (!(HandlerDesc->AddressSpace.HandlerFlags &
-        ACPI_ADDR_HANDLER_DEFAULT_INSTALLED))
-    {
-        /*
-         * For handlers other than the default (supplied) handlers, we must
-         * exit the interpreter because the handler *might* block -- we don't
-         * know what it will do, so we can't hold the lock on the interpreter.
-         */
-        AcpiExExitInterpreter();
-    }
-
     /* Call the handler */
 
     Status = Handler (Function, Address, BitWidth, Value, Context,
         RegionObj2->Extra.RegionContext);
 
+    if (ContextLocked)
+    {
+        AcpiOsReleaseMutex (ContextMutex);
+    }
+
     if (ACPI_FAILURE (Status))
     {
         ACPI_EXCEPTION ((AE_INFO, Status, "Returned by Handler for [%s]",
@@ -438,6 +470,7 @@ AcpiEvAddressSpaceDispatch (
         }
     }
 
+ReEnterInterpreter:
     if (!(HandlerDesc->AddressSpace.HandlerFlags &
         ACPI_ADDR_HANDLER_DEFAULT_INSTALLED))
     {
diff --git a/source/components/events/evxfregn.c b/source/components/events/evxfregn.c
index 9113d8a26..3febc81c2 100644
--- a/source/components/events/evxfregn.c
+++ b/source/components/events/evxfregn.c
@@ -362,6 +362,7 @@ AcpiRemoveAddressSpaceHandler (
 
             /* Now we can delete the handler object */
 
+            AcpiOsReleaseMutex (HandlerObj->AddressSpace.ContextMutex);
             AcpiUtRemoveReference (HandlerObj);
             goto UnlockAndExit;
         }
diff --git a/source/include/acobject.h b/source/include/acobject.h
index f047efb8f..e7d9f1c9b 100644
--- a/source/include/acobject.h
+++ b/source/include/acobject.h
@@ -521,6 +521,7 @@ typedef struct acpi_object_addr_handler
     ACPI_ADR_SPACE_HANDLER          Handler;
     ACPI_NAMESPACE_NODE             *Node;              /* Parent device */
     void                            *Context;
+    ACPI_MUTEX                      ContextMutex;
     ACPI_ADR_SPACE_SETUP            Setup;
     union acpi_operand_object       *RegionList;        /* Regions using this handler */
     union acpi_operand_object       *Next;