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

[Moore, Robert <robert.moore at intel.com>]

[-- 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