lnls-dig / openMMC

Open source firmware for MMC controllers
GNU General Public License v3.0
39 stars 32 forks source link

Memory starvation due leakage #106

Closed augustofg closed 3 years ago

augustofg commented 3 years ago

Description

The MMC crashes after deferencing a NULL pointer returned by pvPortMalloc(). The crash happens in the memset call present here: https://github.com/lnls-dig/openMMC/blob/2a9a55319b8037d72e30636e95e6ec41856d0763/modules/ipmb.c#L205-L208

Calling xPortGetFreeHeapSize() when current_msg_rx is NULL reveals that only 48 bytes are available (not enough for allocating a ipmi_msg_cfg struct).

I saw a similar crash in ipmb.c:253 though I can't reproduce it anymore (I couldn't confirm it is also a memory starvation issue, but it seems likely): https://github.com/lnls-dig/openMMC/blob/2a9a55319b8037d72e30636e95e6ec41856d0763/modules/ipmb.c#L248-L254

How to reproduce

The problem sometimes manifests by itself over long periods, but a reliable way to trigger it is to push and pull the hotswap handle multiple times in a row.

augustofg commented 3 years ago

I found where the leak is occurring:

https://github.com/lnls-dig/openMMC/blob/2a9a55319b8037d72e30636e95e6ec41856d0763/modules/ipmb.c#L205-L230

If any of the conditions tested in lines 214 and 216 are false ipmb_notify_client(current_msg_rx); will never be called and current_msg_rx won't be freed. Using the following patch fix the issue according to my tests:

diff --git a/modules/ipmb.c b/modules/ipmb.c
index 40a56b9..1d6fad9 100644
--- a/modules/ipmb.c
+++ b/modules/ipmb.c
@@ -215,8 +215,12 @@ void IPMB_RXTask ( void *pvParameters )
                     /* Seq number checking is enough to match the messages */
                     if ( current_msg_rx->buffer.seq == last_sent_req->buffer.seq ) {
                         ipmb_notify_client ( current_msg_rx );
+                    } else {
+                        vPortFree(current_msg_rx);
                     }
                     /* If we received a response that doesn't match a previously sent request, just discard it */
+                } else {
+                    vPortFree(current_msg_rx);
                 }

             }else {