From 706f0e4421555ac59211d1af421e0171e8478158 Mon Sep 17 00:00:00 2001 From: Michael Cook Date: Sun, 28 Feb 2021 07:39:33 -0800 Subject: [PATCH] pdcp_fifo_flush_sdus: Fix message size calculation -fsanitize=address detected that we were trying to access bytes past the end of a block of malloc'ed memory. Specifically, in this code: ``` } else if (ENB_NAS_USE_TUN) { if( LOG_DEBUGFLAG(DEBUG_PDCP) ) log_dump(PDCP, pdcpData, sizeToWrite, LOG_DUMP_CHAR,"PDCP output to be sent to TUN interface: \n"); ret = write(nas_sock_fd[0], pdcpData, sizeToWrite); ``` -fsanitize=address said: ``` ==80==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61100004ffdc at pc 0x7f57c5f576a5 bp 0x7f57bb53c240 sp 0x7f57bb53b9e8 READ of size 108 at 0x61100004ffdc thread T7 0x61100004ffdc is located 0 bytes to the right of 220-byte region [0x61100004ff00,0x61100004ffdc) ``` So, the code was trying to access the first byte after a block of heap memory. sizeToWrite was calculated like this: ``` int sizeToWrite= sizeof (pdcp_data_ind_header_t) + pdcpHead->data_size; ``` There were a few other similar invocations of write() in the same function used the wrong size. That sizeToWrite calculation should be used only when the header is being sent, too, which happens in only one place in this function. With this commit, our tests pass and -fsanitize=address is happy. --- cmake_targets/CMakeLists.txt | 4 ++-- openair2/LAYER2/PDCP_v10.1.0/pdcp_fifo.c | 30 +++++++++++++----------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/cmake_targets/CMakeLists.txt b/cmake_targets/CMakeLists.txt index 95159b8c30..3bedc3391b 100644 --- a/cmake_targets/CMakeLists.txt +++ b/cmake_targets/CMakeLists.txt @@ -334,8 +334,8 @@ else (CUDA_FOUND) endif () # Uncomment these lines to enable the address sanitizer -#set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fsanitize=address") -#set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address") +set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fsanitize=address") +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address") # Uncomment these lines to enable gprof profiling #set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -pg -DUSING_GPROF") diff --git a/openair2/LAYER2/PDCP_v10.1.0/pdcp_fifo.c b/openair2/LAYER2/PDCP_v10.1.0/pdcp_fifo.c index 97a60e5086..7ab7d115f3 100644 --- a/openair2/LAYER2/PDCP_v10.1.0/pdcp_fifo.c +++ b/openair2/LAYER2/PDCP_v10.1.0/pdcp_fifo.c @@ -113,8 +113,6 @@ int pdcp_fifo_flush_sdus(const protocol_ctxt_t *const ctxt_pP) { * PDCP packet to pick the right socket below */ int rb_id = pdcpHead->rb_id; - int sizeToWrite= sizeof (pdcp_data_ind_header_t) + - pdcpHead->data_size; void * pdcpData=(void*)(pdcpHead+1); if (rb_id == 10) { //hardcoded for PC5-Signaling if( LOG_DEBUGFLAG(DEBUG_PDCP) ) { @@ -128,25 +126,29 @@ int pdcp_fifo_flush_sdus(const protocol_ctxt_t *const ctxt_pP) { } else if (UE_NAS_USE_TUN) { //ret = write(nas_sock_fd[pdcpHead->inst], &(sdu_p->data[sizeof(pdcp_data_ind_header_t)]),sizeToWrite ); if(rb_id == mbms_rab_id){ - ret = write(nas_sock_mbms_fd, pdcpData,sizeToWrite ); - LOG_I(PDCP,"[PDCP_FIFOS] ret %d TRIED TO PUSH MBMS DATA TO rb_id %d handle %d sizeToWrite %d\n", - ret,rb_id,nas_sock_fd[pdcpHead->inst],sizeToWrite); + ret = write(nas_sock_mbms_fd, pdcpData, pdcpHead->data_size); + LOG_I(PDCP,"[PDCP_FIFOS] ret %d TRIED TO PUSH MBMS DATA TO rb_id %d handle %d size %d\n", + ret,rb_id,nas_sock_fd[pdcpHead->inst], pdcpHead->data_size); } else { if( LOG_DEBUGFLAG(DEBUG_PDCP) ) - log_dump(PDCP, pdcpData, sizeToWrite, LOG_DUMP_CHAR,"PDCP output to be sent to TUN interface: \n"); - ret = write(nas_sock_fd[pdcpHead->inst], pdcpData,sizeToWrite ); - LOG_T(PDCP,"[UE PDCP_FIFOS] ret %d TRIED TO PUSH DATA TO rb_id %d handle %d sizeToWrite %d\n", - ret,rb_id,nas_sock_fd[pdcpHead->inst],sizeToWrite); + log_dump(PDCP, pdcpData, pdcpHead->data_size, LOG_DUMP_CHAR, + "PDCP output to be sent to TUN interface: \n"); + ret = write(nas_sock_fd[pdcpHead->inst], pdcpData, pdcpHead->data_size); + LOG_T(PDCP,"[UE PDCP_FIFOS] ret %d TRIED TO PUSH DATA TO rb_id %d handle %d size %d\n", + ret,rb_id,nas_sock_fd[pdcpHead->inst], pdcpHead->data_size); } } else if (ENB_NAS_USE_TUN) { if( LOG_DEBUGFLAG(DEBUG_PDCP) ) - log_dump(PDCP, pdcpData, sizeToWrite, LOG_DUMP_CHAR,"PDCP output to be sent to TUN interface: \n"); - ret = write(nas_sock_fd[0], pdcpData, sizeToWrite); - LOG_T(PDCP,"[NB PDCP_FIFOS] ret %d TRIED TO PUSH DATA TO rb_id %d handle %d sizeToWrite %d\n",ret,rb_id,nas_sock_fd[0],sizeToWrite); + log_dump(PDCP, pdcpData, pdcpHead->data_size, LOG_DUMP_CHAR, + "PDCP output to be sent to TUN interface: \n"); + ret = write(nas_sock_fd[0], pdcpData, pdcpHead->data_size); + LOG_T(PDCP,"[NB PDCP_FIFOS] ret %d TRIED TO PUSH DATA TO rb_id %d handle %d size %d\n", + ret, rb_id, nas_sock_fd[0], pdcpHead->data_size); } else if (PDCP_USE_NETLINK) { - memcpy(NLMSG_DATA(nas_nlh_tx), (uint8_t *) pdcpHead, sizeToWrite); + int sizeToWrite = sizeof(pdcp_data_ind_header_t) + pdcpHead->data_size; + memcpy(NLMSG_DATA(nas_nlh_tx), (uint8_t *) pdcpHead, sizeToWrite); nas_nlh_tx->nlmsg_len = sizeToWrite; ret = sendmsg(nas_sock_fd[0],&nas_msg_tx,0); } // PDCP_USE_NETLINK @@ -154,7 +156,7 @@ int pdcp_fifo_flush_sdus(const protocol_ctxt_t *const ctxt_pP) { AssertFatal(ret >= 0,"[PDCP_FIFOS] pdcp_fifo_flush_sdus (errno: %d %s), nas_sock_fd[0]: %d\n", errno, strerror(errno), nas_sock_fd[0]); if( LOG_DEBUGFLAG(DEBUG_PDCP) ) - log_dump(PDCP, pdcpData, min(sizeToWrite,30) , LOG_DUMP_CHAR, + log_dump(PDCP, pdcpData, min(pdcpHead->data_size, 30) , LOG_DUMP_CHAR, "Printing first bytes of PDCP SDU before removing it from the list: \n"); delNotifiedFIFO_elt (sdu_p); pdcp_nb_sdu_sent ++;