On 27-11-18 09:39, Hans de Goede wrote:
Hi,
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
message.
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.
(or at least that is my theory)
I can give you the acpidump of the non-booting device but it is not doing
anything special, 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.
>
> Regards,
>
> Hans
p.s.
I spend my entire weekend last week bisecting this. IOW I've spend a
lot of time on this, so some more focus on the actual problem and the
patch would really be appreciated.
-----Original Message-----
From: Moore, Robert
Sent: Monday, November 26, 2018 2:33 PM
To: Schmauss, Erik <erik.schmauss@xxxxxxxxx>; Rafael J. Wysocki <rjw@xxxxxxxxxxxxx>; Hans de Goede <hdegoede@xxxxxxxxxx>
Cc: Len Brown <lenb@xxxxxxxxxx>; linux-acpi@xxxxxxxxxxxxxxx; devel@xxxxxxxxxx
Subject: RE: [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()
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).
Bob
Problems with these ACPI chapters:
5.5.2.4.6.2 Declaring and Using a GenericSerialBus Data Buffer
5.5.2.4.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 either case:
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?)
