> -----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. Here are my thoughts so far: a.) I'm not sure about the changes for the IPMI and SMBus cases... It might be fine. 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... It would be better for us to hold off on this patch until next week when Bob and I can discuss in person. Erik > > A boot crash is a good enough reason for doing a quick fix for 4.20, but it may > be necessary to do something less straightforward long term. > > Thanks, > Rafael >
