Hans,
Could you make a pull request for this (and any related patches) on our github?
Thanks,
Bob
-----Original Message-----
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Sent: Saturday, December 26, 2020 6:28 AM
To: Rafael J . Wysocki <rjw@xxxxxxxxxxxxx>; Len Brown <lenb@xxxxxxxxxx>; Moore, Robert <robert.moore@xxxxxxxxx>; Kaneda, Erik <erik.kaneda@xxxxxxxxx>
Cc: Hans de Goede <hdegoede@xxxxxxxxxx>; linux-acpi@xxxxxxxxxxxxxxx; devel@xxxxxxxxxx
Subject: [PATCH 1/2] ACPICA: Fix race in GenericSerialBus (I2C) and GPIO OpRegion parameter handling
The handling of the GenericSerialBus (I2C) and GPIO OpRegions in
acpi_ev_address_space_dispatch() 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->access_length. Where as for the AttribByte access the number of bytes to transfer is always 1 and field_obj->field.access_length 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 field_obj->field.access_length in context->access_length 2. Thread b. starts an AttribByte access, stores 0 in
context->access_length
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->access_length) 4. At this point context->access_length is 0 (set by thread b.) rather then the field_obj->field.access_length 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 context_mutex to acpi_object_addr_handler and makes acpi_ev_address_space_dispatch() 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 acpi_ex_exit_interpreter() call is moved to above the code which stores the extra parameters in the context.
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/acpi/acpica/acobject.h | 1 +
drivers/acpi/acpica/evhandler.c | 6 ++++ drivers/acpi/acpica/evregion.c | 61 ++++++++++++++++++++++++--------- drivers/acpi/acpica/evxfregn.c | 1 +
4 files changed, 52 insertions(+), 17 deletions(-)
diff --git a/drivers/acpi/acpica/acobject.h b/drivers/acpi/acpica/acobject.h index 9f0219a8cb98..dd7efafcb103 100644
--- a/drivers/acpi/acpica/acobject.h
+++ b/drivers/acpi/acpica/acobject.h
@@ -284,6 +284,7 @@ struct acpi_object_addr_handler {
acpi_adr_space_handler handler;
struct acpi_namespace_node *node; /* Parent device */
void *context;
+ acpi_mutex context_mutex;
acpi_adr_space_setup setup;
union acpi_operand_object *region_list; /* Regions using this handler */
union acpi_operand_object *next;
diff --git a/drivers/acpi/acpica/evhandler.c b/drivers/acpi/acpica/evhandler.c index 5884eba047f7..347199f29afe 100644
--- a/drivers/acpi/acpica/evhandler.c
+++ b/drivers/acpi/acpica/evhandler.c
@@ -489,6 +489,12 @@ acpi_ev_install_space_handler(struct acpi_namespace_node *node,
/* Init handler obj */
+ status = acpi_os_create_mutex(&handler_obj->address_space.context_mutex);
+ if (ACPI_FAILURE(status)) {
+ acpi_ut_remove_reference(handler_obj);
+ goto unlock_and_exit;
+ }
+
handler_obj->address_space.space_id = (u8)space_id;
handler_obj->address_space.handler_flags = flags;
handler_obj->address_space.region_list = NULL; diff --git a/drivers/acpi/acpica/evregion.c b/drivers/acpi/acpica/evregion.c index 21ff341e34a4..8e84eb0641e0 100644
--- a/drivers/acpi/acpica/evregion.c
+++ b/drivers/acpi/acpica/evregion.c
@@ -112,6 +112,8 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
union acpi_operand_object *region_obj2;
void *region_context = NULL;
struct acpi_connection_info *context;
+ acpi_mutex context_mutex;
+ bool context_locked;
acpi_physical_address address;
ACPI_FUNCTION_TRACE(ev_address_space_dispatch);
@@ -136,6 +138,8 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
}
context = handler_desc->address_space.context;
+ context_mutex = handler_desc->address_space.context_mutex;
+ context_locked = false;
/*
* It may be the case that the region has never been initialized.
@@ -204,6 +208,23 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
handler = handler_desc->address_space.handler;
address = (region_obj->region.address + region_offset);
+ ACPI_DEBUG_PRINT((ACPI_DB_OPREGION,
+ "Handler %p (@%p) Address %8.8X%8.8X [%s]\n",
+ ®ion_obj->region.handler->address_space, handler,
+ ACPI_FORMAT_UINT64(address),
+ acpi_ut_get_region_name(region_obj->region.
+ space_id)));
+
+ if (!(handler_desc->address_space.handler_flags &
+ 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.
+ */
+ acpi_ex_exit_interpreter();
+ }
+
/*
* Special handling for generic_serial_bus and general_purpose_io:
* There are three extra parameters that must be passed to the @@ -212,6 +233,11 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
* 2) Length of the above buffer
* 3) Actual access length from the access_as() 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 general_purpose_io, the Address and bit_width fields
* are defined as follows:
* 1) Address is the pin number index of the field (bit offset from
@@ -221,6 +247,13 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
if ((region_obj->region.space_id == ACPI_ADR_SPACE_GSBUS) &&
context && field_obj) {
+ status = acpi_os_acquire_mutex(context_mutex, ACPI_WAIT_FOREVER);
+ if (ACPI_FAILURE(status)) {
+ goto re_enter_interpreter;
+ }
+
+ context_locked = true;
+
/* Get the Connection (resource_template) buffer */
context->connection = field_obj->field.resource_buffer; @@ -230,6 +263,13 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
if ((region_obj->region.space_id == ACPI_ADR_SPACE_GPIO) &&
context && field_obj) {
+ status = acpi_os_acquire_mutex(context_mutex, ACPI_WAIT_FOREVER);
+ if (ACPI_FAILURE(status)) {
+ goto re_enter_interpreter;
+ }
+
+ context_locked = true;
+
/* Get the Connection (resource_template) buffer */
context->connection = field_obj->field.resource_buffer; @@ -239,28 +279,14 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
bit_width = field_obj->field.bit_length;
}
- ACPI_DEBUG_PRINT((ACPI_DB_OPREGION,
- "Handler %p (@%p) Address %8.8X%8.8X [%s]\n",
- ®ion_obj->region.handler->address_space, handler,
- ACPI_FORMAT_UINT64(address),
- acpi_ut_get_region_name(region_obj->region.
- space_id)));
-
- if (!(handler_desc->address_space.handler_flags &
- 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.
- */
- acpi_ex_exit_interpreter();
- }
-
/* Call the handler */
status = handler(function, address, bit_width, value, context,
region_obj2->extra.region_context);
+ if (context_locked)
+ acpi_os_release_mutex(context_mutex);
+
if (ACPI_FAILURE(status)) {
ACPI_EXCEPTION((AE_INFO, status, "Returned by Handler for [%s]",
acpi_ut_get_region_name(region_obj->region.
@@ -277,6 +303,7 @@ acpi_ev_address_space_dispatch(union acpi_operand_object *region_obj,
}
}
+re_enter_interpreter:
if (!(handler_desc->address_space.handler_flags &
ACPI_ADDR_HANDLER_DEFAULT_INSTALLED)) {
/*
diff --git a/drivers/acpi/acpica/evxfregn.c b/drivers/acpi/acpica/evxfregn.c index da97fd0c6b51..74d0ee8a5736 100644
--- a/drivers/acpi/acpica/evxfregn.c
+++ b/drivers/acpi/acpica/evxfregn.c
@@ -201,6 +201,7 @@ acpi_remove_address_space_handler(acpi_handle device,
/* Now we can delete the handler object */
+ acpi_os_release_mutex(handler_obj->address_space.context_mutex);
acpi_ut_remove_reference(handler_obj);
goto unlock_and_exit;
}
--
2.28.0
