-I lanplus hpm check: fix garbage print #290
Conversation
Two changes included in this branch: 1. zero the extra data buffer that are not containing any IPMI message data. 2. copy only the HPM.1 property data to the pCtx response data buffer based on the data length rather than the allowed max property size. Resolves ipmitool#289 Signed-off-by: [email protected] <[email protected]>
| @@ -1572,9 +1572,11 @@ HpmfwupgGetComponentProperties(struct ipmi_intf *intf, | |||
| val2str(rsp->ccode, completion_code_vals)); | |||
| return HPMFWUPG_ERROR; | |||
| } | |||
|
|
|||
| memcpy(&pCtx->resp, rsp->data, rsp->data_len); | |||
| ((uint8_t*)&pCtx->resp)[rsp->data_len] = '\0'; | |||
There was a problem hiding this comment.
Generally rsp->data is not a string, you can't safely terminate it like this.
Instead you need a memset(&pCtx->resp, 0, sizeof(pCtx->resp)) before memcpy().
Also this memcpy(..., rsp->data_len) is a security vulnerability. You should never copy blindly any size that you got from outer world. Please use __min(rsp->data_len, sizeof(pCtx->resp)).
There was a problem hiding this comment.
Also, maybe this memset() should go into HpmfwupgSendCmd() instead? Or maybe even deeper, somewhere into the interfaces sendrecv() calls. I don't think any of the sendrecv() calls should return any garbage in the rsp.
Please investigate this further.
There was a problem hiding this comment.
Well, I agree consolidating memcpy() like this change has a potential security issue. I'll revert the changes of memcpy() and add the __min() to each memcpy() function call.
Ideally, memset() should be in sendrecv(). I agree that sendrecv() calls should not return any garbage. However, unfortunately, as you can see my changes for the src/plugins/lanplus.c, some of interface drivers do return garbage. I mentioned them in the Issue 289. Since I don't plan to change and test all these drivers, add a memset() here will help when other interface drivers are used.
Since I've been steered away to a totally different area, I'll be very slow on this.
| offset = 0; | ||
| payload_start = 0; | ||
| payload_size = extra_data_length; | ||
| memset(rsp->data + rsp->data_len, 0, IPMI_BUF_SIZE - rsp->data_len); |
There was a problem hiding this comment.
First of all, please use sizeof(rsp->data) instead of IPMI_BUF_SIZE. Never copy the size of any object from it's definition, always use sizeof(). Then whoever updates the definition later won't need to also update a million of other places.
Second, it looks like this change is not correct. It may need to go before memmove(). Or maybe it needs to go inside read_ipmi_response() instead or again into sendrecv() ?
There was a problem hiding this comment.
I agree sizeof() is better than IPMI_BUF_SIZE. The only reason I used IPMI_BUF_SIZE is to be consistent with other drivers (e.g. Line 655 of src/lan/lan.c, end of function ipmi_lan_poll_recv()). Please confirm you do want this to be sizeof() and I'll update the change. I don't plan to update other drivers which do not have issues.
memset() position is correct since it sets only the rest of the buffer to \0, which is actually doing the function as you mentioned in your comments for Line 1577 of lib/ipmi_hpmfwupg.c above. Moving it before memmove() will corrupt the received data before moving it to the beginning of the buffer. Please refer to the same code I just mentioned.
There was a problem hiding this comment.
I do want it to be sizeof(). There is plenty of less-than-optimal legacy code. Unless explicitly stated in the Coding Style, please don't blindly follow the pre-existing patterns.Thank you.
Two changes included in this branch:
data.
based on the data length rather than the allowed max property size.
Resolves #289
Signed-off-by: [email protected] [email protected]