Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix attribute reads into a large buffer to not read random memory. #4382

Merged
merged 1 commit into from
Jan 15, 2021
Merged

Fix attribute reads into a large buffer to not read random memory. #4382

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions src/app/util/attribute-storage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,33 +365,46 @@ static uint8_t * singletonAttributeLocation(EmberAfAttributeMetadata * am)
// This function does mem copy, but smartly, which means that if the type is a
// string, it will copy as much as it can.
// If src == NULL, then this method will set memory to zeroes
// See documentation for emAfReadOrWriteAttribute for the semantics of
// readLength when reading and writing.
static EmberAfStatus typeSensitiveMemCopy(uint8_t * dest, uint8_t * src, EmberAfAttributeMetadata * am, bool write,
uint16_t readLength)
{
EmberAfAttributeType attributeType = am->attributeType;
uint16_t size = (readLength == 0) ? am->size : readLength;
// readLength == 0 for a read indicates that we should just trust that the
// caller has enough space for an attribute...
bool ignoreReadLength = write || (readLength == 0);
Copy link
Contributor

@andy31415 andy31415 Jan 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the caller buffer size ignored when write is set? I would expect at least a min(readLength, am-size) and I think we should log on mismatch to catch logic errors.

Generally I have complains about this method to start with: a buffer length that seems to be only relevant on read (called readLength) but method used on both read and write and a direction parameter when the caller could just have flipped src and destination.

I generally would apply the test 'Can one clearly in one sentence describe what a method does, generally without 'IF' or conditionals?'. I think this method fails hard because it has a 'write' parameter that affects behaviour as well as special conditions for readLength.

Could we fix up this method and make it somehow easier to use?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the caller buffer size ignored when write is set?

Apart from that being the documented behavior of emAfReadOrWriteAttribute and all callers who set write passing 0 for the size? I'm afraid I don't know why the setup is the way it is; that's a question for @selissia or @tecimovic.

Could we fix up this method and make it somehow easier to use?

Yes, we could, and I agree we should. We'd need to audit all consumers to see what they are doing with readLength; the comments suggest that there are legacy consumers that pass 0 for reads as well....

I'm happy to do that, but it might be a few days before I can pull enough time off of spec stuff to do it. I'm happy to land this in the meantime to unblock your ASan changes and do the larger semantic-changing refactoring in a followup, or expand the scope of this PR, as long as you're OK with the corresponding lag. Please let me know.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

created #4394 to track improvement in buffer handling within this code.

Is this generated code? if so, how would one go about on changing the code and updating?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to close the loop on this: this is not generated code.

uint16_t bufferSize = ignoreReadLength ? am->size : readLength;

if (emberAfIsStringAttributeType(attributeType))
{
emberAfCopyString(dest, src, static_cast<uint8_t>(size - 1));
if (bufferSize < 1)
{
return EMBER_ZCL_STATUS_INSUFFICIENT_SPACE;
}
emberAfCopyString(dest, src, static_cast<uint8_t>(bufferSize - 1));
}
else if (emberAfIsLongStringAttributeType(attributeType))
{
emberAfCopyLongString(dest, src, static_cast<uint16_t>(size - 2));
if (bufferSize < 2)
{
return EMBER_ZCL_STATUS_INSUFFICIENT_SPACE;
}
emberAfCopyLongString(dest, src, static_cast<uint16_t>(bufferSize - 2));
}
else
{
if (!write && readLength != 0 && readLength < am->size)
if (!ignoreReadLength && readLength < am->size)
{
return EMBER_ZCL_STATUS_INSUFFICIENT_SPACE;
}
if (src == NULL)
{
memset(dest, 0, size);
memset(dest, 0, am->size);
}
else
{
memmove(dest, src, size);
memmove(dest, src, am->size);
}
}
return EMBER_ZCL_STATUS_SUCCESS;
Expand Down