This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Harald Welte gerrit-no-reply at lists.osmocom.orgHarald Welte has submitted this change and it was merged. Change subject: control_if: Avoid heap-use-after-free in osmo_wqueue_bfd_cb ...................................................................... control_if: Avoid heap-use-after-free in osmo_wqueue_bfd_cb Imagine following scenario: 1- client connects to CTRL iface, a new conn is created with POLL_READ enabled. 2- A non-related event happens which triggers a TRAP to be sent. As a result, the wqueue for the conn has now enabled POLL_WRITE, and message will be sent next time we go through osmo_main_select(). 3- At the same time, we receive the GET cmd from the CTRL client, which means POLL_READ event will be also triggered next time we call osmo_main_select(). 4- osmo_main_select triggers osmo_wqueue_bfd_cb with both READ/WRITE flags set. 5- The read_cb of wqueue is executed first. The handler closes the CTRL conn for some reason, freeing the osmo_fd struct and returns. 6- osmo_qeueue_bfd_cb keeps using the already freed osmo_fd and calls write_cb. So in step 6 we get a heap-use-after-free catched by AddressSanitizer: [0;m20180424135406115 [1;32mDLCTRL[0;m <0018> control_if.c:506 accept()ed new CTRL connection from (r=10.42.42.1:53910<->l=10.42.42.7:4249) [0;m20180424135406116 [1;34mDLCTRL[0;m <0018> control_cmd.c:378 Command: GET bts.0.oml-connection-state [0;m20180424135406117 [1;34mDLINP[0;m <0013> bts_ipaccess_nanobts.c:417 Identified BTS 1/0/0 [0;m[1;36m20180424135406118 [1;34mDNM[0;m[1;36m <0005> abis_nm.c:1628 Get Attr (bts=0) [0;m[1;36m20180424135406118 [1;34mDNM[0;m[1;36m <0005> abis_nm.c:1628 Get Attr (bts=0) [0;m20180424135406118 [1;34mDCTRL[0;m <000e> osmo_bsc_ctrl.c:158 BTS connection (re)established, sending TRAP. [0;m20180424135406119 [1;32mDLCTRL[0;m <0018> control_if.c:173 close()d CTRL connection (r=10.42.42.1:53910<->l=10.42.42.7:4249) [0;m================================================================= ==12301==ERROR: AddressSanitizer: heap-use-after-free on address 0x611000003e04 at pc 0x7f23091c3a2f bp 0x7ffc0cb73ff0 sp 0x7ffc0cb73fe8 READ of size 4 at 0x611000003e04 thread T0 #0 0x7f23091c3a2e in osmo_wqueue_bfd_cb /home/osmocom-build/jenkins/workspace/osmo-gsm-tester_build-osmo-bsc/libosmocore/src/write_queue.c:65 #1 0x7f23091ad5d8 in osmo_fd_disp_fds /home/osmocom-build/jenkins/workspace/osmo-gsm-tester_build-osmo-bsc/libosmocore/src/select.c:216 #2 0x7f23091ad5d8 in osmo_select_main /home/osmocom-build/jenkins/workspace/osmo-gsm-tester_build-osmo-bsc/libosmocore/src/select.c:256 #3 0x56538bdb7a26 in main /home/osmocom-build/jenkins/workspace/osmo-gsm-tester_build-osmo-bsc/osmo-bsc/src/osmo-bsc/osmo_bsc_main.c:532 #4 0x7f23077532e0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202e0) #5 0x56538bdb8999 in _start (/home/jenkins/workspace/osmo-gsm-tester_run-prod/trial-896/inst/osmo-bsc/bin/osmo-bsc+0x259999) Fixes: OS#3206 Change-Id: I84d10caaadcfa6bd46ba8756ca89aa0badcfd2e3 --- M include/osmocom/core/write_queue.h M src/ctrl/control_if.c 2 files changed, 25 insertions(+), 22 deletions(-) Approvals: Harald Welte: Looks good to me, approved Jenkins Builder: Verified diff --git a/include/osmocom/core/write_queue.h b/include/osmocom/core/write_queue.h index 2303f87..071621d 100644 --- a/include/osmocom/core/write_queue.h +++ b/include/osmocom/core/write_queue.h @@ -42,11 +42,11 @@ /*! actual linked list implementing the queue */ struct llist_head msg_queue; - /*! call-back in case qeueue is readable */ + /*! call-back in case qeueue is readable. Return -EBADF if fd is freed inside cb. */ int (*read_cb)(struct osmo_fd *fd); - /*! call-back in case qeueue is writable */ + /*! call-back in case qeueue is writable. Return -EBADF if fd is freed inside cb. */ int (*write_cb)(struct osmo_fd *fd, struct msgb *msg); - /*! call-back in case qeueue has exceptions */ + /*! call-back in case qeueue has exceptions. Return -EBADF if fd is freed inside cb. */ int (*except_cb)(struct osmo_fd *fd); }; diff --git a/src/ctrl/control_if.c b/src/ctrl/control_if.c index cc613ee..0ba2512 100644 --- a/src/ctrl/control_if.c +++ b/src/ctrl/control_if.c @@ -327,7 +327,7 @@ static int handle_control_read(struct osmo_fd * bfd) { - int ret = -1; + int ret; struct osmo_wqueue *queue; struct ctrl_connection *ccon; struct msgb *msg = NULL; @@ -337,27 +337,28 @@ ccon = container_of(queue, struct ctrl_connection, write_queue); ret = ipa_msg_recv_buffered(bfd->fd, &msg, &ccon->pending_msg); - if (ret <= 0) { - if (ret == -EAGAIN) - /* received part of a message, it is stored in ccon->pending_msg and there's - * nothing left to do now. */ - return 0; + if (ret == 0) { /* msg was already discarded. */ - if (ret == 0) { - control_close_conn(ccon); - ret = -EIO; - } - else - LOGP(DLCTRL, LOGL_ERROR, "Failed to parse ip access message: %d\n", ret); - - return ret; + goto close_fd; + } else if (ret == -EAGAIN) { + /* received part of a message, it is stored in ccon->pending_msg and there's + * nothing left to do now. */ + return 0; + } else if (ret < 0) { + LOGP(DLCTRL, LOGL_ERROR, "Failed to parse ip access message: %d\n", ret); + return 0; } ret = ctrl_handle_msg(ctrl, ccon, msg); msgb_free(msg); if (ret) - control_close_conn(ccon); - return ret; + goto close_fd; + + return 0; + +close_fd: + control_close_conn(ccon); + return -EBADF; } int ctrl_handle_msg(struct ctrl_handle *ctrl, struct ctrl_connection *ccon, struct msgb *msg) @@ -435,12 +436,14 @@ ccon = container_of(queue, struct ctrl_connection, write_queue); rc = write(bfd->fd, msg->data, msg->len); - if (rc == 0) + if (rc == 0) { control_close_conn(ccon); - else if (rc != msg->len) + return -EBADF; + } + if (rc != msg->len) LOGP(DLCTRL, LOGL_ERROR, "Failed to write message to the CTRL connection.\n"); - return rc; + return 0; } /*! Allocate CTRL connection -- To view, visit https://gerrit.osmocom.org/8031 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: I84d10caaadcfa6bd46ba8756ca89aa0badcfd2e3 Gerrit-PatchSet: 1 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Pau Espin Pedrol <pespin at sysmocom.de> Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org> Gerrit-Reviewer: Jenkins Builder