pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/37870?usp=email )
Change subject: Fix double free during RAU with unexpected Old RAI ......................................................................
Fix double free during RAU with unexpected Old RAI
If an MS which had an MMCTX at the SGSN sent RAU update with an unexpected Old RA field, the RAU was rejected and LLME (LLC layer) unassigned (freed), because no MMCTX was found matching the wrong old RA. However, an MMCTX may actually exist pointing to that LLME, and hence when the LLME is freed, it stayed unnoticed with a dangling pointer to the freed LLME in ctx->gb.llme. Let's try to harder to avoid this kind of bugs which make osmo-sgsn crash.
Once we properly split the code into separate independent layers (LLC, MMCTX, etc.) each holding their own structs, this kind of bugs shouldn't happen anymore.
Related: OS#6441 Change-Id: I5a4328c6e945b85dd815215724feecadba59c435 --- M include/osmocom/sgsn/mmctx.h M src/sgsn/gprs_gmm.c M src/sgsn/mmctx.c 3 files changed, 31 insertions(+), 0 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-sgsn refs/changes/70/37870/1
diff --git a/include/osmocom/sgsn/mmctx.h b/include/osmocom/sgsn/mmctx.h index c19f599..5e4662f 100644 --- a/include/osmocom/sgsn/mmctx.h +++ b/include/osmocom/sgsn/mmctx.h @@ -257,6 +257,7 @@ struct sgsn_mm_ctx *sgsn_mm_ctx_by_ptmsi(uint32_t tmsi); struct sgsn_mm_ctx *sgsn_mm_ctx_by_imsi(const char *imsi); struct sgsn_mm_ctx *sgsn_mm_ctx_by_ue_ctx(const void *uectx); +struct sgsn_mm_ctx *sgsn_mm_ctx_by_llme(const struct gprs_llc_llme *llme);
/* look-up by matching TLLI and P-TMSI (think twice before using this) */ struct sgsn_mm_ctx *sgsn_mm_ctx_by_tlli_and_ptmsi(uint32_t tlli, diff --git a/src/sgsn/gprs_gmm.c b/src/sgsn/gprs_gmm.c index ab8b7ef..911d42d 100644 --- a/src/sgsn/gprs_gmm.c +++ b/src/sgsn/gprs_gmm.c @@ -1748,6 +1748,21 @@ * in the MS */ LOGGBP(llme, DMM, LOGL_NOTICE, "LLC XID RESET\n"); gprs_llgmm_reset_oldmsg(msg, GPRS_SAPI_GMM, llme); + + /* The RAU didn't come from expected TLLI+RAI, so it's for sure bad and should be rejected. + * In any case, before unassigning (freeing) the LLME during the REJECT below, make sure + * beforehand that if there's an mmctx relating to that llme it is also freed. + * Otherwise it would be kept pointining at a dangling freed llme object. + */ + mmctx = sgsn_mm_ctx_by_llme(llme); + if (mmctx) { + char old_ra_id_name[32]; + osmo_rai_name_buf(old_ra_id_name, sizeof(old_ra_id_name), &old_ra_id); + LOGMMCTXP(LOGL_NOTICE, mmctx, + "Rx RA Update Request with unexpected TLLI=%08x Old RA=%s (expected Old RA: %s)!\n", + msgb_tlli(msg), old_ra_id_name, osmo_rai_name(&mmctx->ra)); + /* mmctx will be released (and its llme un assigned) after REJECT below. */ + } } /* The MS has to perform GPRS attach */ /* Device is still IMSI attached for CS but initiate GPRS ATTACH, diff --git a/src/sgsn/mmctx.c b/src/sgsn/mmctx.c index 0e93092..459f6cf 100644 --- a/src/sgsn/mmctx.c +++ b/src/sgsn/mmctx.c @@ -105,6 +105,21 @@ return NULL; }
+ +/* look-up an SGSN MM context based on Gb LLME context (struct gprs_llc_llme)*/ +struct sgsn_mm_ctx *sgsn_mm_ctx_by_llme(const struct gprs_llc_llme *llme) +{ + struct sgsn_mm_ctx *ctx; + + llist_for_each_entry (ctx, &sgsn->mm_list, list) { + if (ctx->ran_type == MM_CTX_T_GERAN_Gb + && llme == ctx->gb.llme) + return ctx; + } + + return NULL; +} + /* look-up a SGSN MM context based on TLLI + RAI */ struct sgsn_mm_ctx *sgsn_mm_ctx_by_tlli(uint32_t tlli, const struct gprs_ra_id *raid)