From patchwork Thu Jun 2 01:02:16 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: gerrit-no-reply@lists.osmocom.org X-Patchwork-Id: 628922 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.osmocom.org (lists.osmocom.org [144.76.43.76]) by ozlabs.org (Postfix) with ESMTP id 3rKpq40Mf8z9t5n for ; Thu, 2 Jun 2016 11:02:24 +1000 (AEST) Received: from lists.osmocom.org (lists.osmocom.org [144.76.43.76]) by lists.osmocom.org (Postfix) with ESMTP id C357422AB9; Thu, 2 Jun 2016 01:02:22 +0000 (UTC) X-Original-To: openbsc@lists.osmocom.org Delivered-To: openbsc@lists.osmocom.org Received: from 127.0.1.12 (unknown [127.0.1.12]) by lists.osmocom.org (Postfix) with ESMTPA id D038922A52; Thu, 2 Jun 2016 01:02:16 +0000 (UTC) Date: Thu, 2 Jun 2016 01:02:16 +0000 From: Neels Hofmeyr Message-ID: X-Gerrit-MessageType: newchange Subject: [PATCH] openbsc[master]: gprs_gmm.c: Don't try to de-reference NULL mmctx X-Gerrit-Change-Id: I7908de65bec91599f7042549b832cbbd7ae5a9a8 X-Gerrit-ChangeURL: X-Gerrit-Commit: e98ba82d2b07c83592a323d41a9afc702ea50c79 MIME-Version: 1.0 Content-Disposition: inline User-Agent: Gerrit/2.12.2-31-gb331dbd-dirty X-BeenThere: openbsc@lists.osmocom.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: "Development of OpenBSC, OsmoBSC, OsmoNITB, OsmoCSCN" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: nhofmeyr@sysmocom.de Errors-To: openbsc-bounces@lists.osmocom.org Sender: "OpenBSC" Review at https://gerrit.osmocom.org/171 gprs_gmm.c: Don't try to de-reference NULL mmctx There was a comment in the code that certain GMM messages require a valid mmctx pointer. However, nothing actually checked if that pointer was in fact non-NULL. We plainly crashed if a MS would send us the wrong message in the wrong state. Original patch by Harald Welte, but it broke message validity checking, resulting in sgsn_test failure. This re-implements the NULL check in a different way, as explained by in-code comment. Change-Id: I7908de65bec91599f7042549b832cbbd7ae5a9a8 --- M openbsc/src/gprs/gprs_gmm.c 1 file changed, 32 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/71/171/1 diff --git a/openbsc/src/gprs/gprs_gmm.c b/openbsc/src/gprs/gprs_gmm.c index 1e10701..e788e3d 100644 --- a/openbsc/src/gprs/gprs_gmm.c +++ b/openbsc/src/gprs/gprs_gmm.c @@ -1339,6 +1339,15 @@ return rc; } + /* + * For a few messages, mmctx may be NULL. For most, we want to ensure a + * non-NULL mmctx. At the same time, we want to keep the message + * validity check intact, so that all message types appear in the + * switch statement and the default case thus means "unknown message". + * If we split the switch in two parts to check non-NULL halfway, the + * unknown-message check breaks, or we'd need to duplicate the switch + * cases in both parts. Just keep one large switch and add some gotos. + */ switch (gh->msg_type) { case GSM48_MT_GMM_RA_UPD_REQ: rc = gsm48_rx_gmm_ra_upd_req(mmctx, msg, llme); @@ -1348,20 +1357,30 @@ break; /* For all the following types mmctx can not be NULL */ case GSM48_MT_GMM_ID_RESP: + if (!mmctx) + goto null_mmctx; rc = gsm48_rx_gmm_id_resp(mmctx, msg); break; case GSM48_MT_GMM_STATUS: + if (!mmctx) + goto null_mmctx; rc = gsm48_rx_gmm_status(mmctx, msg); break; case GSM48_MT_GMM_DETACH_REQ: + if (!mmctx) + goto null_mmctx; rc = gsm48_rx_gmm_det_req(mmctx, msg); break; case GSM48_MT_GMM_DETACH_ACK: + if (!mmctx) + goto null_mmctx; LOGMMCTXP(LOGL_INFO, mmctx, "-> DETACH ACK\n"); mm_ctx_cleanup_free(mmctx, "GPRS DETACH ACK"); rc = 0; break; case GSM48_MT_GMM_ATTACH_COMPL: + if (!mmctx) + goto null_mmctx; /* only in case SGSN offered new P-TMSI */ LOGMMCTXP(LOGL_INFO, mmctx, "-> ATTACH COMPLETE\n"); mmctx_timer_stop(mmctx, 3350); @@ -1380,6 +1399,8 @@ osmo_signal_dispatch(SS_SGSN, S_SGSN_ATTACH, &sig_data); break; case GSM48_MT_GMM_RA_UPD_COMPL: + if (!mmctx) + goto null_mmctx; /* only in case SGSN offered new P-TMSI */ LOGMMCTXP(LOGL_INFO, mmctx, "-> ROUTING AREA UPDATE COMPLETE\n"); mmctx_timer_stop(mmctx, 3350); @@ -1398,6 +1419,8 @@ osmo_signal_dispatch(SS_SGSN, S_SGSN_UPDATE, &sig_data); break; case GSM48_MT_GMM_PTMSI_REALL_COMPL: + if (!mmctx) + goto null_mmctx; LOGMMCTXP(LOGL_INFO, mmctx, "-> PTMSI REALLLICATION COMPLETE\n"); mmctx_timer_stop(mmctx, 3350); mmctx->t3350_mode = GMM_T3350_MODE_NONE; @@ -1409,6 +1432,8 @@ rc = 0; break; case GSM48_MT_GMM_AUTH_CIPH_RESP: + if (!mmctx) + goto null_mmctx; rc = gsm48_rx_gmm_auth_ciph_resp(mmctx, msg); break; default: @@ -1419,6 +1444,13 @@ } return rc; + +null_mmctx: + LOGP(DMM, LOGL_ERROR, + "Received GSM 04.08 message type 0x%02x," + " but no MM context available\n", + gh->msg_type); + return -EINVAL; } static void mmctx_timer_cb(void *_mm)