Harald 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 <pes...@sysmocom.de>
Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
