Hey,
in patch22 Andreas pointed out that we might have unbalanced ref count in the location updating request path(s) and these patches try to fix some possible reliable issues, handle spoofing to a certain degree and recover from error (abnormal channel close).
include/openbsc/signal.h | 10 ++++++++++ src/chan_alloc.c | 4 +++- src/gsm_04_08.c | 32 ++++++++++++++++++++++++-------- 3 files changed, 37 insertions(+), 9 deletions(-)
Andreas Eversberg is suspecting that some of these messages are not balanced and we are seeing a leak here. The general location updating request is guarded by the "location updating request" object inside the lchan that will keep the channel open for at least five seconds to get all the information we need. --- src/gsm_04_08.c | 8 -------- 1 files changed, 0 insertions(+), 8 deletions(-)
diff --git a/src/gsm_04_08.c b/src/gsm_04_08.c index fd47d8b..a91ef7a 100644 --- a/src/gsm_04_08.c +++ b/src/gsm_04_08.c @@ -481,11 +481,6 @@ static int mm_rx_id_resp(struct msgb *msg) DEBUGP(DMM, "IDENTITY RESPONSE: mi_type=0x%02x MI(%s)\n", mi_type, mi_string);
- /* - * Rogue messages could trick us but so is life - */ - put_lchan(lchan); - switch (mi_type) { case GSM_MI_TYPE_IMSI: if (!lchan->subscr) @@ -573,7 +568,6 @@ static int mm_rx_loc_upd_req(struct msgb *msg) switch (mi_type) { case GSM_MI_TYPE_IMSI: /* we always want the IMEI, too */ - use_lchan(lchan); rc = mm_tx_identity_req(lchan, GSM_MI_TYPE_IMEI); lchan->loc_operation->waiting_for_imei = 1;
@@ -582,7 +576,6 @@ static int mm_rx_loc_upd_req(struct msgb *msg) break; case GSM_MI_TYPE_TMSI: /* we always want the IMEI, too */ - use_lchan(lchan); rc = mm_tx_identity_req(lchan, GSM_MI_TYPE_IMEI); lchan->loc_operation->waiting_for_imei = 1;
@@ -590,7 +583,6 @@ static int mm_rx_loc_upd_req(struct msgb *msg) subscr = subscr_get_by_tmsi(mi_string); if (!subscr) { /* send IDENTITY REQUEST message to get IMSI */ - use_lchan(lchan); rc = mm_tx_identity_req(lchan, GSM_MI_TYPE_IMSI); lchan->loc_operation->waiting_for_imsi = 1; }
Do not allow two location updating requests at the same time on the same lchan. Such an event is certainly spoofed and can confuse the internal logic of the application. Prevent that. --- src/gsm_04_08.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/src/gsm_04_08.c b/src/gsm_04_08.c index a91ef7a..18371b5 100644 --- a/src/gsm_04_08.c +++ b/src/gsm_04_08.c @@ -563,6 +563,12 @@ static int mm_rx_loc_upd_req(struct msgb *msg) DEBUGP(DMM, "LUPDREQ: mi_type=0x%02x MI(%s) type=%s\n", mi_type, mi_string, lupd_name(lu->type));
+ /* two location updating request on the same lchan... *spoof* */ + if (lchan->loc_operation) { + gsm0408_loc_upd_rej(lchan, GSM48_REJECT_PROTOCOL_ERROR); + return 0; + } + allocate_loc_updating_req(lchan);
switch (mi_type) {
The abnormal case is that lchan_free ist getting called due a RSL_MT_CHAN_REL_ACK in the RSL but the refcount of this channel is not zero. This means that some "logical operation" is still going on that needs to be cancelled. Instead of always queuing up all operations in the struct lchan use the signal framework to inform higher layers about this abnormal case.
In gsm_04_08.c a signal handler is installed and in the abnormal case the location updating request operation is freed. --- include/openbsc/signal.h | 10 ++++++++++ src/chan_alloc.c | 4 +++- src/gsm_04_08.c | 18 ++++++++++++++++++ 3 files changed, 31 insertions(+), 1 deletions(-)
diff --git a/include/openbsc/signal.h b/include/openbsc/signal.h index c2cf46a..ca16296 100644 --- a/include/openbsc/signal.h +++ b/include/openbsc/signal.h @@ -37,6 +37,7 @@ enum signal_subsystems { SS_SMS, SS_ABISIP, SS_NM, + SS_LCHAN, };
/* SS_PAGING signals */ @@ -55,6 +56,15 @@ enum signal_nm { S_NM_FAIL_REP, /* GSM 12.21 failure event report */ };
+/* SS_LCHAN signals */ +enum signal_lchan { + /* + * The lchan got freed with refcount != 0 and error + * recovery needs to happen right now. + */ + S_LCHAN_UNEXPECTED_RELEASE, +}; + typedef int signal_cbfn(unsigned int subsys, unsigned int signal, void *handler_data, void *signal_data);
diff --git a/src/chan_alloc.c b/src/chan_alloc.c index fa07273..77a4f57 100644 --- a/src/chan_alloc.c +++ b/src/chan_alloc.c @@ -31,6 +31,7 @@ #include <openbsc/abis_nm.h> #include <openbsc/abis_rsl.h> #include <openbsc/debug.h> +#include <openbsc/signal.h>
static void auto_release_channel(void *_lchan);
@@ -193,8 +194,9 @@ void lchan_free(struct gsm_lchan *lchan) lchan->subscr = 0; }
- /* We might kill an active channel... FIXME: call cancellations */ + /* We might kill an active channel... */ if (lchan->use_count != 0) { + dispatch_signal(SS_LCHAN, S_LCHAN_UNEXPECTED_RELEASE, lchan); lchan->use_count = 0; }
diff --git a/src/gsm_04_08.c b/src/gsm_04_08.c index 18371b5..3dfa780 100644 --- a/src/gsm_04_08.c +++ b/src/gsm_04_08.c @@ -182,6 +182,24 @@ static void allocate_loc_updating_req(struct gsm_lchan *lchan) memset(lchan->loc_operation, 0, sizeof(*lchan->loc_operation)); }
+static int gsm0408_handle_lchan_signal(unsigned int subsys, unsigned int signal, + void *handler_data, void *signal_data) +{ + if (subsys != SS_LCHAN || signal != S_LCHAN_UNEXPECTED_RELEASE) + return 0; + + /* give up on the loc_operation in case of error */ + struct gsm_lchan *lchan = (struct gsm_lchan *)handler_data; + release_loc_updating_req(lchan); + + return 0; +} + +static __attribute__((constructor)) void on_load_0408(void) +{ + register_signal_handler(SS_LCHAN, gsm0408_handle_lchan_signal, NULL); +} + static void to_bcd(u_int8_t *bcd, u_int16_t val) { bcd[2] = val % 10;
-
Holger Freyther