> -----Original Message----- > From: linux-acpi-owner@xxxxxxxxxxxxxxx [mailto:linux-acpi- > owner@xxxxxxxxxxxxxxx] On Behalf Of Hans de Goede > Sent: Wednesday, November 28, 2018 3:16 AM > To: Schmauss, Erik <erik.schmauss@xxxxxxxxx>; Rafael J. Wysocki > <rafael@xxxxxxxxxx> > Cc: Moore, Robert <robert.moore@xxxxxxxxx>; Rafael J. Wysocki > <rjw@xxxxxxxxxxxxx>; Len Brown <lenb@xxxxxxxxxx>; ACPI Devel Maling List > <linux-acpi@xxxxxxxxxxxxxxx>; devel@xxxxxxxxxx > Subject: Re: [PATCH 4.20 regression fix] ACPICA: Fix handling of buffer-size in > acpi_ex_write_data_to_field() > > 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
