Re: [Devel] [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()
2:03 p.m.
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(a)intel.com>
Cc: Erik Schmauss <erik.schmauss(a)intel.com>
Signed-off-by: Hans de Goede <hdegoede(a)redhat.com>
---
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 */
I've queued this up, but I would like Bob and Erik to speak up too.
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
2:39 p.m.
New subject: [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()
-----Original Message-----
From: Rafael J. Wysocki [mailto:[email protected]]
Sent: Tuesday, November 20, 2018 2:03 PM
To: Hans de Goede <hdegoede(a)redhat.com>
Cc: Len Brown <lenb(a)kernel.org>; linux-acpi(a)vger.kernel.org;
devel(a)acpica.org; Moore, Robert <robert.moore(a)intel.com>; Schmauss,
Erik <erik.schmauss(a)intel.com>
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(a)intel.com>
> Cc: Erik Schmauss <erik.schmauss(a)intel.com>
> Signed-off-by: Hans de Goede <hdegoede(a)redhat.com>
> ---
> 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
3:48 p.m.
New subject: [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()
On Tue, Nov 20, 2018 at 11:39 PM Schmauss, Erik <erik.schmauss(a)intel.com> wrote:
[cut]
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.
OK, fair enough.
2:33 p.m.
New subject: [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?)
2:41 p.m.
New subject: [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()
On Monday, November 26, 2018 11:33:25 PM CET Moore, Robert wrote:
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).
OK, so what about the $subject patch?
Machines don't boot without it and that's rather serious, so I'm inclined to
apply it as a quick fix for 4.20, unless you have a better idea.
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?)
7:11 p.m.
New subject: [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()
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.
Thanks,
Bob
-----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
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?)
1354
days inactive
1361
days old
5 comments
4 participants
participants (4)
-
Moore, Robert -
Rafael J. Wysocki -
Rafael J. Wysocki -
Schmauss, Erik