On Tue, Nov 27, 2018 at 9:39 AM Hans de Goede <hdegoede(a)redhat.com> wrote:
On 27-11-18 04:11, Moore, Robert wrote:
> Also, please send the acpidump for whatever machine is showing a problem.
> We want to look closely at the surface 3 code vs. the code for this machine.
Bob, I know you are unhappy with / frustrated about this part of the ACPI
specification (*), but please read the !@#$ patch description / commit
The current problem has very little to do with the specification being
ambiguous, the code currently in 4.20 is using a struct field which is
*clearly* marked in the specification as "reserved" as length argument
to a memcpy without checking that either the source or the destination
buffer is big enough.
Nor does it guarantee that we copy *enough* data when doing a write to
an i2c device through the GSB code, resulting in the OpRegion handler
writing whatever was in the tmp-buffer when we got it from
acpi_ut_create_buffer_object to the i2c-device, crashing the machine.
The description of the problem is clear to me and and the patch does
make it go away.
Question is if the patch as is clashes with any future ACPICA work,
because if it does, it may be better to do something else in
principle. So far this is still unclear, but this regression needs to
go away before final 4.20 and the patch still is there in my
I can give you the acpidump of the non-booting device but it is not
Sending it over wouldn't hurt though IMO, would it?
it is only using AttribByte accesses, the problem is
not the DSDT, the problem is the 4.20 code clearly, *obviously*, being
buggy, so please review the patch first.
*) IMHO this mostly is about the Surface line of products abusing the
specification, all other uses I've seen are very straight forward
So until recently it has not been clear to Bob/Erik if there are any
other production users of GenericSerialBus AFAICS and it would help to
give them ASL with it from whatever machines you have access to. I
would suggest opening a BZ for that and attaching some relevant
acpidump files in there, for future reference if nothing else.
If you make a claim like the above, it's rather good to support it
with some evidence. :-)
> -----Original Message-----
> From: Moore, Robert
> Sent: Monday, November 26, 2018 2:33 PM
> To: Schmauss, Erik <erik.schmauss(a)intel.com>; Rafael J. Wysocki
<rjw(a)rjwysocki.net>; Hans de Goede <hdegoede(a)redhat.com>
> Cc: Len Brown <lenb(a)kernel.org>; linux-acpi(a)vger.kernel.org; devel(a)acpica.org
> Subject: RE: [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in
> Looks like another round of issues with the completely ill-defined generic serial
bus. Below is a summary of the problems we have seen with the Length field.
> I have not had time to completely read through this thread yet, so I'm posting
some text that I wrote a month or two ago.
> Let me say this, however:
> The biggest (and it is very big) issue is that the GenericSerialBus interfaces are
so poorly defined in the ACPI specification that any implementation simply must guess at
the proper behavior (Both the ACPI implementation and any related drivers).
> Problems with these ACPI chapters:
> 188.8.131.52.6.2 Declaring and Using a GenericSerialBus Data Buffer
> 184.108.40.206.6.3 Using the GenericSerialBus Protocols
> 1) All of the ASL code examples do not compile due to syntax errors. There are
> 10 such examples, often with multiple syntax errors.
> 2) The basic/common data buffer format is not fully defined:
> typedef struct
> BYTE Status; // Byte 0 of the data buffer
> BYTE Length; // Byte 1 of the data buffer
> BYTE[x-1] Data; // Bytes 2-x of the arbitrary length data buffer,
> } // where x is the last index of the overall buffer
> The "Length" field is not defined. Is it the length of "Data" or
the Length of the entire structure (Status + Length + Data)?
> The example code from the spec illustrates how the "Length" term in the
structure above is undefined. Again, is it the "length of entire structure" or
is it the "length of the Data element"?. The example code makes it appear that
it is the length of the entire structure, but it is not fully clear because it is wrong in
> Name(BUFF, Buffer(34)()) // Create GenericSerialBus data buffer
> as BUFF
> CreateByteField(BUFF, 0x00, STAT) // STAT = Status (Byte)
> CreateByteField(BUFF, 0x01, LEN) // LEN = Length (Byte)
> CreateField(BUFF, 0x10, 256, DATA) // Data (Block)
> Store("ACPI", DATA) // Fill in outgoing data
> Store(8, LEN) // Length of the valid data
> (Actually should be 6? Or should it be 4?)