Hi,
On 20-11-18 23:39, Schmauss, Erik wrote:
-----Original Message-----
From: Rafael J. Wysocki [mailto:rjw@xxxxxxxxxxxxx]
Sent: Tuesday, November 20, 2018 2:03 PM
To: Hans de Goede <hdegoede@xxxxxxxxxx>
Cc: Len Brown <lenb@xxxxxxxxxx>; linux-acpi@xxxxxxxxxxxxxxx;
devel@xxxxxxxxxx; Moore, Robert <robert.moore@xxxxxxxxx>; Schmauss,
Erik <erik.schmauss@xxxxxxxxx>
Subject: Re: [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in
acpi_ex_write_data_to_field()
On Sunday, November 18, 2018 8:25:35 PM CET Hans de Goede wrote:
Generic Serial Bus transfers use a data struct like this:
struct gsb_buffer {
u8 status;
u8 len;
u8 data[0];
};
acpi_ex_write_data_to_field() copies the data which is to be written
from the source-buffer to a temp-buffer. This is done because the
OpReg-handler overwrites the status field and some transfers do a write +
read-back.
Commit f99b89eefeb6 ("ACPICA: Update for generic_serial_bus and
attrib_raw_process_bytes protocol") acpi_ex_write_data_to_field()
introduces a number of problems with this:
1) It drops a "length += 2" statement used to calculate the
temp-buffer size causing the temp-buffer to only be 1/2 bytes large
for byte/word transfers while it should be 3/4 bytes (taking the
status and len field into account). This is already fixed in commit
e324e10109fc ("ACPICA:
Update for field unit access") which refactors the code.
The ACPI 6.0 spec (ACPI_6.0.pdf) "5.5.2.4.5.2 Declaring and Using a
GenericSerialBusData Buffer" (page 232) states that the
GenericSerialBus Data Buffer Length field is only valid when doing a
Read/Write Block
(AttribBlock) transfer, but since the troublesome commit we
unconditionally use the len field to determine how much data to copy
from the source-buffer into the temp-buffer passed to the OpRegion.
This causes 3 further issues:
2) This may lead to not copying enough data to the temp-buffer causing
the OpRegion handler for the serial-bus to write garbage to the hardware.
3) The temp-buffer passed to the OpRegion is allocated to the size
returned by acpi_ex_get_serial_access_length(), which may be as little
as 1, so potentially this may lead to a write overflow of the temp-buffer.
4) Commit e324e10109fc ("ACPICA: Update for field unit access") drops
a length check on the source-buffer, leading to a potential read
overflow of the source-buffer.
This commit fixes all 3 remaining issues by not looking at the len
field at all (the interpretation of this field is left up to the
OpRegion handler), and copying the minimum of the source- and
temp-buffer sizes from the source-buffer to the temp-buffer.
This fixes e.g. an Acer S1003 no longer booting since the troublesome
commit.
Fixes: f99b89eefeb6 ("ACPICA: Update for generic_serial_bus and ...")
Fixes: e324e10109fc ("ACPICA: Update for field unit access")
Cc: Bob Moore <robert.moore@xxxxxxxxx>
Cc: Erik Schmauss <erik.schmauss@xxxxxxxxx>
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
I know that normally ACPICA patches go upstream through ACPICA
upstream, but since this is a somewhat nasty regression, I'm
submitting it directly to the ACPI subsys.
---
drivers/acpi/acpica/exserial.c | 21 ++-------------------
1 file changed, 2 insertions(+), 19 deletions(-)
diff --git a/drivers/acpi/acpica/exserial.c
b/drivers/acpi/acpica/exserial.c index 0d42f30e5b25..9920fac6413f
100644
--- a/drivers/acpi/acpica/exserial.c
+++ b/drivers/acpi/acpica/exserial.c
@@ -244,7 +244,6 @@ acpi_ex_write_serial_bus(union
acpi_operand_object
*source_desc, {
acpi_status status;
u32 buffer_length;
- u32 data_length;
void *buffer;
union acpi_operand_object *buffer_desc;
u32 function;
@@ -282,14 +281,12 @@ acpi_ex_write_serial_bus(union
acpi_operand_object *source_desc,
case ACPI_ADR_SPACE_SMBUS:
buffer_length = ACPI_SMBUS_BUFFER_SIZE;
- data_length = ACPI_SMBUS_DATA_SIZE;
function = ACPI_WRITE | (obj_desc->field.attribute << 16);
break;
case ACPI_ADR_SPACE_IPMI:
buffer_length = ACPI_IPMI_BUFFER_SIZE;
- data_length = ACPI_IPMI_DATA_SIZE;
function = ACPI_WRITE;
break;
@@ -310,7 +307,6 @@ acpi_ex_write_serial_bus(union
acpi_operand_object *source_desc,
/* Add header length to get the full size of the buffer */
buffer_length += ACPI_SERIAL_HEADER_SIZE;
- data_length = source_desc->buffer.pointer[1];
function = ACPI_WRITE | (accessor_type << 16);
break;
@@ -318,20 +314,6 @@ acpi_ex_write_serial_bus(union
acpi_operand_object *source_desc,
return_ACPI_STATUS(AE_AML_INVALID_SPACE_ID);
}
-#if 0
- OBSOLETE ?
- /* Check for possible buffer overflow */
- if (data_length > source_desc->buffer.length) {
- ACPI_ERROR((AE_INFO,
- "Length in buffer header (%u)(%u) is greater than "
- "the physical buffer length (%u) and will overflow",
- data_length, buffer_length,
- source_desc->buffer.length));
-
- return_ACPI_STATUS(AE_AML_BUFFER_LIMIT);
- }
-#endif
-
/* Create the transfer/bidirectional/return buffer */
buffer_desc = acpi_ut_create_buffer_object(buffer_length);
@@ -342,7 +324,8 @@ acpi_ex_write_serial_bus(union
acpi_operand_object *source_desc,
/* Copy the input buffer data to the transfer buffer */
buffer = buffer_desc->buffer.pointer;
- memcpy(buffer, source_desc->buffer.pointer, data_length);
+ memcpy(buffer, source_desc->buffer.pointer,
+ min(buffer_length, source_desc->buffer.length));
/* Lock entire transaction if requested */
Hi,
I've queued this up, but I would like Bob and Erik to speak up too.
Sorry for the late response.
With the US holiday approaching, we need a little more time for us to look at this.
There is a Kernel Bugzilla report on a similar issue and it would be nice for us to have more time to look at this.
What is the link for this issue, and have you asked the reporter to test my patch ?
Here are my thoughts so far:
a.) I'm not sure about the changes for the IPMI and SMBus cases... It might be fine.
My patch does 2 things:
1) Stop using the length field of the Generic Serial Bus data structure
2) Instead just copy as much data as we safely can without overrunning either the
source or destination (tmp) buffer
Since we should never overrun either buffer my patch copies to maximum amount
of data possible, leaving the interpretation of the length field to the OpRegion
handler. So the only downside it can have is copying more data then the OpRegion
handler will consume in which case the OpRegion handler will just ignore it.
IOW my patch is obviously correct, no matter which bus type is used (my patch is
not bus type specific at all).
Where as the current code which uses a value which is reserved and thus
uninitialized in many DSTDs to determine how much to copy, possibly overruning both
source and destination buffers is obviously incorrect.
b.) This part of the specification needs a lot of work and we have requested for this portion to be re-written. The spec is incorrect and referencing this as a part of the commit message might be confusing to people who look at it in the future...
I refer to the spec by version, currently written DSTDs are written to the current
spec which indeed is not clear on some parts, but it is very clear that the
Generic Serial Bus data structure length field is reserved for all transfers
which are not of the AttribBlock type. So (always) using the length field is simply
wrong.
The patches my patch build-on / fix workaround any ambiguity in the spec by just
sizing the buffer to 255 bytes (+ 2 bytes header) for all Attrib types which
may be considered variable length in some way.
My patch builds on top of this by simply always copying the maximum amount possible
without overrunning either buffer from the source-buffer to the temp-buffer passed
to the OpRegion handler. This leaves dealing with any ambiguity in the spec to the
OpRegion handler which has more specific knowledge about how to deal with this.
It would be better for us to hold off on this patch until next week when Bob and I can discuss in person.
I'm ok with waiting a bit with this, but we really need to get this fixed ASAP.
Regards,
Hans
