Change in osmo-msc[master]: store classmark in vlr_subscr, not conn

2018-09-17 Thread Harald Welte
Harald Welte has submitted this change and it was merged. ( 
https://gerrit.osmocom.org/10985 )

Change subject: store classmark in vlr_subscr, not conn
..

store classmark in vlr_subscr, not conn

Store all Classmark information in the VLR.

So, we now always know the Classmark 1 (mandatory IE for LU). This is visible
in the msc_vlr_tests -- they no longer indicate "assuming A5/1 is supported"
because classmark 1 is missing, because we now know the Classmark 1.

Rationale:

During Location Updating, we receive Classmark 1; during CM Service Request and
Paging Response, we receive Classmark 2. So far we stored these only for the
duration of the conn, so as soon as a LU is complete, we would forget CM1.

In other words, for anything else than a LU Request, we had no Classmark 1
available at all.

During Ciphering Mode Command, we rely on Classmark 1 to determine whether A5/1
is supported. That is moot if we don't even have a Classmark 1 for any CM
Service Request or Paging Response initiated connections.

The only reason that A5/1 worked is that we assume A5/1 to work if Classmark 1
is missing. To add to the confusion, if a phone indicated that it did *not*
support A5/1 in the Classmark 1, according to spec we're supposed to not
service it at all. A code comment however says that we instead want to heed the
flag -- which so far was only present in a Location Updating initiated
connection. Now we can make this decision without assuming things.

This got my attention while hacking on sending a BSSMAP Classmark Request from
the MSC if it finds missing Classmark information, and was surprised to see it
it lacking CM1 to decide about A5/1.

Change-Id: I27081bf6e9e017923b2d02607f7ea06beddad82a
---
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/vlr.h
M src/libmsc/gsm_04_08.c
M src/libmsc/osmo_msc.c
M tests/msc_vlr/msc_vlr_test_gsm_ciph.err
M tests/msc_vlr/msc_vlr_test_rest.err
6 files changed, 75 insertions(+), 69 deletions(-)

Approvals:
  Jenkins Builder: Verified
  Vadim Yanitskiy: Looks good to me, but someone else must approve
  Harald Welte: Looks good to me, approved



diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 27f7fc5..ffe3afc 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -127,8 +127,6 @@
/* connected via 2G or 3G? */
enum ran_type via_ran;

-   struct gsm_classmark classmark;
-
uint16_t lac;
struct gsm_encr encr;

diff --git a/include/osmocom/msc/vlr.h b/include/osmocom/msc/vlr.h
index 386a548..d52713c 100644
--- a/include/osmocom/msc/vlr.h
+++ b/include/osmocom/msc/vlr.h
@@ -174,6 +174,8 @@
uint8_t lac;
enum ran_type attached_via_ran;
} cs;
+
+   struct gsm_classmark classmark;
 };

 enum vlr_ciph {
diff --git a/src/libmsc/gsm_04_08.c b/src/libmsc/gsm_04_08.c
index 19b0572..b942a03 100644
--- a/src/libmsc/gsm_04_08.c
+++ b/src/libmsc/gsm_04_08.c
@@ -98,14 +98,25 @@
return gsm48_conn_sendmsg(msg, conn, NULL);
 }

+static bool classmark1_is_r99(const struct gsm48_classmark1 *cm1)
+{
+   return cm1->rev_lev >= 2;
+}
+
+static bool classmark2_is_r99(const uint8_t *cm2, uint8_t cm2_len)
+{
+   uint8_t rev_lev;
+   if (!cm2_len)
+   return false;
+   rev_lev = (cm2[0] >> 5) & 0x3;
+   return rev_lev >= 2;
+}
+
 static bool classmark_is_r99(struct gsm_classmark *cm)
 {
-   int rev_lev = 0;
if (cm->classmark1_set)
-   rev_lev = cm->classmark1.rev_lev;
-   else if (cm->classmark2_len > 0)
-   rev_lev = (cm->classmark2[0] >> 5) & 0x3;
-   return rev_lev >= 2;
+   return classmark1_is_r99(>classmark1);
+   return classmark2_is_r99(cm->classmark2, cm->classmark2_len);
 }

 /* Determine if the given CLASSMARK (1/2/3) value permits a given A5/n cipher 
*/
@@ -345,9 +356,6 @@

msc_subscr_conn_update_id(conn, COMPLETE_LAYER3_LU, mi_string);

-   conn->classmark.classmark1 = lu->classmark1;
-   conn->classmark.classmark1_set = true;
-
DEBUGP(DMM, "LOCATION UPDATING REQUEST: MI(%s)=%s type=%s\n",
   gsm48_mi_type_name(mi_type), mi_string,
   get_value_string(lupd_names, lu->type));
@@ -402,7 +410,7 @@
_lai, _lai,
is_utran || 
conn->network->authentication_required,
is_utran || conn->network->a5_encryption_mask > 
0x01,
-   classmark_is_r99(>classmark),
+   classmark1_is_r99(>classmark1),
is_utran,
net->vlr->cfg.assign_tmsi);
if (!lu_fsm) {
@@ -421,6 +429,9 @@
return -EIO;
}

+   conn->vsub->classmark.classmark1 = lu->classmark1;
+   conn->vsub->classmark.classmark1_set = true;
+

Change in osmo-msc[master]: store classmark in vlr_subscr, not conn

2018-09-17 Thread Harald Welte
Harald Welte has posted comments on this change. ( 
https://gerrit.osmocom.org/10985 )

Change subject: store classmark in vlr_subscr, not conn
..


Patch Set 1: Code-Review+2


--
To view, visit https://gerrit.osmocom.org/10985
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27081bf6e9e017923b2d02607f7ea06beddad82a
Gerrit-Change-Number: 10985
Gerrit-PatchSet: 1
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-Comment-Date: Mon, 17 Sep 2018 10:33:10 +
Gerrit-HasComments: No
Gerrit-HasLabels: Yes


Change in osmo-msc[master]: store classmark in vlr_subscr, not conn

2018-09-16 Thread Vadim Yanitskiy
Vadim Yanitskiy has posted comments on this change. ( 
https://gerrit.osmocom.org/10985 )

Change subject: store classmark in vlr_subscr, not conn
..


Patch Set 1: Code-Review+1


--
To view, visit https://gerrit.osmocom.org/10985
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27081bf6e9e017923b2d02607f7ea06beddad82a
Gerrit-Change-Number: 10985
Gerrit-PatchSet: 1
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder (102)
Gerrit-Reviewer: Vadim Yanitskiy 
Gerrit-Comment-Date: Mon, 17 Sep 2018 05:38:08 +
Gerrit-HasComments: No
Gerrit-HasLabels: Yes


Change in osmo-msc[master]: store classmark in vlr_subscr, not conn

2018-09-16 Thread Neels Hofmeyr
Neels Hofmeyr has uploaded this change for review. ( 
https://gerrit.osmocom.org/10985


Change subject: store classmark in vlr_subscr, not conn
..

store classmark in vlr_subscr, not conn

Store all Classmark information in the VLR.

So, we now always know the Classmark 1 (mandatory IE for LU). This is visible
in the msc_vlr_tests -- they no longer indicate "assuming A5/1 is supported"
because classmark 1 is missing, because we now know the Classmark 1.

Rationale:

During Location Updating, we receive Classmark 1; during CM Service Request and
Paging Response, we receive Classmark 2. So far we stored these only for the
duration of the conn, so as soon as a LU is complete, we would forget CM1.

In other words, for anything else than a LU Request, we had no Classmark 1
available at all.

During Ciphering Mode Command, we rely on Classmark 1 to determine whether A5/1
is supported. That is moot if we don't even have a Classmark 1 for any CM
Service Request or Paging Response initiated connections.

The only reason that A5/1 worked is that we assume A5/1 to work if Classmark 1
is missing. To add to the confusion, if a phone indicated that it did *not*
support A5/1 in the Classmark 1, according to spec we're supposed to not
service it at all. A code comment however says that we instead want to heed the
flag -- which so far was only present in a Location Updating initiated
connection. Now we can make this decision without assuming things.

This got my attention while hacking on sending a BSSMAP Classmark Request from
the MSC if it finds missing Classmark information, and was surprised to see it
it lacking CM1 to decide about A5/1.

Change-Id: I27081bf6e9e017923b2d02607f7ea06beddad82a
---
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/vlr.h
M src/libmsc/gsm_04_08.c
M src/libmsc/osmo_msc.c
M tests/msc_vlr/msc_vlr_test_gsm_ciph.err
M tests/msc_vlr/msc_vlr_test_rest.err
6 files changed, 75 insertions(+), 69 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/85/10985/1

diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 27f7fc5..ffe3afc 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -127,8 +127,6 @@
/* connected via 2G or 3G? */
enum ran_type via_ran;

-   struct gsm_classmark classmark;
-
uint16_t lac;
struct gsm_encr encr;

diff --git a/include/osmocom/msc/vlr.h b/include/osmocom/msc/vlr.h
index 386a548..d52713c 100644
--- a/include/osmocom/msc/vlr.h
+++ b/include/osmocom/msc/vlr.h
@@ -174,6 +174,8 @@
uint8_t lac;
enum ran_type attached_via_ran;
} cs;
+
+   struct gsm_classmark classmark;
 };

 enum vlr_ciph {
diff --git a/src/libmsc/gsm_04_08.c b/src/libmsc/gsm_04_08.c
index 19b0572..b942a03 100644
--- a/src/libmsc/gsm_04_08.c
+++ b/src/libmsc/gsm_04_08.c
@@ -98,14 +98,25 @@
return gsm48_conn_sendmsg(msg, conn, NULL);
 }

+static bool classmark1_is_r99(const struct gsm48_classmark1 *cm1)
+{
+   return cm1->rev_lev >= 2;
+}
+
+static bool classmark2_is_r99(const uint8_t *cm2, uint8_t cm2_len)
+{
+   uint8_t rev_lev;
+   if (!cm2_len)
+   return false;
+   rev_lev = (cm2[0] >> 5) & 0x3;
+   return rev_lev >= 2;
+}
+
 static bool classmark_is_r99(struct gsm_classmark *cm)
 {
-   int rev_lev = 0;
if (cm->classmark1_set)
-   rev_lev = cm->classmark1.rev_lev;
-   else if (cm->classmark2_len > 0)
-   rev_lev = (cm->classmark2[0] >> 5) & 0x3;
-   return rev_lev >= 2;
+   return classmark1_is_r99(>classmark1);
+   return classmark2_is_r99(cm->classmark2, cm->classmark2_len);
 }

 /* Determine if the given CLASSMARK (1/2/3) value permits a given A5/n cipher 
*/
@@ -345,9 +356,6 @@

msc_subscr_conn_update_id(conn, COMPLETE_LAYER3_LU, mi_string);

-   conn->classmark.classmark1 = lu->classmark1;
-   conn->classmark.classmark1_set = true;
-
DEBUGP(DMM, "LOCATION UPDATING REQUEST: MI(%s)=%s type=%s\n",
   gsm48_mi_type_name(mi_type), mi_string,
   get_value_string(lupd_names, lu->type));
@@ -402,7 +410,7 @@
_lai, _lai,
is_utran || 
conn->network->authentication_required,
is_utran || conn->network->a5_encryption_mask > 
0x01,
-   classmark_is_r99(>classmark),
+   classmark1_is_r99(>classmark1),
is_utran,
net->vlr->cfg.assign_tmsi);
if (!lu_fsm) {
@@ -421,6 +429,9 @@
return -EIO;
}

+   conn->vsub->classmark.classmark1 = lu->classmark1;
+   conn->vsub->classmark.classmark1_set = true;
+
msc_subscr_conn_complete_layer_3(conn);
return 0;
 }
@@ -773,8 +784,6 @@