From: Schmauss, Erik
Sent: Wednesday, November 28, 2018 9:18 AM
To: Hans de Goede <hdegoede(a)redhat.com>; Rafael J. Wysocki
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
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
> 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
> Hi Erik,
> On 28-11-18 01:05, Schmauss, Erik wrote:
> > 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
> >> 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
> >> 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
> > I will refer to the contents of the original patch using the #
> > character to avoid confusion with >. In the original patch you
> > #The ACPI 6.0 spec (ACPI_6.0.pdf) "188.8.131.52.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
> > 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
> > 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-
> > The temp buffer is at least 2. We missed adding 2 to the
> 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
> > 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"
> > 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
> 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)
> > 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)
> 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,
> > 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
> 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!
Agreed. We simply have had a few questions, but your changes are fundamentally OK with
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.