[MME][Memory] Enable UBSAN for debug builds and resolve unaligned memory access issues by talkhasib · Pull Request #5262 · magma/magma

talkhasib · 2021-03-02T21:29:41Z

Signed-off-by: Tariq Al-Khasib [email protected]

Summary

Enable UBSAN for debug builds (sanitize=undefined)
Once enabled, it uncovered many unaligned memory access problems.

ZMQ received message needs to be copied to avoid unaligned memory access.
Use memcpy() instead of casting memory addresses in few places
Potential overflow when performing left shifts on bytes

sample logs

intertask_interface.c:407:48: runtime error: member access within misaligned address 0x629000163206 for type 'struct MessageDef', which requires 8 byte alignment
mme_app_main.c:78:11: runtime error: member access within misaligned address 0x629000163206 for type 'struct MessageDef', which requires 8 byte alignment
nas_message.c:729:9: runtime error: load of misaligned address 0x60200003c991 for type 'uint32_t', which requires 4 byte alignment
nas_stream_eia1.c:194:31: runtime error: load of misaligned address 0x603000ec0ef5 for type 'uint32_t', which requires 4 byte alignment

Test Plan

fab integ_test

…ory access issues Signed-off-by: Tariq Al-Khasib <[email protected]>

codecov · 2021-03-02T21:48:53Z

Codecov Report

Merging #5262 (37ff194) into master (a94f0f6) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5262      +/-   ##
==========================================
- Coverage   63.61%   63.61%   -0.01%     
==========================================
  Files         301      301              
  Lines       20300    20300              
==========================================
- Hits        12914    12913       -1     
- Misses       5485     5486       +1     
  Partials     1901     1901

Impacted Files	Coverage Δ
...oud/go/services/state/indexer/reindex/reindexer.go	`66.21% <0.00%> (-0.68%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a94f0f6...37ff194. Read the comment docs.

ulaskozat · 2021-03-03T00:07:04Z

lte/gateway/c/oai/common/common_defs.h

-  size += sizeof(uint8_t) + sizeof(uint16_t)
+  do {                                                                         \
+    uint32_t n_value = htonl(value);                                           \
+    memcpy(buffer, (unsigned char*) &n_value, sizeof(uint32_t));               \


should not we just copy 3 bytes?

This macro is a bit odd too - because as far as I can see in code search, it always receives a uint32_t and then packs all four bytes here. So it's really not an ENCODE_U24 its an ENCODE_U32... At least for the call sites I am spotting.

I noticed the oddness of this and kept the logic intact for the purpose of this PR

It does that, but size is incremented by 3 bytes. So the last byte will be overwritten. For that reason I commented that we should make the copy for the 3 bytes as well to avoid any confusion and also avoid potential buffer overflow if this happens to be the last 3 bytes of space in the allocated buffer.

ulaskozat

LGTM

electronjoe

As a top level concern, my experience with our CMake configs has me worried that other libraries that may be handling these buffers may not have UBSAN turned on - and it's possible that we might be fixing the issue in some MAGMA code but not detecting it at root cause / in other areas of the codebase. CMake badly needs to be fixed up IMO. See questions in review!

electronjoe · 2021-03-03T09:34:43Z

lte/gateway/c/oai/common/async_system.c

-  assert(msg_frame);
-  MessageDef* received_message_p = (MessageDef*) zframe_data(msg_frame);
+  int rc                         = 0;
+  MessageDef* received_message_p = receive_msg(reader);


Do we understand why this buffer is unaligned? By fixing it here are we fixing it everywhere? Or is it possibly unaligned in a third party library etc? Wondering if this is hiding the root cause (maybe not, but asking the question).

ZMQ does not care about the data structure and treats it as a byte buffer. Thus, alignment is not an issue inside ZMQ.
When we cast back to the message structure at the receiving end. That caused the misalignment. Copying the data resolves this issue.

electronjoe · 2021-03-03T09:35:35Z

lte/gateway/c/oai/common/async_system.c


    case TERMINATE_MESSAGE: {
      itti_free_msg_content(received_message_p);
-      zframe_destroy(&msg_frame);


Here and elsewhere. Are we certain that zframe_destroy can be substituted with free() here? I think without a comment that's not clear to me. E.g. our Bstring library b_destroy actually frees two allocations - and if we moved to free() we would leak.

free does not replace zframe_destroy(). We still do a zframe_destroy()
inside the receive_msg function after we copy the message body. We free the message that receive_msg() created.
itti_free_msg_content() handels freeing second level data allocations inside messages. Thats why we need to call both.

electronjoe · 2021-03-03T09:37:10Z

lte/gateway/c/oai/common/common_defs.h


 #define DECODE_U16(bUFFER, vALUE, sIZE)                                        \
-  vALUE = ntohs(*(uint16_t*) (bUFFER));                                        \
+  memcpy((unsigned char*) &vALUE, bUFFER, sizeof(uint16_t));                   \


Same question as above - do we expect that these buffers might be unaligned? If so then no surprises here and great. In this particular fixup I could see it being less surprising - if this is say parsing a uint16_t that came after a type field that was a byte or whatever.

Buffers are mainly packed data (IEs) received over the wire. Thus, no guarantee on alignment

electronjoe · 2021-03-03T09:40:56Z

lte/gateway/c/oai/common/common_defs.h

-  size += sizeof(uint8_t) + sizeof(uint16_t)
+  do {                                                                         \
+    uint32_t n_value = htonl(value);                                           \
+    memcpy(buffer, (unsigned char*) &n_value, sizeof(uint32_t));               \


This macro is a bit odd too - because as far as I can see in code search, it always receives a uint32_t and then packs all four bytes here. So it's really not an ENCODE_U24 its an ENCODE_U32... At least for the call sites I am spotting.

electronjoe · 2021-03-03T09:42:18Z

lte/gateway/c/oai/lib/secu/nas_stream_eia1.c

+      sizeof(uint32_t) * ((stream_cipher->blength >> 5) + 1));
+
+  /* copy message to avoid memory alignment issues */
+  memcpy(message, stream_cipher->message, ceil(stream_cipher->blength / 8.0));


Again, wondering if alignment issue here was expected or not?

Same here, input is a stream of bytes that guarantees no alignment

…ory access issues (#5262) Signed-off-by: Tariq Al-Khasib <[email protected]>

…ory access issues (magma#5262) Signed-off-by: Tariq Al-Khasib <[email protected]>

[MME][Memory] Enable UBSAN for debug builds and resolve unaligned mem…

37ff194

…ory access issues Signed-off-by: Tariq Al-Khasib <[email protected]>

talkhasib requested review from lionelgo, ssanadhya and ulaskozat as code owners March 2, 2021 21:29

magmabot added the component: agw Access gateway-related issue label Mar 2, 2021

ssanadhya approved these changes Mar 2, 2021

View reviewed changes

ulaskozat reviewed Mar 3, 2021

View reviewed changes

ulaskozat approved these changes Mar 3, 2021

View reviewed changes

talkhasib mentioned this pull request Mar 3, 2021

MME crash unknown cause #4778

Closed

talkhasib merged commit 89d0f38 into magma:master Mar 3, 2021

talkhasib added apply-v1.3 Needs to be applied to v1.3 release branch as well apply-v1.4 Needs to be applied to v1.4 release branch as well labels Mar 3, 2021

electronjoe reviewed Mar 3, 2021

View reviewed changes

talkhasib deleted the memory branch March 4, 2021 20:50

talkhasib added a commit that referenced this pull request Mar 4, 2021

[MME][Memory] Enable UBSAN for debug builds and resolve unaligned mem…

1e9e3e0

…ory access issues (#5262) Signed-off-by: Tariq Al-Khasib <[email protected]>

talkhasib added the backported-v1.4 Has been backported to v1.4 release branch label Mar 4, 2021

talkhasib added a commit that referenced this pull request Mar 5, 2021

[MME][Memory] Enable UBSAN for debug builds and resolve unaligned mem…

e52725a

…ory access issues (#5262) Signed-off-by: Tariq Al-Khasib <[email protected]>

talkhasib added the backported-v1.3 Has been backported to v1.3 release branch label Mar 5, 2021

chandra-77 pushed a commit to chandra-77/magma that referenced this pull request Mar 30, 2021

[MME][Memory] Enable UBSAN for debug builds and resolve unaligned mem…

8a95995

…ory access issues (magma#5262) Signed-off-by: Tariq Al-Khasib <[email protected]>

Conversation

talkhasib commented Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

codecov bot commented Mar 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ulaskozat left a comment

Choose a reason for hiding this comment

Uh oh!

electronjoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

talkhasib commented Mar 2, 2021 •

edited

Loading

codecov bot commented Mar 2, 2021 •

edited

Loading