Re: [Devel] [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()

Re: [Devel] [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 5109 bytes --]

On Tue, Nov 27, 2018 at 9:39 AM Hans de Goede <hdegoede(a)redhat.com> 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.

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
linux-next branch.

> I can give you the acpidump of the non-booting device but it is not doing
> anything special,

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 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?)
> >
> >
> >

Re: [Devel] [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

On Thu, Nov 29, 2018 at 12:10 PM Benjamin Tissoires <btissoir(a)redhat.com> wrote:
>
> On Wed, Nov 28, 2018 at 6:48 PM Hans de Goede <hdegoede(a)redhat.com> wrote:
> >
> > Hi,
> >
> > On 28-11-18 18:18, Schmauss, Erik wrote:
> >
> > <big snip>
> >
> > >> Repeating myself I'm somewhat surprised by your and Bob's objections
> > >> against my approach, since it gets acpica out of the game of having to guess
> > >> what the LEN field means / when to use then LEN field and I expected that
> > >> both you and Bob would actually be quite happy to no longer having to do
> > >> that.
> > > This is a good point..
> > >
> > > Bob and I have talked it over. We'll accept your patch as long as your surface 3 also
> > > boots and behaves as expected with this patch
> >
> > I don't have a surface 3, I got involved in the whole surface 3 discussion
> > because I was trying to help to get the battery monitoring support for it
> > upstream, but I don't have one myself.
> >
> > Still I don't see how this can break the surface 3, since we simply end up
> > potentially copying more data to the tmp-buffer then before my patch and
> > if the Surface 3's opregion driver does not expect that data to be there
> > it will simply ignore it.
> >
> > I will ask Benjamin Tissoires to test 4.20 + my patch on his Surface 3
> > to double-check, but in the mean time I believe it is really time to get
> > this fix into 4.20 now.
>
> Thanks Hans for all of this work. And thanks everybody for taking care
> of this problem.
>
> I just tested the patch on top of 4.20-rc, and I can confirm that this
> doesn't regress on the Surface 3.
> So it is fine from my HW point of view to apply it.

Thank you for the confirmation.

I'm going to send a pull request including this patch later today.

Re: [Devel] [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()

[Moore, Robert]

[-- Attachment #1: Type: text/plain, Size: 11915 bytes --]



> -----Original Message-----
> From: Schmauss, Erik
> Sent: Wednesday, November 28, 2018 9:18 AM
> To: Hans de Goede <hdegoede(a)redhat.com>; Rafael J. Wysocki
> <rafael(a)kernel.org>
> Cc: Moore, Robert <robert.moore(a)intel.com>; Rafael J. Wysocki
> <rjw(a)rjwysocki.net>; Len Brown <lenb(a)kernel.org>; ACPI Devel Maling List
> <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()
> 
> 
> 
> > -----Original Message-----
> > From: linux-acpi-owner(a)vger.kernel.org [mailto:linux-acpi-
> > owner(a)vger.kernel.org] On Behalf Of Hans de Goede
> > Sent: Wednesday, November 28, 2018 3:16 AM
> > To: Schmauss, Erik <erik.schmauss(a)intel.com>; Rafael J. Wysocki
> > <rafael(a)kernel.org>
> > Cc: Moore, Robert <robert.moore(a)intel.com>; Rafael J. Wysocki
> > <rjw(a)rjwysocki.net>; Len Brown <lenb(a)kernel.org>; ACPI Devel Maling
> > List <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()
> >
> > Hi Erik,
> >
> > On 28-11-18 01:05, Schmauss, Erik wrote:
> >
> > <snip>
> >
> > > Hi Hans,
> > >
> > > Thanks for your patience
> > >
> > >> I do not think that is the case. A lot of laptops out there use the
> > >> GenericSerialBus interface in the form of an I2cSerialBus for
> > >> Battery monitoring, but all these laptops pretty much exclusively
> > >> use GenericSerialBus OpRegion accesses with AccessAs set to either
> > >> AttribByte or AttribBytes and that has been working fine for years
> > >> and the spec is clear enough on how to interpret these (esp. in
> > >> combination with the DSTDs using them which make the intention
> clear).
> > >>
> > >> The problem with the Surface line of devices is that the use
> > >> AccessAs AttribRawProcessBytes on which the spec is a bit handwavy,
> > >> the patch in 4.20 to fix AttribRawProcessBytes breaks AttribByte
> > >> and AttribBytes accesses, because for those the length field of the
> > >> ACPI buffer passed when writing the field is reserved, so we should
> not use it.
> > >
> > > Thanks for this data point
> > >
> > >>
> > >> Frankly given that using GenericSerialBus OpRegion accesses with
> > >> AccessAs set to either AttribByte or AttribBytes is very common,
> > >> I'm amazed that there have not been more regressions reported
> > >> against 4.20. Note these do not necessarily have to end up as
> > >> system not booting, but could also e.g. be battery monitoring
> behaving weirdly.
> > >>
> > >> Note I was involved in the acpica discussion surrounding the
> > >> patches to deal with AttribRawProcessBytes, only I was so focused
> > >> on AttribRawProcessBytes that I failed to see they would break
> > >> AttribByte and AttribBytes accesses at that time, only to have it
> > >> poetically bite myself in the ass and at the end of a long bisect
> > >> end up at the very patch I was sideways involved with.
> > >
> > > We looked at the spec and your patch and we came to the following
> > conclusions:
> > >
> > > I will refer to the contents of the original patch using the #
> > > character to avoid confusion with >. In the original patch you
> stated this:
> > >
> > > #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.
> > >
> > > The spec is wrong on this part. LEN is used in AttribBlock,
> > > AttribBlockProcessCall, and AttribRawProcessBytes,
> >
> > Right, but it is NOT used for AttribByte or AttribBytes which are way
> > more common and currently broken in 4.20 because of this.
> >
> > > we need to know how much data to copy.
> >
> > But do we really? The tmp-buffer allocated to pass to the OpRegion
> > handler is sized by acpi_ex_get_protocol_buffer_length() at 255 to
> > which 2 bytes get added for the header.
> >
> > This is correct, since this may be an in/out buffer and the LEN field
> > may be changed to indicate that more data was returned then passed in,
> > since we do not know the size of the returned data when the buffer is
> > also used as an out buffer, we must make it large enough that the data
> > should always fit, just as we do in the read paths.
> >
> > I really don't see why the exfield.c / exserial.c code needs to check
> > LEN, it can simply copy all data it has in the src buffer (or as much
> > as will fit if the source buffer is bigger then the tmp-buffer) and
> > let the OpRegion handler figure out how to interpret LEN without
> > worrying about the ambiguity in the spec surrounding
> AttribBlockProcessCall and AttribRawProcessBytes.
> >
> > Note that how much data has been decided to copy is not passed to the
> > OpRegion handler at all.
> >
> > I would expect Bob and you to actually like my approach because it
> > delegates the problem of interpreting LEN to the OpRegion handler
> > which has more domain specific knowledge (and may be custom to the
> > device such as in the Surface 3 battery monitoring case).
> >
> > > Len has to be
> > > correct for the handler. Your solution is treating AttribBlock,
> > > AttribBlockProcessCall, and AttribRawProcessBytes in the same way as
> > > everything else. I think we can try to attempt to "honor" the spirit
> > > of these
> > protocols by looking at LEN in certain cases.
> > > However, LEN is not always reliable.
> > >
> > > #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.
> > >
> > > The temp buffer is at least 2. We missed adding 2 to the
> DataLengthField.
> >
> > Right I missed that while writing the commit message, but the +2
> > really does not make a difference. Since the 4.20 code currently does
> > a memcpy using the reserved LEN field for AttribByte, so whether we
> > make the tmp-buffer 1 or 3 bytes big, we still end up copying up to
> > 255 bytes to it which is a clear buffer overrun.
> >
> > > With the new approach, we let the temporary buffer be of the max
> size.
> > > Since LEN is
> > > 8 bits, we can only have a max data length of 256. Accounting for
> > > the STAT and LEN fields, the Temp Buffer should be 258. In other
> > > words, we can't overflow the buffer but we definitely need to choose
> > > wisely between
> > the two.
> > >
> > > Anyway aside from all of those details, the primary issue here is
> > > the DataLength and also how to go about choosing the appropriate
> > > one. This will give the handler the correct data to deal with.
> >
> > My fix is to simply give the handler *all* data we have and let it
> > figure things out as I said above, this delegates the interpreting of
> > the LEN field to the OpRegion handler nicely solving the problem
> > without needing needing to worry "about choosing the appropriate one"
> at all.
> >
> > > There are hints in the spec that imply that LEN is important for
> > > some protocols but the value of LEN cannot be trusted so we need to
> > > make sure to check to make sure that this value is sane.
> > >
> > > Could you modify your patch so that we take the DataLength as
> > > correct as
> > possible?
> >
> > Why? There is no need for this and it needlessly complicates things,
> > doing this does not win us anything at all. On the contrary it just
> > introduces the possibility of getting things wrong again leading to
> > more bugs now or in the future.
> >
> > > Here's what I'm thinking:
> > >
> > > AttribQuick - data length can be one or greater but with the
> > > structure of
> > these buffers,
> > >                            2 might be easier for the handler to use.
> >
> > I've no idea what the correct data length would be for AttribQuick
> >
> > > AttribSendRecieve - data length can be 3 (2 for STAT/ LEN header + 1
> > > data)
> >
> > Ack.
> >
> > > AttribByte - datalength can be 3 (same as above)
> >
> > Ack.
> >
> > > AttribWord - datalength can be 4 (2 header + 2 data)
> >
> > Ack.
> >
> > > AttribBlock - datalength should be LEN unless LEN is greater than
> > > the
> > source buffer.
> > >                            In this case, DataLength should be the
> > > SourceBuffer's Length or  the max (whichever one is smaller)
> >
> > Ack.
> >
> > > AttribProcessCall - should be 4 (2 header + 2 data)
> >
> > No idea again.
> >
> > > AttribBlockProcessCall - same as AttribBlock
> >
> > No idea again.
> >
> > > AttribBytes - ignore AttribBytesFiedl and use LEN like AttribBlock
> >
> > Wrong this uses the access_length field associated with the field, not
> > the LEN field.
> >
> > > AttribRawBytes - same as AttribBytes (this one is confusing)
> >
> > No idea again.
> >
> > > AtrribRawProcessBytes - same as AttribBytes
> >
> > The one implementation that we know of uses LEN indeed, but no idea
> > about potential future implementations.
> >
> > ***
> >
> > So doing what you suggest would require introducing a new
> > acpi_ex_get_serial_data_length function implementing the switch-case
> > you've suggested above.
> >
> > Then this switch-case would contain partly guess work and to make sure
> > we don't overrun anything the actual memcpy would become:
> >
> > 	memcpy(buffer, source_desc->buffer.pointer,
> > 	       min(buffer_length, min(source_desc->buffer.length,
> > data_length)));
> >
> > > I think this would be a better solution. Let me know what you think
> > > and
> > apologies for the slow response.
> >
> > I really do not see how adding a new acpi_ex_get_serial_data_length
> > function based on a bunch of guess work, which likely will be proven
> > wrong in the future, is better then my solution.
> >
> > The *only* effective result of all this extra code would be that we
> > would maybe memcpy some less data then for the cases where
> > acpi_ex_get_serial_access_length returns 255. Nothing else would
> > change, the OpRegion handlers still need to interpret LEN themselves
> > as we don't pass data_length to them, only now they potentially do not
> > have the data they need in the buffer we pass them because we did not
> copy it.
> >
> > I completely fail to see how this would be better, we add extra code,
> > based on a bunch of guesses. The only thing this can do is introduce
> > more bugs, see e.g. how you got the data-length for AtrribBytes wrong.
> 
> Right, it really is a bunch of guesses at this point...
> >
> > Repeating myself I'm somewhat surprised by your and Bob's objections
> > against my approach, since it gets acpica out of the game of having to
> > guess what the LEN field means / when to use then LEN field and I
> > expected that both you and Bob would actually be quite happy to no
> > longer having to do that.
> This is a good point..
> 
> Bob and I have talked it over. We'll accept your patch as long as your
> surface 3 also boots and behaves as expected with this patch
> 
> Thanks for your efforts!
> Erik
> >
> > Regards,
> >
> > Hans
[Moore, Robert] 

Agreed. We simply have had a few questions, but your changes are fundamentally OK with us.

Unfortunately, we are stuck with this:
1) Must be windows compatible.
2) "Engineering" by guessing and trial & error

Thanks again for your help on this.
Bob

Re: [Devel] [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()

[Schmauss, Erik]

[-- Attachment #1: Type: text/plain, Size: 10584 bytes --]



> -----Original Message-----
> From: linux-acpi-owner(a)vger.kernel.org [mailto:linux-acpi-
> owner(a)vger.kernel.org] On Behalf Of Hans de Goede
> Sent: Wednesday, November 28, 2018 3:16 AM
> To: Schmauss, Erik <erik.schmauss(a)intel.com>; Rafael J. Wysocki
> <rafael(a)kernel.org>
> Cc: Moore, Robert <robert.moore(a)intel.com>; Rafael J. Wysocki
> <rjw(a)rjwysocki.net>; Len Brown <lenb(a)kernel.org>; ACPI Devel Maling List
> <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()
> 
> Hi Erik,
> 
> On 28-11-18 01:05, Schmauss, Erik wrote:
> 
> <snip>
> 
> > Hi Hans,
> >
> > Thanks for your patience
> >
> >> I do not think that is the case. A lot of laptops out there use the
> >> GenericSerialBus interface in the form of an I2cSerialBus for Battery
> >> monitoring, but all these laptops pretty much exclusively use
> >> GenericSerialBus OpRegion accesses with AccessAs set to either
> >> AttribByte or AttribBytes and that has been working fine for years
> >> and the spec is clear enough on how to interpret these (esp. in
> >> combination with the DSTDs using them which make the intention clear).
> >>
> >> The problem with the Surface line of devices is that the use AccessAs
> >> AttribRawProcessBytes on which the spec is a bit handwavy, the patch
> >> in 4.20 to fix AttribRawProcessBytes breaks AttribByte and
> >> AttribBytes accesses, because for those the length field of the ACPI
> >> buffer passed when writing the field is reserved, so we should not use it.
> >
> > Thanks for this data point
> >
> >>
> >> Frankly given that using GenericSerialBus OpRegion accesses with
> >> AccessAs set to either AttribByte or AttribBytes is very common, I'm
> >> amazed that there have not been more regressions reported against
> >> 4.20. Note these do not necessarily have to end up as system not
> >> booting, but could also e.g. be battery monitoring behaving weirdly.
> >>
> >> Note I was involved in the acpica discussion surrounding the patches
> >> to deal with AttribRawProcessBytes, only I was so focused on
> >> AttribRawProcessBytes that I failed to see they would break
> >> AttribByte and AttribBytes accesses at that time, only to have it
> >> poetically bite myself in the ass and at the end of a long bisect end
> >> up at the very patch I was sideways involved with.
> >
> > We looked at the spec and your patch and we came to the following
> conclusions:
> >
> > I will refer to the contents of the original patch using the #
> > character to avoid confusion with >. In the original patch you stated this:
> >
> > #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.
> >
> > The spec is wrong on this part. LEN is used in AttribBlock,
> > AttribBlockProcessCall, and AttribRawProcessBytes,
> 
> Right, but it is NOT used for AttribByte or AttribBytes which are way more
> common and currently broken in 4.20 because of this.
> 
> > we need to know how much data to copy.
> 
> But do we really? The tmp-buffer allocated to pass to the OpRegion handler
> is sized by acpi_ex_get_protocol_buffer_length() at 255 to which 2 bytes get
> added for the header.
> 
> This is correct, since this may be an in/out buffer and the LEN field may be
> changed to indicate that more data was returned then passed in, since we do
> not know the size of the returned data when the buffer is also used as an out
> buffer, we must make it large enough that the data should always fit, just as
> we do in the read paths.
> 
> I really don't see why the exfield.c / exserial.c code needs to check LEN, it can
> simply copy all data it has in the src buffer (or as much as will fit if the source
> buffer is bigger then the tmp-buffer) and let the OpRegion handler figure out
> how to interpret LEN without worrying about the ambiguity in the spec
> surrounding AttribBlockProcessCall and AttribRawProcessBytes.
> 
> Note that how much data has been decided to copy is not passed to the
> OpRegion handler at all.
> 
> I would expect Bob and you to actually like my approach because it delegates
> the problem of interpreting LEN to the OpRegion handler which has more
> domain specific knowledge (and may be custom to the device such as in the
> Surface 3 battery monitoring case).
> 
> > Len has to be
> > correct for the handler. Your solution is treating AttribBlock,
> > AttribBlockProcessCall, and AttribRawProcessBytes in the same way as
> > everything else. I think we can try to attempt to "honor" the spirit of these
> protocols by looking at LEN in certain cases.
> > However, LEN is not always reliable.
> >
> > #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.
> >
> > The temp buffer is at least 2. We missed adding 2 to the DataLengthField.
> 
> Right I missed that while writing the commit message, but the +2 really does
> not make a difference. Since the 4.20 code currently does a memcpy using
> the reserved LEN field for AttribByte, so whether we make the tmp-buffer 1
> or 3 bytes big, we still end up copying up to 255 bytes to it which is a clear
> buffer overrun.
> 
> > With the new approach, we let the temporary buffer be of the max size.
> > Since LEN is
> > 8 bits, we can only have a max data length of 256. Accounting for the
> > STAT and LEN fields, the Temp Buffer should be 258. In other words, we
> > can't overflow the buffer but we definitely need to choose wisely between
> the two.
> >
> > Anyway aside from all of those details, the primary issue here is the
> > DataLength and also how to go about choosing the appropriate one. This
> > will give the handler the correct data to deal with.
> 
> My fix is to simply give the handler *all* data we have and let it figure things
> out as I said above, this delegates the interpreting of the LEN field to the
> OpRegion handler nicely solving the problem without needing needing to
> worry "about choosing the appropriate one" at all.
> 
> > There are hints in the spec that imply that LEN is important for some
> > protocols but the value of LEN cannot be trusted so we need to make
> > sure to check to make sure that this value is sane.
> >
> > Could you modify your patch so that we take the DataLength as correct as
> possible?
> 
> Why? There is no need for this and it needlessly complicates things, doing
> this does not win us anything at all. On the contrary it just introduces the
> possibility of getting things wrong again leading to more bugs now or in the
> future.
> 
> > Here's what I'm thinking:
> >
> > AttribQuick - data length can be one or greater but with the structure of
> these buffers,
> >                            2 might be easier for the handler to use.
> 
> I've no idea what the correct data length would be for AttribQuick
> 
> > AttribSendRecieve - data length can be 3 (2 for STAT/ LEN header + 1
> > data)
> 
> Ack.
> 
> > AttribByte - datalength can be 3 (same as above)
> 
> Ack.
> 
> > AttribWord - datalength can be 4 (2 header + 2 data)
> 
> Ack.
> 
> > AttribBlock - datalength should be LEN unless LEN is greater than the
> source buffer.
> >                            In this case, DataLength should be the
> > SourceBuffer's Length or  the max (whichever one is smaller)
> 
> Ack.
> 
> > AttribProcessCall - should be 4 (2 header + 2 data)
> 
> No idea again.
> 
> > AttribBlockProcessCall - same as AttribBlock
> 
> No idea again.
> 
> > AttribBytes - ignore AttribBytesFiedl and use LEN like AttribBlock
> 
> Wrong this uses the access_length field associated with the field, not the LEN
> field.
> 
> > AttribRawBytes - same as AttribBytes (this one is confusing)
> 
> No idea again.
> 
> > AtrribRawProcessBytes - same as AttribBytes
> 
> The one implementation that we know of uses LEN indeed, but no idea
> about potential future implementations.
> 
> ***
> 
> So doing what you suggest would require introducing a new
> acpi_ex_get_serial_data_length function implementing the switch-case
> you've suggested above.
> 
> Then this switch-case would contain partly guess work and to make sure we
> don't overrun anything the actual memcpy would become:
> 
> 	memcpy(buffer, source_desc->buffer.pointer,
> 	       min(buffer_length, min(source_desc->buffer.length,
> data_length)));
> 
> > I think this would be a better solution. Let me know what you think and
> apologies for the slow response.
> 
> I really do not see how adding a new acpi_ex_get_serial_data_length
> function based on a bunch of guess work, which likely will be proven wrong
> in the future, is better then my solution.
> 
> The *only* effective result of all this extra code would be that we would
> maybe memcpy some less data then for the cases where
> acpi_ex_get_serial_access_length returns 255. Nothing else would change,
> the OpRegion handlers still need to interpret LEN themselves as we don't
> pass data_length to them, only now they potentially do not have the data
> they need in the buffer we pass them because we did not copy it.
> 
> I completely fail to see how this would be better, we add extra code, based
> on a bunch of guesses. The only thing this can do is introduce more bugs, see
> e.g. how you got the data-length for AtrribBytes wrong.

Right, it really is a bunch of guesses at this point...
> 
> Repeating myself I'm somewhat surprised by your and Bob's objections
> against my approach, since it gets acpica out of the game of having to guess
> what the LEN field means / when to use then LEN field and I expected that
> both you and Bob would actually be quite happy to no longer having to do
> that.
This is a good point..

Bob and I have talked it over. We'll accept your patch as long as your surface 3 also
boots and behaves as expected with this patch

Thanks for your efforts!
Erik
> 
> Regards,
> 
> Hans

Re: [Devel] [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()

[Schmauss, Erik]

[-- Attachment #1: Type: text/plain, Size: 11074 bytes --]



> -----Original Message-----
> From: Hans de Goede [mailto:hdegoede(a)redhat.com]
> Sent: Tuesday, November 27, 2018 11:21 AM
> To: Rafael J. Wysocki <rafael(a)kernel.org>
> Cc: Moore, Robert <robert.moore(a)intel.com>; Schmauss, Erik
> <erik.schmauss(a)intel.com>; Rafael J. Wysocki <rjw(a)rjwysocki.net>; Len
> Brown <lenb(a)kernel.org>; ACPI Devel Maling List <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()
> 
> Hi,
> 
> On 27-11-18 17:40, Rafael J. Wysocki wrote:
> > On Tue, Nov 27, 2018 at 9:39 AM Hans de Goede <hdegoede(a)redhat.com>
> 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.
> >
> > 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
> > linux-next branch.
> >
> >> I can give you the acpidump of the non-booting device but it is not
> >> doing anything special,
> >
> > Sending it over wouldn't hurt though IMO, would it?
> 
> True, here it is: https://fedorapeople.org/~jwrdegoede/acpidump.acer-
> One-S1003
> 
> >> 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.
> 

Hi Hans,

Thanks for your patience

> I do not think that is the case. A lot of laptops out there use the
> GenericSerialBus interface in the form of an I2cSerialBus for Battery
> monitoring, but all these laptops pretty much exclusively use
> GenericSerialBus OpRegion accesses with AccessAs set to either AttribByte
> or AttribBytes and that has been working fine for years and the spec is clear
> enough on how to interpret these (esp. in combination with the DSTDs using
> them which make the intention clear).
> 
> The problem with the Surface line of devices is that the use AccessAs
> AttribRawProcessBytes on which the spec is a bit handwavy, the patch in 4.20
> to fix AttribRawProcessBytes breaks AttribByte and AttribBytes accesses,
> because for those the length field of the ACPI buffer passed when writing
> the field is reserved, so we should not use it.

Thanks for this data point

> 
> Frankly given that using GenericSerialBus OpRegion accesses with AccessAs
> set to either AttribByte or AttribBytes is very common, I'm amazed that there
> have not been more regressions reported against 4.20. Note these do not
> necessarily have to end up as system not booting, but could also e.g. be
> battery monitoring behaving weirdly.
> 
> Note I was involved in the acpica discussion surrounding the patches to deal
> with AttribRawProcessBytes, only I was so focused on
> AttribRawProcessBytes that I failed to see they would break AttribByte and
> AttribBytes accesses at that time, only to have it poetically bite myself in the
> ass and at the end of a long bisect end up at the very patch I was sideways
> involved with.

We looked at the spec and your patch and we came to the following conclusions:

I will refer to the contents of the original patch using the # character to avoid
confusion with >. In the original patch you stated this:

#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.

The spec is wrong on this part. LEN is used in AttribBlock, AttribBlockProcessCall, and
AttribRawProcessBytes, we need to know how much data to copy. Len has to be
correct for the handler. Your solution is treating AttribBlock, AttribBlockProcessCall,
and AttribRawProcessBytes in the same way as everything else. I think we can try to
attempt to "honor" the spirit of these protocols by looking at LEN in certain cases.
However, LEN is not always reliable.

#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.

The temp buffer is at least 2. We missed adding 2 to the DataLengthField.

With the new approach, we let the temporary buffer be of the max size. Since LEN is
8 bits, we can only have a max data length of 256. Accounting for the STAT and LEN
fields, the Temp Buffer should be 258. In other words, we can't overflow the buffer
but we definitely need to choose wisely between the two.

Anyway aside from all of those details, the primary issue here is the DataLength
and also how to go about choosing the appropriate one. This will give the handler the
correct data to deal with.

There are hints in the spec that imply that LEN is important for some protocols but the
value of LEN cannot be trusted so we need to make sure to check to make sure that
this value is sane.

Could you modify your patch so that we take the DataLength as correct as possible?

Here's what I'm thinking:

AttribQuick - data length can be one or greater but with the structure of these buffers,
                          2 might be easier for the handler to use.
AttribSendRecieve - data length can be 3 (2 for STAT/ LEN header + 1 data)
AttribByte - datalength can be 3 (same as above)
AttribWord - datalength can be 4 (2 header + 2 data)
AttribBlock - datalength should be LEN unless LEN is greater than the source buffer.
                          In this case, DataLength should be the SourceBuffer's Length or  the max (whichever one is smaller)
AttribProcessCall - should be 4 (2 header + 2 data)
AttribBlockProcessCall - same as AttribBlock
AttribBytes - ignore AttribBytesFiedl and use LEN like AttribBlock
AttribRawBytes - same as AttribBytes (this one is confusing)
AtrribRawProcessBytes - same as AttribBytes

I think this would be a better solution. Let me know what you think and apologies for the slow response.

Erik

> 
> Regards,
> 
> Hans
> 
> 
> 
> > 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 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?)
> >>>
> >>>
> >>>

Re: [Devel] [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()

[Moore, Robert]

[-- Attachment #1: Type: text/plain, Size: 2712 bytes --]

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?)

Re: [Devel] [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 2451 bytes --]

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?)
> 
> 
> 

Re: [Devel] [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()

[Moore, Robert]

[-- Attachment #1: Type: text/plain, Size: 2122 bytes --]

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?)

Re: [Devel] [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 934 bytes --]

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.

Re: [Devel] [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()

[Schmauss, Erik]

[-- Attachment #1: Type: text/plain, Size: 7126 bytes --]



> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw(a)rjwysocki.net]
> 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
> 

Re: [Devel] [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in acpi_ex_write_data_to_field()

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 5642 bytes --]

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