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.

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

Prevent buffer overflows due to misconfigured BLE MTU. #2398

Merged
merged 2 commits into from Mar 1, 2023

Conversation

nvt
Copy link
Member

@nvt nvt commented Jan 30, 2023

Thanks to @SWW13 and @Scepticz for reporting the issue.

#define BLE_L2CAP_NODE_MTU 1280
#define BLE_L2CAP_NODE_MTU PACKETBUF_SIZE
#endif

#if BLE_L2CAP_NODE_MTU != PACKETBUF_SIZE
#error Invalid BLE node MTU setting
#endif
Copy link
Member

Choose a reason for hiding this comment

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

I know nothing about BLE MTU, but are you not allowed to have a smaller MTU than the packetbuf? If not, and BLE_L2CAP_NODE_MTU must indeed be equal to PACKETBUF_SIZE, would it not be cleaner to simply remove BLE_L2CAP_CONF_NODE_MTU, as it would be moot?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the comment. The PR has been updated and this restriction has been removed. Moreover, the default value is now PACKETBUF_SIZE instead of 1280.

Copy link
Member

Choose a reason for hiding this comment

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

Don't you want to keep a check for for BLE_L2CAP_NODE_MTU > PACKETBUF_SIZE to provide a compile-time warning for misconfigurations? (if I understood the issue correctly)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I added a check for this now.

@nfi nfi merged commit 210c39d into contiki-ng:develop Mar 1, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants