-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
[spinel] check the frame buffer size before reading the next frame #10015
base: main
Are you sure you want to change the base?
[spinel] check the frame buffer size before reading the next frame #10015
Conversation
When the spinel rx multiple frame buffer is full, calling `GetNextSavedFrame()` may cause the code to access the area behind the multiple frame buffer. This commit checks the received frame buffer size before reading the next frame to avoid accessing illegal area.
Here is the backtrace we got when this issue happens.
|
Size Report of OpenThread
|
@@ -371,7 +371,7 @@ template <uint16_t kSize> class MultiFrameBuffer : public FrameWritePointer | |||
|
|||
aFrame = (aFrame == nullptr) ? mBuffer : aFrame + aLength; | |||
|
|||
if (HasSavedFrame() && (aFrame != mWriteFrameStart)) | |||
if (HasSavedFrame() && (aFrame != mWriteFrameStart) && ((aFrame + kHeaderSize) < GetArrayEnd(mBuffer))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why will there be an incomplete frame while HasSavedFrame()
is true? Should we avoid saving incomplete frame?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function GetNextSavedFrame ()
only iterate frames in the multi-frame-buffer. All frames are still be saved in the multi-frame-buffer when calling GetNextSavedFrame()
, so the function HasSavedFrame ()
does the right job here.
@@ -371,7 +371,7 @@ template <uint16_t kSize> class MultiFrameBuffer : public FrameWritePointer | |||
|
|||
aFrame = (aFrame == nullptr) ? mBuffer : aFrame + aLength; | |||
|
|||
if (HasSavedFrame() && (aFrame != mWriteFrameStart)) | |||
if (HasSavedFrame() && (aFrame != mWriteFrameStart) && ((aFrame + kHeaderSize) < GetArrayEnd(mBuffer))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhanglongxia can you please share how we may get into bad state where we would need the ((aFrame + kHeaderSize) < GetArrayEnd(mBuffer))
.
I tried to check the code, but cannot see how this can happen.
In SaveFrame()
we have a check which makes sure we have bytes left for the next frame header, otherwise we fail (not save the frame and not change mWriteFrameStart
).
otError SaveFrame(void)
{
otError error = OT_ERROR_NONE;
// If the next header will overflow the buffer, we can't save the frame.
if (!CanWrite(kHeaderSize))
{
error = OT_ERROR_NO_BUFS;
}
else
{
LittleEndian::WriteUint16(GetSkipLength() + GetLength(), mWriteFrameStart + kHeaderTotalLengthOffset);
mWriteFrameStart = mWritePointer;
IgnoreError(SetSkipLength(0));
}
return error;
}
So we should always have at least kHeaderSize
remaining in buffer after mWriteFrameStart
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhanglongxia I think this was addressed in
When the spinel rx multiple frame buffer is full, calling
GetNextSavedFrame()
may cause the code to access the area behind the multiple frame buffer.This commit checks the received frame buffer size before reading the next frame to avoid accessing illegal area.