From patchwork Tue May 31 11:42:47 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: 628168 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 3rJs7B1TMYz9t41 for ; Tue, 31 May 2016 21:43:02 +1000 (AEST) Received: from lists.osmocom.org (lists.osmocom.org [144.76.43.76]) by lists.osmocom.org (Postfix) with ESMTP id 3234A22829; Tue, 31 May 2016 11:43:00 +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 610A42278A; Tue, 31 May 2016 11:42:47 +0000 (UTC) Date: Tue, 31 May 2016 11:42:47 +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: I10e6fee84abf05179f9e70981cdd295c57a58391 X-Gerrit-ChangeURL: X-Gerrit-Commit: 08415be57e34e2060778c7e1be1c6c5546b79251 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/152 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: I10e6fee84abf05179f9e70981cdd295c57a58391 --- M openbsc/src/gprs/gprs_gmm.c 1 file changed, 27 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/52/152/1 diff --git a/openbsc/src/gprs/gprs_gmm.c b/openbsc/src/gprs/gprs_gmm.c index 1e10701..44b4508 100644 --- a/openbsc/src/gprs/gprs_gmm.c +++ b/openbsc/src/gprs/gprs_gmm.c @@ -1339,6 +1339,23 @@ return rc; } + /* For a few messages, mmctx may be NULL. For all others, 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. Instead, just have this macro to + * avoid too much code dup: */ +#define CHECK_MMCTX \ + if (!mmctx) { \ + LOGP(DMM, LOGL_ERROR, \ + "Received GSM 04.08 message type 0x%02x," \ + " but no MM context available\n", \ + gh->msg_type); \ + return -EINVAL; \ + } + switch (gh->msg_type) { case GSM48_MT_GMM_RA_UPD_REQ: rc = gsm48_rx_gmm_ra_upd_req(mmctx, msg, llme); @@ -1348,20 +1365,25 @@ break; /* For all the following types mmctx can not be NULL */ case GSM48_MT_GMM_ID_RESP: + CHECK_MMCTX rc = gsm48_rx_gmm_id_resp(mmctx, msg); break; case GSM48_MT_GMM_STATUS: + CHECK_MMCTX rc = gsm48_rx_gmm_status(mmctx, msg); break; case GSM48_MT_GMM_DETACH_REQ: + CHECK_MMCTX rc = gsm48_rx_gmm_det_req(mmctx, msg); break; case GSM48_MT_GMM_DETACH_ACK: + CHECK_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: + CHECK_MMCTX /* only in case SGSN offered new P-TMSI */ LOGMMCTXP(LOGL_INFO, mmctx, "-> ATTACH COMPLETE\n"); mmctx_timer_stop(mmctx, 3350); @@ -1380,6 +1402,7 @@ osmo_signal_dispatch(SS_SGSN, S_SGSN_ATTACH, &sig_data); break; case GSM48_MT_GMM_RA_UPD_COMPL: + CHECK_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 +1421,7 @@ osmo_signal_dispatch(SS_SGSN, S_SGSN_UPDATE, &sig_data); break; case GSM48_MT_GMM_PTMSI_REALL_COMPL: + CHECK_MMCTX LOGMMCTXP(LOGL_INFO, mmctx, "-> PTMSI REALLLICATION COMPLETE\n"); mmctx_timer_stop(mmctx, 3350); mmctx->t3350_mode = GMM_T3350_MODE_NONE; @@ -1409,6 +1433,7 @@ rc = 0; break; case GSM48_MT_GMM_AUTH_CIPH_RESP: + CHECK_MMCTX rc = gsm48_rx_gmm_auth_ciph_resp(mmctx, msg); break; default: @@ -1418,6 +1443,8 @@ break; } +#undef CHECK_MMCTX + return rc; }