11
29
Fork
You've already forked ipmitool
33

open: Eliminate buffer overrun #24

Merged
AlexanderAmelkin merged 2 commits from Howitzer105mm/ipmitool:bugfix/23-clangd-buffer-overrun-in-open-plugin into master 2024年03月26日 12:28:16 +01:00
Contributor
Copy link

clangd reports a buffer overrun issue.
The sprintf() used to fill ipmi_devfs2 requires 17 bytes to store the
null terminated string. The character buffer is only 16 bytes in
length.

clangd also flagged the C2X style structure initialization. IPMItool
is not using that compile flag. Changed the initialization of the
struct to meet pre-C2X compilation requirements.

Signed-off-by: Johnathan Mantey johnathanx.mantey@intel.com

clangd reports a buffer overrun issue. The sprintf() used to fill ipmi_devfs2 requires 17 bytes to store the null terminated string. The character buffer is only 16 bytes in length. clangd also flagged the C2X style structure initialization. IPMItool is not using that compile flag. Changed the initialization of the struct to meet pre-C2X compilation requirements. Signed-off-by: Johnathan Mantey <johnathanx.mantey@intel.com>
clangd reports a buffer overrun issue.
The sprintf() used to fill ipmi_devfs2 requires 17 bytes to store the
null terminated string. The character buffer is only 16 bytes in
length.
clangd also flagged the C2X style structure initialization. IPMItool
is not using that compile flag. Changed the initialization of the
struct to meet pre-C2X compilation requirements.
Signed-off-by: Johnathan Mantey <johnathanx.mantey@intel.com>
vmauery requested changes 2024年02月10日 00:37:40 +01:00
Dismissed
@ -95,3 +95,3 @@
char ipmi_dev[16];
char ipmi_devfs[16];
char ipmi_devfs2[16];
char ipmi_devfs2[17];
First-time contributor
Copy link

This does assume a small devnum. intf->devnum is a uint8_t, so it in theory could be 3 characters, which would necessitate 18 (17+NUL).

I don't know how deep the compiler is looking. The local devnum value is an integer, which would have an even larger range. But by code inspection, it is clear that the range is 0-255.

Alternatively, you could change the sprintf lines to snprintf.

This does assume a small devnum. intf->devnum is a uint8_t, so it in theory could be 3 characters, which would necessitate 18 (17+NUL). I don't know how deep the compiler is looking. The local devnum value is an integer, which would have an even larger range. But by code inspection, it is clear that the range is 0-255. Alternatively, you could change the sprintf lines to snprintf.
Author
Contributor
Copy link

Counting the chars in the string constant after the first " and up to the % returns 13 chars.
Add three more chars and that is 16. One more for the termination character sums to 17.
This is consistent with the clangd message saying 17 chars were being placed in a 16 char array.
How did you get to 18 chars?

Counting the chars in the string constant after the first " and up to the % returns 13 chars. Add three more chars and that is 16. One more for the termination character sums to 17. This is consistent with the clangd message saying 17 chars were being placed in a 16 char array. How did you get to 18 chars?
First-time contributor
Copy link

You are right. I can't count. :sheepish:

You are right. I can't count. :sheepish:
vmauery approved these changes 2024年02月12日 18:33:40 +01:00
Dismissed
AlexanderAmelkin left a comment
Copy link

Please update the commit log summary in accordance with the rules:
open: Eliminate buffer overrun

Please update the commit log summary in accordance with the rules: `open: Eliminate buffer overrun`
@ -171,3 +171,3 @@
ipmi_openipmi_send_cmd(struct ipmi_intf *intf, struct ipmi_rq *req)
{
struct ipmi_recv recv = {};
struct ipmi_recv recv;

I believe, it's not the only place where "C2X struct initialization" is used like that.
I personally prefer this method, and apparently it's been supported by GCC for a long time.
If you want to use clang and are worried about it's warning regarding this, then I would prefer if you added a compilation flag to allow for such struct initializations rather than falling back to memset().

Also, please, as said in "How to contribute", do not mix different changes in one commit. This change has nothing to do with buffer overflow.

I believe, it's not the only place where "C2X struct initialization" is used like that. I personally prefer this method, and apparently it's been supported by GCC for a long time. If you want to use `clang` and are worried about it's warning regarding this, then I would prefer if you added a compilation flag to allow for such struct initializations rather than falling back to `memset()`. Also, please, as said in "How to contribute", do not mix different changes in one commit. This change has nothing to do with buffer overflow.
clangd reports a buffer overrun issue.
The sprintf() used to fill ipmi_devfs2 requires 17 bytes to store the
null terminated string. The character buffer is only 16 bytes in
length.
clangd also flagged the C2X style structure initialization. IPMItool
is not using that compile flag. Changed the initialization of the
struct to meet pre-C2X compilation requirements.
Signed-off-by: Johnathan Mantey <johnathanx.mantey@intel.com>
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

Howitzer105mm changed title from (削除) Eliminate buffer overrun in open.c (削除ここまで) to open: Eliminate buffer overrun 2024年02月21日 22:16:24 +01:00
Sign in to join this conversation.
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
IPMITool/ipmitool!24
Reference in a new issue
IPMITool/ipmitool
No description provided.
Delete branch "Howitzer105mm/ipmitool:bugfix/23-clangd-buffer-overrun-in-open-plugin"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?