From b0162077da4d2b60c9006173c11bfab5a0369164 Mon Sep 17 00:00:00 2001 From: Stefan Sperling Date: Tue, 22 May 2018 18:19:00 +0200 Subject: fix double-free/use-after-free of pointers in struct e1inp_line Ensure that pointers in cloned e1inp_lines point to valid memory. Some members of struct e1inp_line can simply be deep-copied. Use talloc reference counting for pointers to objects which may be shared between clones (driver-private state and counters). Prevents double-free bugs, e.g. when multiple links referring to the same line are closed. Also, do not forget to unlink struct e1inp_line's counter group from the counter list. Fixes use-after-free in rate_ctr_timer_cb() during osmo-bts shutdown. Change-Id: I9f4724b4a5a064801591e9acf4f2fd1db006d082 Related: OS#3011 Related: OS#3137 --- src/e1_input.c | 41 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/src/e1_input.c b/src/e1_input.c index 29ba440..11949a1 100644 --- a/src/e1_input.c +++ b/src/e1_input.c @@ -394,6 +394,25 @@ e1inp_line_clone(void *ctx, struct e1inp_line *line) return NULL; memcpy(clone, line, sizeof(struct e1inp_line)); + + if (line->name) { + clone->name = talloc_strdup(clone, line->name); + OSMO_ASSERT(clone->name); + } + if (line->sock_path) { + clone->sock_path = talloc_strdup(clone, line->sock_path); + OSMO_ASSERT(clone->sock_path); + } + + /* + * Rate counters and driver data are shared between clones. These are pointers + * to dynamic memory so we use reference counting to avoid a double-free (see OS#3137). + */ + OSMO_ASSERT(line->rate_ctr); + clone->rate_ctr = talloc_reference(clone, line->rate_ctr); + if (line->driver_data) + clone->driver_data = talloc_reference(clone, line->driver_data); + clone->refcnt = 1; return clone; } @@ -406,8 +425,28 @@ void e1inp_line_get(struct e1inp_line *line) void e1inp_line_put(struct e1inp_line *line) { line->refcnt--; - if (line->refcnt == 0) + if (line->refcnt == 0) { + /* Remove our counter group from libosmocore's global counter + * list if we are freeing the last remaining talloc context. + * Otherwise we get a use-after-free when libosmocore's timer + * ticks again and attempts to update these counters (OS#3011). + * + * Note that talloc internally counts "secondary" references + * _in addition to_ the initial allocation context, so yes, + * we must check for *zero* remaining secondary contexts here. */ + if (talloc_reference_count(line->rate_ctr) == 0) { + rate_ctr_group_free(line->rate_ctr); + } else { + /* We are not freeing the last talloc context. + * Instead of calling talloc_free(), unlink this 'line' pointer + * which serves as one of several talloc contexts for the rate + * counters and driver private state. */ + talloc_unlink(line, line->rate_ctr); + if (line->driver_data) + talloc_unlink(line, line->driver_data); + } talloc_free(line); + } } void -- cgit v1.2.3