laforge has submitted this change. ( https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/28048 )
Change subject: msc: add test for OS#5532: crash from CM Serv Rej
......................................................................
msc: add test for OS#5532: crash from CM Serv Rej
Reproduce the assertion trigger crashing osmo-msc reported in OS#5532,
i.e. a CM Service Request that contains a mismatching Mobile Identity.
Causes osmo-msc to crash with an assertion, so run it last.
Fix of the crash: I6c735b79b67108bcaadada3f01c7046e262f939b
Related: OS#5532
Depends: I6c735b79b67108bcaadada3f01c7046e262f939b (osmo-msc)
Change-Id: I3f84d00f456aaee578787059d7010c25efcdcf56
---
M msc/MSC_Tests.ttcn
1 file changed, 28 insertions(+), 0 deletions(-)
Approvals:
Jenkins Builder: Verified
pespin: Looks good to me, but someone else must approve
fixeria: Looks good to me, approved
laforge: Looks good to me, approved
diff --git a/msc/MSC_Tests.ttcn b/msc/MSC_Tests.ttcn
index 2fad7b4..5bf199c 100644
--- a/msc/MSC_Tests.ttcn
+++ b/msc/MSC_Tests.ttcn
@@ -6801,6 +6801,32 @@
vc_conn2.done;
}
+/* Establish a conn with a valid Mobile Identity. Then send a CM Service Request containing a mismatching Mobile
+ * Identity on the same conn. Caused a crash, see OS#5532. */
+friend function f_tc_cm_serv_wrong_mi(charstring id, BSC_ConnHdlrPars pars) runs on BSC_ConnHdlr {
+ f_init_handler(pars);
+
+ /* Set up a fully identified conn */
+ f_perform_lu();
+ f_establish_fully();
+
+ /* CM Serv Req with mismatching Mobile Identity */
+ var MobileIdentityLV mi := valueof(ts_MI_IMSI_LV(f_gen_imsi(99999))); /* ensure it is different from below*/
+ BSSAP.send(ts_PDU_DTAP_MO(ts_CM_SERV_REQ(CM_TYPE_MO_SMS, mi)));
+ BSSAP.receive(tr_PDU_DTAP_MT(tr_CM_SERV_REJ));
+
+ /* Cancel the first CM Service from f_establish_fully() */
+ BSSAP.send(ts_BSSMAP_ClearRequest(0));
+
+ f_expect_clear();
+}
+testcase TC_cm_serv_wrong_mi() runs on MTC_CT {
+ var BSC_ConnHdlr vc_conn;
+ f_init();
+ vc_conn := f_start_handler(refers(f_tc_cm_serv_wrong_mi), 94);
+ vc_conn.done;
+}
+
control {
execute( TC_cr_before_reset() );
execute( TC_lu_imsi_noauth_tmsi() );
@@ -6963,6 +6989,8 @@
execute( TC_call_re_establishment() );
execute( TC_call_re_establishment_auth() );
execute( TC_call_re_establishment_ciph() );
+
+ execute( TC_cm_serv_wrong_mi() );
}
--
To view, visit https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/28048
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-ttcn3-hacks
Gerrit-Branch: master
Gerrit-Change-Id: I3f84d00f456aaee578787059d7010c25efcdcf56
Gerrit-Change-Number: 28048
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
Attention is currently required from: neels.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/28048 )
Change subject: msc: add test for OS#5532: crash from CM Serv Rej
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/28048
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-ttcn3-hacks
Gerrit-Branch: master
Gerrit-Change-Id: I3f84d00f456aaee578787059d7010c25efcdcf56
Gerrit-Change-Number: 28048
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Comment-Date: Fri, 06 May 2022 07:40:11 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: neels.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/28048 )
Change subject: msc: add test for OS#5532: crash from CM Serv Rej
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
File msc/MSC_Tests.ttcn:
https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/28048/comment/d829379c_c9f3…
PS1, Line 6815: BSSAP.send(ts_PDU_DTAP_MO(ts_CM_SERV_REQ(CM_TYPE_MO_SMS, mi)));
Ah, indeed. I was confused because {BTS,BSC}_Tests.f_est_dchan() does not send anything on the established channel, while BSC_ConnectionHandler.f_establish_fully() does send a CM Service Request. Nevermind.
> Is the comment above not clear enough?
If you said "Send *another* CM Serv Req ..." it would be cleaner, IMO.
Not critical, giving CR+2.
--
To view, visit https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/28048
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-ttcn3-hacks
Gerrit-Branch: master
Gerrit-Change-Id: I3f84d00f456aaee578787059d7010c25efcdcf56
Gerrit-Change-Number: 28048
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 05 May 2022 21:36:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: laforge, pespin.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libgtpnl/+/26990 )
Change subject: fix some cases of rc == 0 on error
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
thanks, i'm still uncertain about implications of changing the behavior,
given that a "workaround" is possible.
@pablo, could you +1 if you would like to see this merged?
--
To view, visit https://gerrit.osmocom.org/c/libgtpnl/+/26990
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libgtpnl
Gerrit-Branch: master
Gerrit-Change-Id: I22fd69709e023572c6c616a4184340a554456faf
Gerrit-Change-Number: 26990
Gerrit-PatchSet: 2
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pablo <pablo(a)gnumonks.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 05 May 2022 20:11:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: fixeria.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/28048 )
Change subject: msc: add test for OS#5532: crash from CM Serv Rej
......................................................................
Patch Set 1:
(1 comment)
File msc/MSC_Tests.ttcn:
https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/28048/comment/d8b05660_5861…
PS1, Line 6815: BSSAP.send(ts_PDU_DTAP_MO(ts_CM_SERV_REQ(CM_TYPE_MO_SMS, mi)));
> I am trying to understand the TC scenario. […]
the scenario is: a conn is already open (e.g. an earlier CM Service is ongoing).
Then rx a second CM Service, but its mobile identity is wrong.
Is the comment above not clear enough?
The first IMSI is xxxx00094, the wrong one is xxxx99999.
Could also be a TMSI, but code wise it's easier to create an IMSI that is always guaranteed to mismatch.
The LU is just a necessary prelude to allow the first valid CM Service Request to begin.
--
To view, visit https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/28048
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-ttcn3-hacks
Gerrit-Branch: master
Gerrit-Change-Id: I3f84d00f456aaee578787059d7010c25efcdcf56
Gerrit-Change-Number: 28048
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 05 May 2022 20:02:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
neels has submitted this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/28049 )
Change subject: fix crash on CM Serv Rej: fix use count mismatch
......................................................................
fix crash on CM Serv Rej: fix use count mismatch
With comments, clarify the code paths where a CM Service use count has
not yet been placed on the conn (just send CM Service Reject) and where
the use count is placed (decrement count on CM Service Reject).
Place the CM Service use count slightly earlier:
- it is then correctly present when checking the mobile identity in
cm_serv_reuse_conn(), avoiding the crash reported in OS#5532.
- there is only one place incrementing the use count instead of two.
Related: OS#5532
Change-Id: I6c735b79b67108bcaadada3f01c7046e262f939b
---
M src/libmsc/gsm_04_08.c
M src/libmsc/msc_a.c
M tests/msc_vlr/msc_vlr_test_reject_concurrency.err
3 files changed, 24 insertions(+), 7 deletions(-)
Approvals:
Jenkins Builder: Verified
fixeria: Looks good to me, but someone else must approve
pespin: Looks good to me, but someone else must approve
laforge: Looks good to me, approved
diff --git a/src/libmsc/gsm_04_08.c b/src/libmsc/gsm_04_08.c
index b5d46fd..9d3b5a2 100644
--- a/src/libmsc/gsm_04_08.c
+++ b/src/libmsc/gsm_04_08.c
@@ -246,7 +246,9 @@
return msc_a_tx_dtap_to_i(msc_a, msg);
}
-/* 9.2.6 CM service reject */
+/* 9.2.6 CM service reject.
+ * For an active and valid CM Service Request, instead use msc_vlr_tx_cm_serv_rej(), which also takes care of
+ * decrementing the use token for that service type. */
static int msc_gsm48_tx_mm_serv_rej(struct msc_a *msc_a,
enum gsm48_reject_value value)
{
@@ -701,7 +703,6 @@
accept_reuse:
LOG_MSC_A_CAT(msc_a, DMM, LOGL_DEBUG, "re-using already accepted connection\n");
- msc_a_get(msc_a, msc_a_cm_service_type_to_use(cm_serv_type));
msub_update_id(msc_a->c.msub);
return net->vlr->ops.tx_cm_serv_acc(msc_a, cm_serv_type);
}
@@ -729,6 +730,14 @@
struct osmo_mobile_identity mi;
int rc;
+ /* There are two ways to respond with a CM Service Reject:
+ * Directly and only send the CM Service Reject with msc_gsm48_tx_mm_serv_rej().
+ * Decrement the CM Service use count token and send the message with msc_vlr_tx_cm_serv_rej().
+ *
+ * Until we accept the CM Service Request message as such, there is no use count placed for the service type.
+ * So in here use msc_gsm48_tx_mm_serv_rej() to respond.
+ */
+
/* Make sure that both header and CM Service Request fit into the buffer */
if (msgb_l3len(msg) < sizeof(*gh) + sizeof(*req)) {
LOG_MSC_A(msc_a, LOGL_ERROR, "Rx CM SERVICE REQUEST: wrong message size (%u < %zu)\n",
@@ -791,13 +800,16 @@
if (!msc_a_cm_service_type_to_use(req->cm_service_type))
return msc_gsm48_tx_mm_serv_rej(msc_a, GSM48_REJECT_SRV_OPT_NOT_SUPPORTED);
+ /* At this point, the CM Service Request message is being accepted.
+ * Increment the matching use token, and from here on use msc_vlr_tx_cm_serv_rej() to respond in case of
+ * failure. */
+ msc_a_get(msc_a, msc_a_cm_service_type_to_use(req->cm_service_type));
+
if (msc_a_is_accepted(msc_a))
return cm_serv_reuse_conn(msc_a, &mi, req->cm_service_type);
osmo_signal_dispatch(SS_SUBSCR, S_SUBSCR_IDENTITY, &mi);
- msc_a_get(msc_a, msc_a_cm_service_type_to_use(req->cm_service_type));
-
is_utran = (msc_a->c.ran->type == OSMO_RAT_UTRAN_IU);
vlr_proc_acc_req(msc_a->c.fi,
MSC_A_EV_AUTHENTICATED, MSC_A_EV_CN_CLOSE, NULL,
@@ -1508,7 +1520,8 @@
return gsm48_tx_mm_info(msc_a);
}
-/* VLR asks us to transmit a CM Service Reject */
+/* VLR asks us to transmit a CM Service Reject.
+ * Decrement the CM Service type's use token and send the CM Service Reject message. */
static int msc_vlr_tx_cm_serv_rej(void *msc_conn_ref, enum osmo_cm_service_type cm_service_type,
enum gsm48_reject_value cause)
{
diff --git a/src/libmsc/msc_a.c b/src/libmsc/msc_a.c
index e2bf975..5761ed7 100644
--- a/src/libmsc/msc_a.c
+++ b/src/libmsc/msc_a.c
@@ -1792,6 +1792,10 @@
return msc_a_start_assignment(msc_a, cc_trans);
}
+/* Map CM Service type to use token.
+ * Given a CM Service type, return a matching token intended for osmo_use_count.
+ * For unknown service type, return NULL.
+ */
const char *msc_a_cm_service_type_to_use(enum osmo_cm_service_type cm_service_type)
{
switch (cm_service_type) {
diff --git a/tests/msc_vlr/msc_vlr_test_reject_concurrency.err b/tests/msc_vlr/msc_vlr_test_reject_concurrency.err
index 67b4819..25ccdfb 100644
--- a/tests/msc_vlr/msc_vlr_test_reject_concurrency.err
+++ b/tests/msc_vlr/msc_vlr_test_reject_concurrency.err
@@ -1032,8 +1032,8 @@
DBSSAP msc_a(IMSI-901700000004620:MSISDN-46071:GERAN-A:CM_SERVICE_REQ){MSC_A_ST_AUTHENTICATED}: RAN decode: DTAP
DRLL msc_a(IMSI-901700000004620:MSISDN-46071:GERAN-A:CM_SERVICE_REQ){MSC_A_ST_AUTHENTICATED}: Dispatching 04.08 message: MM GSM48_MT_MM_CM_SERV_REQ
DMM msc_a(IMSI-901700000004620:GERAN-A:CM_SERVICE_REQ){MSC_A_ST_AUTHENTICATED}: Rx CM SERVICE REQUEST cm_service_type=Short-Messaging-Service
-DMM msc_a(IMSI-901700000004620:GERAN-A:CM_SERVICE_REQ){MSC_A_ST_AUTHENTICATED}: re-using already accepted connection
DREF msc_a(IMSI-901700000004620:GERAN-A:CM_SERVICE_REQ){MSC_A_ST_AUTHENTICATED}: + cm_service_sms: now used by 3 (2*cm_service_sms,rx_from_ms)
+DMM msc_a(IMSI-901700000004620:GERAN-A:CM_SERVICE_REQ){MSC_A_ST_AUTHENTICATED}: re-using already accepted connection
DBSSAP msc_a(IMSI-901700000004620:MSISDN-46071:GERAN-A:CM_SERVICE_REQ){MSC_A_ST_AUTHENTICATED}: Sending DTAP: MM GSM48_MT_MM_CM_SERV_ACC
DMSC msc_a(IMSI-901700000004620:MSISDN-46071:GERAN-A:CM_SERVICE_REQ){MSC_A_ST_AUTHENTICATED}: RAN encode: DTAP on GERAN-A
- DTAP --GERAN-A--> MS: GSM48_MT_MM_CM_SERV_ACC: 0521
@@ -1842,8 +1842,8 @@
DBSSAP msc_a(IMSI-901700000004620:MSISDN-46071:GERAN-A:PAGING_RESP){MSC_A_ST_AUTHENTICATED}: RAN decode: DTAP
DRLL msc_a(IMSI-901700000004620:MSISDN-46071:GERAN-A:PAGING_RESP){MSC_A_ST_AUTHENTICATED}: Dispatching 04.08 message: MM GSM48_MT_MM_CM_SERV_REQ
DMM msc_a(IMSI-901700000004620:GERAN-A:CM_SERVICE_REQ){MSC_A_ST_AUTHENTICATED}: Rx CM SERVICE REQUEST cm_service_type=Short-Messaging-Service
-DMM msc_a(IMSI-901700000004620:GERAN-A:CM_SERVICE_REQ){MSC_A_ST_AUTHENTICATED}: re-using already accepted connection
DREF msc_a(IMSI-901700000004620:GERAN-A:CM_SERVICE_REQ){MSC_A_ST_AUTHENTICATED}: + cm_service_sms: now used by 3 (sms,rx_from_ms,cm_service_sms)
+DMM msc_a(IMSI-901700000004620:GERAN-A:CM_SERVICE_REQ){MSC_A_ST_AUTHENTICATED}: re-using already accepted connection
DBSSAP msc_a(IMSI-901700000004620:MSISDN-46071:GERAN-A:CM_SERVICE_REQ){MSC_A_ST_AUTHENTICATED}: Sending DTAP: MM GSM48_MT_MM_CM_SERV_ACC
DMSC msc_a(IMSI-901700000004620:MSISDN-46071:GERAN-A:CM_SERVICE_REQ){MSC_A_ST_AUTHENTICATED}: RAN encode: DTAP on GERAN-A
- DTAP --GERAN-A--> MS: GSM48_MT_MM_CM_SERV_ACC: 0521
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/28049
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: I6c735b79b67108bcaadada3f01c7046e262f939b
Gerrit-Change-Number: 28049
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
Attention is currently required from: laforge.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-remsim/+/28055 )
Change subject: bankd: Log more clearly if we fail to open a PC/SC reader
......................................................................
Patch Set 1: Code-Review+1
--
To view, visit https://gerrit.osmocom.org/c/osmo-remsim/+/28055
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-remsim
Gerrit-Branch: master
Gerrit-Change-Id: I424f585a8a37f21806898e59e350201119645a21
Gerrit-Change-Number: 28055
Gerrit-PatchSet: 1
Gerrit-Owner: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Thu, 05 May 2022 17:28:27 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment