Change in libosmo-abis[master]: fix double-free/use-after-free of pointers in struct e1inp_line

[Stefan Sperling]

Stefan Sperling has submitted this change and it was merged. ( https://gerrit.osmocom.org/9252 )

Change subject: fix double-free/use-after-free of pointers in struct e1inp_line
......................................................................

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
---
M src/e1_input.c
1 file changed, 40 insertions(+), 1 deletion(-)

Approvals:
  Jenkins Builder: Verified
  Pau Espin Pedrol: Looks good to me, approved



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 @@
 	        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_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

-- 
To view, visit https://gerrit.osmocom.org/9252
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmo-abis
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9f4724b4a5a064801591e9acf4f2fd1db006d082
Gerrit-Change-Number: 9252
Gerrit-PatchSet: 3
Gerrit-Owner: Stefan Sperling <ssperling at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: Stefan Sperling <ssperling at sysmocom.de>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180524/b404ad92/attachment.htm>