Fix attr read overflows #36003
Merged
Fix attr read overflows #36003
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
|
Review changes with |
|
PR #36003: Size comparison from a7bbd7b to 4436605 Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36003: Size comparison from 6880437 to 4e05b8f Full report (27 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, nxp, qpg, stm32, tizen)
|
witowsg-amazon
force-pushed
the
fix-attr-read-overflows
branch
3 times, most recently
from
c8380ca to
0599ec9
Compare
|
PR #36003: Size comparison from 6880437 to 0599ec9 Full report (83 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
witowsg-amazon
force-pushed
the
fix-attr-read-overflows
branch
from
0599ec9 to
b08064e
Compare
witowsg-amazon
force-pushed
the
fix-attr-read-overflows
branch
2 times, most recently
from
068f9e4 to
d636f39
Compare
|
PR #36003: Size comparison from 579b1b1 to d636f39 Full report (82 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
* Prevent buffer overflows by blocking creation of dynamic endpoints with attributes that would overflow the static attribute buffer * Added additional sanity checks to block reads of attributes that would overflow the attribute reads; return RESOURCE_EXHAUSTED instead
witowsg-amazon
force-pushed
the
fix-attr-read-overflows
branch
2 times, most recently
from
e74b31c to
30389a7
Compare
|
PR #36003: Size comparison from 629fea4 to 30389a7 Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
approved these changes
Feb 27, 2025
bzbarsky-apple
approved these changes
Mar 5, 2025
| size_t bufferSize = Compatibility::Internal::gEmberAttributeIOBufferSpan.size(); | ||
| for (uint8_t i = 0; i < ep->clusterCount; i++) | ||
| { | ||
| const EmberAfCluster * cluster = &(ep->cluster[i]); |
There was a problem hiding this comment.
Could also do:
Suggested change
| const EmberAfCluster * cluster = &(ep->cluster[i]); | |
| const EmberAfCluster & cluster = ep->cluster[i]; |
but either way.
There was a problem hiding this comment.
I'm going to leave the pointer-style since the structure is ultimately storing pointers -- in hindsight I probably should have done ep->cluster + i to just access the pointer directly.
There was a problem hiding this comment.
For what it's worth, the structure is not in fact storing pointers. It's storing EmberAfCluster objects.
witowsg-amazon
force-pushed
the
fix-attr-read-overflows
branch
from
30389a7 to
9d1204e
Compare
witowsg-amazon
force-pushed
the
fix-attr-read-overflows
branch
from
9d1204e to
d263b42
Compare
|
PR #36003: Size comparison from a579c8d to d263b42 Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
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.
Fixes 34937
RESOURCE_EXHAUSTEDinsteadTesting
ATTRIBUTE_LARGESTprovided by the ZAP headers results in an error being returned and that endpoint not being published on the node.Manual Tests
Performed A/B testing on my devices and confirmed that the fix worked as expected. Specifically:
The way this bug manifested for me was that the buffer was defaulting to size 8, I'd send a read for some external attributes (these were 32 bytes) from one device running the SDK (local) to another (remote). On the remote, I'd fill the buffer + size provided to me by
emberAfExternalAttributeReadCallback, this resulted in me unknowingly overflowing said buffer. Back on the local, after reading an external attribute, I'd try to subscribe to the parts list on the remote's EP0 and that would cause the remote's CHIP SDK to crash. After a bunch of painstaking debugging I finally traced it to the buffer overflow. These were my logs:Basically:
0xb23142b8 + 0x20 = 0xb23142d8, and0xb23142d8 > 0xb23142d4So by writing the amount of data the CHIP SDK told me was available for writing, I had actually overwritten part of the buffer that was storing the pre-defined attribute access interface for Endpoint 0, thereby resulting in the crash since the memory stored in that buffer was no longer valid.