Prevent TLVWrite from possibly performing buffer overrun if trying to reserve #30714
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kkasperczyk-no,
kpschoedel,
ksperling-apple,
lazarkov and
lpbeliveau-silabs
|
PR #30714: Size comparison from b379158 to 21e888d Increases (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
Decreases (2 builds for linux)
Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
andy31415
approved these changes
Nov 30, 2023
|
I think the real overflow bug is that |
This shouldn't be the case according to the documentation of the |
You are right, I have remove that section now. Sorry about that |
ksperling-apple
approved these changes
Dec 5, 2023
bzbarsky-apple
approved these changes
Dec 11, 2023
|
PR #30714: Size comparison from b6e465a to 190c324 Increases (59 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, mbed, nrfconnect, psoc6, telink)
Decreases (39 builds for bl602, bl702, bl702l, cc13x4_26x4, efr32, esp32, linux, nrfconnect, qpg, telink)
Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
PR #30714: Size comparison from b6e465a to 7fb1c88 Increases (59 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, mbed, nrfconnect, psoc6, telink)
Decreases (39 builds for bl602, bl702, bl702l, cc13x4_26x4, efr32, esp32, linux, nrfconnect, qpg, telink)
Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
This was referenced Dec 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem:
TLVWriter::WriteDatamay callGetNewBufferto try getting more buffers for TLVWriter to continue writing large amounts of data into. When this happens inTLVWriter::WriteData, we do not properly account for the reserved memory. As a result, after getting the new buffer and callingUnreserveBufferthe Writer can write into memory beyond what was allocated resulting in buffer overrun.TLVWriter::WriteDatawere to try accounting for the reserved data, you would first need to fill in the rest of the buffer that is supposed to be reserved. After that you would be relying on 2 things that are not guaranteed. First thatGetNewBufferis successful. Secondly, if successful it is possible that the new buffer's size is smaller than what you had reserved.Alternative solution considered:
ReserveBuffertaking away frommRemainingLenyou take away frommMaxLen.TLVWriter::ReserveBuffer. As of today everything that callsTLVWriter::ReserveBufferis usingSystem::PacketBufferHandle::New(chip::app::kMaxSecureSduLengthBytes);. This means that theTLVBackingStorefor the TLVWriter that isTLVPacketBufferBackingStore. For this particular caseuseChainBuffers == false.PacketBufferHandlewill reserve the header for us, but doesn't reserve anything at the end which we are trying to do for closing TLV containers. In this casemRemainingLen != mMaxLen, andmMaxLenis not to be relied on to convey anything meaningful.Alternative not full explored, but might be better:
GetNewBufferfails until you call it, I am unsure how we could properly undo what you have written previously inTLVWriter::WriteData. Just because I don't know right now how to "rollback" doesn't me this solution doesn't exist.System::PacketBufferHandle::New(chip::app::kMaxSecureSduLengthBytes);whymMaxLencannot bemRemainingLen. If someone know why any thinks this is a viable option I can explore it more.Why things are implemented the way they are right now:
TLVWriter::ReserveBufferas we have always set useChainedBuffers to false. In order to keep usage safe I am trying to make sure what we are currently doing today is allowed why stopping incorrect possible usage.ReserveBuffersimilarly to how it is used in the SDK today, but didn't want to set more precedent of using it without closing out possible footgun for usages different then what is in the SDK today.Tests:
TLVWriter::ReserveBuffer