laforge has submitted this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/38579?usp=email )
Change subject: Check for protocol extension bit in message type at a central location ......................................................................
Check for protocol extension bit in message type at a central location
Bits 8 and 7 of message type can be used as sequence number or have specific default values. The added check will verify the bits according to the prococol descriptor and Release version.
The Rules for these bits are described in GSM TS 124.007, clause 11.2.3.2.1 for Release <= 98 and 11.2.3.2.2 for Release >= 99.
There is also no need to clear bit 7 in message type for these protocols, as it is done already in gsm48_hdr_msg_type().
Some unit tests fail without fixes that are included in this patch.
Change-Id: Iae41bc6a2e9fd85583509b6c6154dd5a935fb5df --- M src/libmsc/gsm_04_08_cc.c M src/libmsc/msc_a.c M src/libmsc/msc_vgcs.c M tests/msc_vlr/msc_vlr_test_authen_reuse.c M tests/msc_vlr/msc_vlr_test_call.c M tests/msc_vlr/msc_vlr_test_gsm_ciph.c M tests/msc_vlr/msc_vlr_test_umts_authen.c M tests/msc_vlr/msc_vlr_tests.c M tests/msc_vlr/msc_vlr_tests.h 9 files changed, 64 insertions(+), 10 deletions(-)
Approvals: pespin: Looks good to me, approved laforge: Looks good to me, but someone else must approve Jenkins Builder: Verified
diff --git a/src/libmsc/gsm_04_08_cc.c b/src/libmsc/gsm_04_08_cc.c index 46ab176..11bebb0 100644 --- a/src/libmsc/gsm_04_08_cc.c +++ b/src/libmsc/gsm_04_08_cc.c @@ -2681,6 +2681,7 @@ #define DATASLLEN \ (sizeof(datastatelist) / sizeof(struct datastate))
+ int gsm0408_rcv_cc(struct msc_a *msc_a, struct msgb *msg) { struct gsm48_hdr *gh = msgb_l3(msg); @@ -2691,11 +2692,6 @@ struct gsm_network *net = msc_a_net(msc_a); int i, rc = 0;
- if (msg_type & 0x80) { - LOG_TRANS(trans, LOGL_DEBUG, "MSG 0x%2x not defined for PD error\n", msg_type); - return -EINVAL; - } - if (!vsub) { LOG_TRANS(trans, LOGL_ERROR, "Invalid conn: no subscriber\n"); /* Decrement use counter that has been incremented by CM Service Request (CC). diff --git a/src/libmsc/msc_a.c b/src/libmsc/msc_a.c index c682004..631e711 100644 --- a/src/libmsc/msc_a.c +++ b/src/libmsc/msc_a.c @@ -1455,6 +1455,45 @@ return silent_call_rx(conn, msg); #endif
+ /* Check for correct Bit 8. + * In R99 or above, it is used as sequence number for MM, CC and SS protocol. + * In any other case it must be set to default (0). */ + if (!is_r99 || (pdisc != GSM48_PDISC_MM && pdisc != GSM48_PDISC_CC && pdisc != GSM48_PDISC_NC_SS)) { + if ((gh->msg_type & 0x80)) { + LOG_MSC_A_CAT(msc_a, DRLL, LOGL_NOTICE, "MSG 0x%2x not defined for PD (bit 8 is not 0)\n", + gh->msg_type); + return -EINVAL; + } + } + + /* Check for correct Bit 7. + * For MM, CC, SS, GCC, BCC and LCS, it is used as sequence number. + * For RR it is part of the message type. + * For any other case it must be set to default (1 for SM, 0 for others). */ + switch (pdisc) { + case GSM48_PDISC_CC: + case GSM48_PDISC_MM: + case GSM48_PDISC_NC_SS: + case GSM48_PDISC_GROUP_CC: + case GSM48_PDISC_BCAST_CC: + case GSM48_PDISC_LOC: + case GSM48_PDISC_RR: + break; + case GSM48_PDISC_SM_GPRS: + if (!(gh->msg_type & 0x40)) { + LOG_MSC_A_CAT(msc_a, DRLL, LOGL_NOTICE, "SM MSG 0x%2x not defined for PD (bit 7 is not 1)\n", + gh->msg_type); + return -EINVAL; + } + break; + default: + if ((gh->msg_type & 0x40)) { + LOG_MSC_A_CAT(msc_a, DRLL, LOGL_NOTICE, "MSG 0x%2x not defined for PD (bit 7 is not 0)\n", + gh->msg_type); + return -EINVAL; + } + } + switch (pdisc) { case GSM48_PDISC_GROUP_CC: case GSM48_PDISC_BCAST_CC: diff --git a/src/libmsc/msc_vgcs.c b/src/libmsc/msc_vgcs.c index 528d282..fdb5356 100644 --- a/src/libmsc/msc_vgcs.c +++ b/src/libmsc/msc_vgcs.c @@ -1468,9 +1468,6 @@ uint8_t da, ua, comm, oi; int rc = 0;
- /* Remove sequence number (bit 7) from message type. */ - msg_type &= 0xbf; - /* Parse messages. */ switch (msg_type) { case OSMO_GSM44068_MSGT_SETUP: diff --git a/tests/msc_vlr/msc_vlr_test_authen_reuse.c b/tests/msc_vlr/msc_vlr_test_authen_reuse.c index 04f7da8..181c073 100644 --- a/tests/msc_vlr/msc_vlr_test_authen_reuse.c +++ b/tests/msc_vlr/msc_vlr_test_authen_reuse.c @@ -42,6 +42,7 @@ rx_from_ran = via_ran;
btw("Location Update request causes a GSUP Send Auth Info request to HLR"); + release_99 = true; lu_result_sent = RES_NONE; gsup_expect_tx("080108" "09710000000156f0" CN_DOMAIN VLR_TO_HLR); ms_sends_msg("0508" /* MM LU */ @@ -257,6 +258,7 @@ gsup_expect_tx("0c010809710000000156f0" CN_DOMAIN VLR_TO_HLR); ms_sends_msg("050130" "089910070000106005" /* IMSI */); + release_99 = false; ASSERT_RELEASE_CLEAR(via_ran); ran_sends_clear_complete(via_ran);
diff --git a/tests/msc_vlr/msc_vlr_test_call.c b/tests/msc_vlr/msc_vlr_test_call.c index 376a8c7..0ddba96 100644 --- a/tests/msc_vlr/msc_vlr_test_call.c +++ b/tests/msc_vlr/msc_vlr_test_call.c @@ -53,6 +53,7 @@ rx_from_ran = OSMO_RAT_UTRAN_IU;
btw("Location Update request causes a GSUP Send Auth Info request to HLR"); + release_99 = true; lu_result_sent = RES_NONE; gsup_expect_tx("080108" "09710000000156f0" CN_DOMAIN VLR_TO_HLR); ms_sends_msg("0508" /* MM LU */ @@ -171,6 +172,7 @@ net->vlr->cfg.assign_tmsi = false;
btw("Location Update request causes a GSUP LU request to HLR"); + release_99 = true; lu_result_sent = RES_NONE; gsup_expect_tx("04010809710000000156f0280102" VLR_TO_HLR); ms_sends_msg("0508" /* MM LU */ @@ -319,6 +321,7 @@ EXPECT_CONN_COUNT(0); clear_vlr(); comment_end(); + release_99 = false; }
static void test_call_mt() @@ -455,6 +458,7 @@ EXPECT_CONN_COUNT(0); clear_vlr(); comment_end(); + release_99 = false; }
static void test_call_mt2() @@ -580,6 +584,7 @@
clear_vlr(); comment_end(); + release_99 = false; }
static void test_call_mo_to_unknown() @@ -678,6 +683,7 @@ EXPECT_CONN_COUNT(0); clear_vlr(); comment_end(); + release_99 = false; }
static void test_call_mo_to_unknown_timeout() @@ -777,6 +783,7 @@ EXPECT_CONN_COUNT(0); clear_vlr(); comment_end(); + release_99 = false; }
#define LIST_END 0xffff @@ -1630,6 +1637,7 @@ EXPECT_CONN_COUNT(0); clear_vlr(); comment_end(); + release_99 = false; }
msc_vlr_test_func_t msc_vlr_tests[] = { diff --git a/tests/msc_vlr/msc_vlr_test_gsm_ciph.c b/tests/msc_vlr/msc_vlr_test_gsm_ciph.c index 4bf786f..0c9883d 100644 --- a/tests/msc_vlr/msc_vlr_test_gsm_ciph.c +++ b/tests/msc_vlr/msc_vlr_test_gsm_ciph.c @@ -848,6 +848,7 @@ rx_from_ran = OSMO_RAT_GERAN_A;
btw("Location Update request causes a GSUP Send Auth Info request to HLR"); + release_99 = true; lu_result_sent = RES_NONE; gsup_expect_tx("080108" "09710000000156f0" CN_DOMAIN VLR_TO_HLR); ms_sends_msg("0508" /* MM LU */ @@ -1054,6 +1055,7 @@ ms_sends_msg("050130" "089910070000106005" /* IMSI */); VERBOSE_ASSERT(bssap_clear_sent, == true, "%d"); + release_99 = false;
ran_sends_clear_complete(); EXPECT_CONN_COUNT(0); diff --git a/tests/msc_vlr/msc_vlr_test_umts_authen.c b/tests/msc_vlr/msc_vlr_test_umts_authen.c index 37aa96a..168173c 100644 --- a/tests/msc_vlr/msc_vlr_test_umts_authen.c +++ b/tests/msc_vlr/msc_vlr_test_umts_authen.c @@ -23,6 +23,8 @@
#include "msc_vlr_tests.h"
+ + static void _test_umts_authen(enum osmo_rat_type via_ran) { struct vlr_subscr *vsub; @@ -57,6 +59,7 @@ rx_from_ran = via_ran;
btw("Location Update request causes a GSUP Send Auth Info request to HLR"); + release_99 = true; lu_result_sent = RES_NONE; gsup_expect_tx("080108" "09710000000156f0" CN_DOMAIN VLR_TO_HLR); ms_sends_msg("0508" /* MM LU */ @@ -338,6 +341,7 @@ gsup_expect_tx("0c010809710000000156f0" CN_DOMAIN VLR_TO_HLR); ms_sends_msg("050130" "089910070000106005" /* IMSI */); + release_99 = false; ASSERT_RELEASE_CLEAR(via_ran); ran_sends_clear_complete(via_ran);
@@ -390,6 +394,7 @@ rx_from_ran = via_ran;
btw("Location Update request causes a GSUP Send Auth Info request to HLR"); + release_99 = true; lu_result_sent = RES_NONE; gsup_expect_tx("080108" "09710000000156f0" CN_DOMAIN VLR_TO_HLR); ms_sends_msg("0508" /* MM LU */ @@ -598,6 +603,7 @@ btw("MS sends TMSI Realloc Complete"); expect_release_clear(via_ran); ms_sends_msg("055b"); + release_99 = false; ASSERT_RELEASE_CLEAR(via_ran); ran_sends_clear_complete(via_ran);
diff --git a/tests/msc_vlr/msc_vlr_tests.c b/tests/msc_vlr/msc_vlr_tests.c index 376b2a4..a2f3c85 100644 --- a/tests/msc_vlr/msc_vlr_tests.c +++ b/tests/msc_vlr/msc_vlr_tests.c @@ -90,6 +90,8 @@ bool expecting_crcx[2] = {}; bool got_crcx[2] = {};
+bool release_99 = false; + extern int ran_dec_dtap_undup_pdisc_ctr_bin(uint8_t pdisc);
/* static state variables for the L3 send sequence numbers */ @@ -105,10 +107,10 @@
if (bin >= 0 && bin < ARRAY_SIZE(n_sd)) { /* patch in n_sd into the msg_type octet */ - *msg_type_oct = (*msg_type_oct & 0x3f) | ((n_sd[bin] & 0x3) << 6); + *msg_type_oct = (*msg_type_oct & 0x3f) | ((n_sd[bin] & (release_99 ? 0x3 : 0x1)) << 6); //fprintf(stderr, "pdisc=0x%02x bin=%d, patched n_sd=%u\n\n", pdisc, bin, n_sd[bin] & 3); /* increment N(SD) */ - n_sd[bin] = (n_sd[bin] + 1) % 4; + n_sd[bin] = (n_sd[bin] + 1) % (release_99 ? 4 : 2); } else { //fprintf(stderr, "pdisc=0x%02x NO SEQ\n\n", pdisc); } diff --git a/tests/msc_vlr/msc_vlr_tests.h b/tests/msc_vlr/msc_vlr_tests.h index f095a7f..2938d96 100644 --- a/tests/msc_vlr/msc_vlr_tests.h +++ b/tests/msc_vlr/msc_vlr_tests.h @@ -120,6 +120,8 @@ extern uint32_t cc_to_mncc_tx_got_callref; extern char cc_to_mncc_tx_last_sdp[1024];
+extern bool release_99; + extern struct gsm_mncc *on_call_release_mncc_sends_to_cc_data;
static inline void expect_iu_release()