Change in ...osmo-msc[master]: fix error on BSSMAP Cipher Mode Complete L3 msg IE

2019-09-18 Thread neels
neels has submitted this change and it was merged. ( 
https://gerrit.osmocom.org/c/osmo-msc/+/15317 )

Change subject: fix error on BSSMAP Cipher Mode Complete L3 msg IE
..

fix error on BSSMAP Cipher Mode Complete L3 msg IE

When an MS returns the IMEISV in the BSSMAP Cipher Mode Complete message in
the Layer 3 Message Contents IE, do not re-invoke the decode_cb() a second
time, but instead point to it from the ran_msg.cipher_mode_complete struct.

When the MSC-A decodes the Ciphering Mode Complete message, it always wants to
also decode the enclosed DTAP from the Layer 3 Message Contents IE. However,
when the MSC-I preliminarily decodes messages, it often just wants to identify
specific messages without fully acting on them, let alone dispatching RAN_UP_L2
events more than once. So leave it up to the supplied decode_cb passed to
ran_dec_l2() implementations to decide whether to decode the DTAP.

In msc_a.c hence evaluate the DTAP by passing a msgb to msc_a_up_l3(), which
will evaluate the RR Ciphering Mode Complete message found in the BSSMAP Cipher
Mode Complete's Layer 3 Message Contents IE.

Particularly, the previous choice of calling the decode_cb a second time for
the enclosed DTAP caused a header/length parsing error: the second decode_cb
call tried to mimick DTAP by overwriting the l3h pointer and truncating the
length of the msgb, but subsequently ran_a_decode_l2() would again derive the
l3h from the l2h, obliterating the intended re-interpretation as DTAP, and
hence the previous truncation caused error messages on each and every Cipher
Mode Complete message, like:

DBSSAP ERROR libmsc/ran_msg_a.c:764 
msc_a(IMSI-26242340300:MSISDN-:TMSI-0xA73E055A:GERAN-A-77923:LU)[0x5563947521e0]{MSC_A_ST_AUTH_CIPH}:
 RAN decode: BSSMAP: BSSMAP data truncated, discarding message

This error was seen a lot at CCCamp2019.

Modifying the msgb was a bad idea to begin with, the approach taken in this
patch is much cleaner.

Note that apparently many phones include the IMEISV in the Cipher Mode Complete
message even though the BSSMAP Cipher Mode Command did not include the Cipher
Response Mode IE. So, even though we did not specifically ask for the Cipher
Mode Complete to include any identity, many MS default to including the IMEISV
of their own accord. Reproduce: attach to osmo-msc with ciphering enabled using
a Samsung Galaxy S4mini.

Related: OS#4168
Change-Id: Icd8dad18d6dda24d075dd8da72c3d6db1302090d
---
M include/osmocom/msc/ran_msg.h
M src/libmsc/msc_a.c
M src/libmsc/ran_msg_a.c
3 files changed, 16 insertions(+), 11 deletions(-)

Approvals:
  Jenkins Builder: Verified



diff --git a/include/osmocom/msc/ran_msg.h b/include/osmocom/msc/ran_msg.h
index af0822b..081c7ad 100644
--- a/include/osmocom/msc/ran_msg.h
+++ b/include/osmocom/msc/ran_msg.h
@@ -210,6 +210,7 @@
 * alg_id == 0 means no such IE was present. */
uint8_t alg_id;
const char *imeisv;
+   const struct tlv_p_entry *l3_msg;
} cipher_mode_complete;
struct {
enum gsm0808_cause bssap_cause;
diff --git a/src/libmsc/msc_a.c b/src/libmsc/msc_a.c
index b414574..344b442 100644
--- a/src/libmsc/msc_a.c
+++ b/src/libmsc/msc_a.c
@@ -1407,6 +1407,18 @@
};
vlr_subscr_rx_ciph_res(vsub, VLR_CIPH_COMPL);
rc = 0;
+
+   /* Evaluate enclosed L3 message, typically Identity Response 
(IMEISV) */
+   if (msg->cipher_mode_complete.l3_msg) {
+   unsigned char *data = (unsigned 
char*)(msg->cipher_mode_complete.l3_msg->val);
+   uint16_t len = msg->cipher_mode_complete.l3_msg->len;
+   struct msgb *dtap = msgb_alloc(len, "DTAP from Cipher 
Mode Complete");
+   unsigned char *pos = msgb_put(dtap, len);
+   memcpy(pos, data, len);
+   dtap->l3h = pos;
+   rc = msc_a_up_l3(msc_a, dtap);
+   msgb_free(dtap);
+   }
break;
 
case RAN_MSG_CIPHER_MODE_REJECT:
diff --git a/src/libmsc/ran_msg_a.c b/src/libmsc/ran_msg_a.c
index 43e27f6..7672d86 100644
--- a/src/libmsc/ran_msg_a.c
+++ b/src/libmsc/ran_msg_a.c
@@ -194,18 +194,10 @@
ran_dec_msg.cipher_mode_complete.alg_id = 
ie_chosen_encr_alg->val[0];
}

-   rc = ran_decoded(ran_dec, &ran_dec_msg);
+   if (ie_l3_msg)
+   ran_dec_msg.cipher_mode_complete.l3_msg = ie_l3_msg;

-   if (ie_l3_msg) {
-   msg->l3h = (uint8_t*)ie_l3_msg->val;
-   msgb_l3trim(msg, ie_l3_msg->len);
-   ran_dec_msg = (struct ran_msg){
-   .msg_type = RAN_MSG_DTAP,
-   .msg_name = "BSSMAP Ciphering Mode Complete (L3 Message 
Contents)",
-   

Change in ...osmo-msc[master]: fix error on BSSMAP Cipher Mode Complete L3 msg IE

2019-09-18 Thread neels
Hello fixeria, laforge, Jenkins Builder,

I'd like you to reexamine a change. Please visit

https://gerrit.osmocom.org/c/osmo-msc/+/15317

to look at the new patch set (#8).

Change subject: fix error on BSSMAP Cipher Mode Complete L3 msg IE
..

fix error on BSSMAP Cipher Mode Complete L3 msg IE

When an MS returns the IMEISV in the BSSMAP Cipher Mode Complete message in
the Layer 3 Message Contents IE, do not re-invoke the decode_cb() a second
time, but instead point to it from the ran_msg.cipher_mode_complete struct.

When the MSC-A decodes the Ciphering Mode Complete message, it always wants to
also decode the enclosed DTAP from the Layer 3 Message Contents IE. However,
when the MSC-I preliminarily decodes messages, it often just wants to identify
specific messages without fully acting on them, let alone dispatching RAN_UP_L2
events more than once. So leave it up to the supplied decode_cb passed to
ran_dec_l2() implementations to decide whether to decode the DTAP.

In msc_a.c hence evaluate the DTAP by passing a msgb to msc_a_up_l3(), which
will evaluate the RR Ciphering Mode Complete message found in the BSSMAP Cipher
Mode Complete's Layer 3 Message Contents IE.

Particularly, the previous choice of calling the decode_cb a second time for
the enclosed DTAP caused a header/length parsing error: the second decode_cb
call tried to mimick DTAP by overwriting the l3h pointer and truncating the
length of the msgb, but subsequently ran_a_decode_l2() would again derive the
l3h from the l2h, obliterating the intended re-interpretation as DTAP, and
hence the previous truncation caused error messages on each and every Cipher
Mode Complete message, like:

DBSSAP ERROR libmsc/ran_msg_a.c:764 
msc_a(IMSI-26242340300:MSISDN-:TMSI-0xA73E055A:GERAN-A-77923:LU)[0x5563947521e0]{MSC_A_ST_AUTH_CIPH}:
 RAN decode: BSSMAP: BSSMAP data truncated, discarding message

This error was seen a lot at CCCamp2019.

Modifying the msgb was a bad idea to begin with, the approach taken in this
patch is much cleaner.

Note that apparently many phones include the IMEISV in the Cipher Mode Complete
message even though the BSSMAP Cipher Mode Command did not include the Cipher
Response Mode IE. So, even though we did not specifically ask for the Cipher
Mode Complete to include any identity, many MS default to including the IMEISV
of their own accord. Reproduce: attach to osmo-msc with ciphering enabled using
a Samsung Galaxy S4mini.

Related: OS#4168
Change-Id: Icd8dad18d6dda24d075dd8da72c3d6db1302090d
---
M include/osmocom/msc/ran_msg.h
M src/libmsc/msc_a.c
M src/libmsc/ran_msg_a.c
3 files changed, 16 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/17/15317/8
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/15317
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: Icd8dad18d6dda24d075dd8da72c3d6db1302090d
Gerrit-Change-Number: 15317
Gerrit-PatchSet: 8
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria 
Gerrit-Reviewer: laforge 
Gerrit-Reviewer: neels 
Gerrit-MessageType: newpatchset


Change in ...osmo-msc[master]: fix error on BSSMAP Cipher Mode Complete L3 msg IE

2019-09-18 Thread laforge
laforge has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-msc/+/15317 )

Change subject: fix error on BSSMAP Cipher Mode Complete L3 msg IE
..


Patch Set 7: Code-Review+2


--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/15317
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: Icd8dad18d6dda24d075dd8da72c3d6db1302090d
Gerrit-Change-Number: 15317
Gerrit-PatchSet: 7
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria 
Gerrit-Reviewer: laforge 
Gerrit-Reviewer: neels 
Gerrit-Comment-Date: Wed, 18 Sep 2019 08:52:28 +
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment


Change in ...osmo-msc[master]: fix error on BSSMAP Cipher Mode Complete L3 msg IE

2019-09-17 Thread fixeria
fixeria has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-msc/+/15317 )

Change subject: fix error on BSSMAP Cipher Mode Complete L3 msg IE
..


Patch Set 7: Code-Review+1


--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/15317
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: Icd8dad18d6dda24d075dd8da72c3d6db1302090d
Gerrit-Change-Number: 15317
Gerrit-PatchSet: 7
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria 
Gerrit-Reviewer: neels 
Gerrit-CC: laforge 
Gerrit-Comment-Date: Wed, 18 Sep 2019 05:13:56 +
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment


Change in ...osmo-msc[master]: fix error on BSSMAP Cipher Mode Complete L3 msg IE

2019-09-17 Thread neels
Hello Jenkins Builder,

I'd like you to reexamine a change. Please visit

https://gerrit.osmocom.org/c/osmo-msc/+/15317

to look at the new patch set (#7).

Change subject: fix error on BSSMAP Cipher Mode Complete L3 msg IE
..

fix error on BSSMAP Cipher Mode Complete L3 msg IE

When an MS returns the IMEISV in the BSSMAP Cipher Mode Complete message in
the Layer 3 Message Contents IE, do not re-invoke the decode_cb() a second
time, but instead point to it from the ran_msg.cipher_mode_complete struct.

When the MSC-A decodes the Ciphering Mode Complete message, it always wants to
also decode the enclosed DTAP from the Layer 3 Message Contents IE. However,
when the MSC-I preliminarily decodes messages, it often just wants to identify
specific messages without fully acting on them, let alone dispatching RAN_UP_L2
events more than once. So leave it up to the supplied decode_cb passed to
ran_dec_l2() implementations to decide whether to decode the DTAP.

In msc_a.c hence evaluate the DTAP by passing a non-allocated msgb instance to
msc_a_up_l3(), which will evaluate the RR Ciphering Mode Complete message found
in the BSSMAP Cipher Mode Complete's Layer 3 Message Contents IE.

Particularly, the previous choice of calling the decode_cb a second time for
the enclosed DTAP caused a header/length parsing error: the second decode_cb
call tried to mimick DTAP by overwriting the l3h pointer and truncating the
length of the msgb, but subsequently ran_a_decode_l2() would again derive the
l3h from the l2h, obliterating the intended re-interpretation as DTAP, and
hence the previous truncation caused error messages on each and every Cipher
Mode Complete message, like:

DBSSAP ERROR libmsc/ran_msg_a.c:764 
msc_a(IMSI-26242340300:MSISDN-:TMSI-0xA73E055A:GERAN-A-77923:LU)[0x5563947521e0]{MSC_A_ST_AUTH_CIPH}:
 RAN decode: BSSMAP: BSSMAP data truncated, discarding message

This error was seen a lot at CCCamp2019.

Modifying the msgb was a bad idea to begin with, the approach taken in this
patch is much cleaner.

Note that apparently many phones include the IMEISV in the Cipher Mode Complete
message even though the BSSMAP Cipher Mode Command did not include the Cipher
Response Mode IE. So, even though we did not specifically ask for the Cipher
Mode Complete to include any identity, many MS default to including the IMEISV
of their own accord. Reproduce: attach to osmo-msc with ciphering enabled using
a Samsung Galaxy S4mini.

Related: OS#4168
Change-Id: Icd8dad18d6dda24d075dd8da72c3d6db1302090d
---
M include/osmocom/msc/ran_msg.h
M src/libmsc/msc_a.c
M src/libmsc/ran_msg_a.c
3 files changed, 16 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/17/15317/7
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/15317
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: Icd8dad18d6dda24d075dd8da72c3d6db1302090d
Gerrit-Change-Number: 15317
Gerrit-PatchSet: 7
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels 
Gerrit-CC: fixeria 
Gerrit-CC: laforge 
Gerrit-MessageType: newpatchset


Change in ...osmo-msc[master]: fix error on BSSMAP Cipher Mode Complete L3 msg IE

2019-09-17 Thread neels
neels has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-msc/+/15317 )

Change subject: fix error on BSSMAP Cipher Mode Complete L3 msg IE
..


Patch Set 6:

almost forgot about this one. will do.


--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/15317
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: Icd8dad18d6dda24d075dd8da72c3d6db1302090d
Gerrit-Change-Number: 15317
Gerrit-PatchSet: 6
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels 
Gerrit-CC: fixeria 
Gerrit-CC: laforge 
Gerrit-Comment-Date: Tue, 17 Sep 2019 23:38:09 +
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment


Change in ...osmo-msc[master]: fix error on BSSMAP Cipher Mode Complete L3 msg IE

2019-09-03 Thread laforge
laforge has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-msc/+/15317 )

Change subject: fix error on BSSMAP Cipher Mode Complete L3 msg IE
..


Patch Set 6:

as indicated, I'm not arguing that right now you are smart enough to know it is 
safe.  But what about the next 5... years down the road, after (god forbid) 
another refactoring, ...?   Just do msgb_alloc() and prevent any related issues 
once and for all.  Why micro-optimize this particular use case at the cost of 
having some different behavior than in any other code path in any other osmocom 
program?


--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/15317
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: Icd8dad18d6dda24d075dd8da72c3d6db1302090d
Gerrit-Change-Number: 15317
Gerrit-PatchSet: 6
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels 
Gerrit-CC: fixeria 
Gerrit-CC: laforge 
Gerrit-Comment-Date: Tue, 03 Sep 2019 16:13:20 +
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment


Change in ...osmo-msc[master]: fix error on BSSMAP Cipher Mode Complete L3 msg IE

2019-09-02 Thread neels
neels has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-msc/+/15317 )

Change subject: fix error on BSSMAP Cipher Mode Complete L3 msg IE
..


Patch Set 6:

(1 comment)

https://gerrit.osmocom.org/#/c/15317/3/src/libmsc/msc_a.c
File src/libmsc/msc_a.c:

https://gerrit.osmocom.org/#/c/15317/3/src/libmsc/msc_a.c@1411
PS3, Line 1411: but this static msgb saves the extra allocation
> ACK. […]
I don't see how this can be a problem. None of the DTAP handling code is 
permitted to take ownership of the msgb.

What we would do here is allocate a msgb, dispatch it to DTAP, then deallocate 
it right after that here in this function. Even if it were allocated, it would 
still not be possible to put it in a queue. Nothing would be gained AFAICT.

This is the osmo-msc internal DTAP code, it is fully contained here: all 
handling of this msgb is in gsm_04_08.c. We're not talking about the general 
public or another osmocom library here.

The premise of DTAP decoding is to keep the incoming buffer unchanged, 
essentially a 'const' by convention. It is the incoming data and it is not a 
good idea to want to modify it.

Passing the msgb as 'const' is not necessary: even though this is a stack 
struct msgb, we are still free to write to it. We are not allowed to free it, 
but none of the DTAP code is allowed to free msgb passed to it -- it would lead 
to a double free.

If you guys still insist, this can become a dynamic allocation, but I am 
convinced that this patch is completely fine as it is now. The data is there, 
no need to allocate.

[BTW, talking of const msgb... I never added the 'const' to msgb args because 
the legacy gsm0408_* code neglected that. I would gladly change it to const, 
now that we have the time for it. But we do typically write l3h and cb items 
into the msgb, i.e. keep state for later in the code path. Also the msgb API 
from libosmocore often takes non-const msgb even for read-only API (e.g. 
msgb_get_u8()), which would need a lot of casting to non-const.]



--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/15317
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: Icd8dad18d6dda24d075dd8da72c3d6db1302090d
Gerrit-Change-Number: 15317
Gerrit-PatchSet: 6
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels 
Gerrit-CC: fixeria 
Gerrit-CC: laforge 
Gerrit-Comment-Date: Mon, 02 Sep 2019 23:35:47 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria 
Comment-In-Reply-To: laforge 
Gerrit-MessageType: comment


Change in ...osmo-msc[master]: fix error on BSSMAP Cipher Mode Complete L3 msg IE

2019-08-29 Thread fixeria
fixeria has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-msc/+/15317 )

Change subject: fix error on BSSMAP Cipher Mode Complete L3 msg IE
..


Patch Set 3:

(1 comment)

https://gerrit.osmocom.org/#/c/15317/3/src/libmsc/msc_a.c
File src/libmsc/msc_a.c:

https://gerrit.osmocom.org/#/c/15317/3/src/libmsc/msc_a.c@1411
PS3, Line 1411: but this static msgb saves the extra allocation
> I'm really not sure if this is the right approach. […]
ACK. This approach wouldn't be that dangerous if all DTAP-handling functions 
were accepting 'const struct msgb *', so we could be sure that nobody calls 
msgb_free(). But they don't.



--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/15317
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: Icd8dad18d6dda24d075dd8da72c3d6db1302090d
Gerrit-Change-Number: 15317
Gerrit-PatchSet: 3
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: fixeria 
Gerrit-CC: laforge 
Gerrit-Comment-Date: Thu, 29 Aug 2019 12:32:36 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge 
Gerrit-MessageType: comment


Change in ...osmo-msc[master]: fix error on BSSMAP Cipher Mode Complete L3 msg IE

2019-08-28 Thread laforge
laforge has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-msc/+/15317 )

Change subject: fix error on BSSMAP Cipher Mode Complete L3 msg IE
..


Patch Set 3:

(1 comment)

https://gerrit.osmocom.org/#/c/15317/3/src/libmsc/msc_a.c
File src/libmsc/msc_a.c:

https://gerrit.osmocom.org/#/c/15317/3/src/libmsc/msc_a.c@1411
PS3, Line 1411: but this static msgb saves the extra allocation
I'm really not sure if this is the right approach.  No code in the osmocom 
universe ever assumes msgbs are on the stack, or which may not have gone 
through normal libosmocore handling.  What if somebody later for some reason 
wants to put this on a queue?  How does msgb ownership work out here?

To me, it looks like a very dangerous premature optimization with potential to 
waste a lot of time and effort at some potential future point of development.



--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/15317
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: Icd8dad18d6dda24d075dd8da72c3d6db1302090d
Gerrit-Change-Number: 15317
Gerrit-PatchSet: 3
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge 
Gerrit-Comment-Date: Thu, 29 Aug 2019 05:39:32 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment


Change in ...osmo-msc[master]: fix error on BSSMAP Cipher Mode Complete L3 msg IE

2019-08-28 Thread neels
Hello Jenkins Builder,

I'd like you to reexamine a change. Please visit

https://gerrit.osmocom.org/c/osmo-msc/+/15317

to look at the new patch set (#3).

Change subject: fix error on BSSMAP Cipher Mode Complete L3 msg IE
..

fix error on BSSMAP Cipher Mode Complete L3 msg IE

When an MS returns the IMEISV in the BSSMAP Cipher Mode Complete message in
the Layer 3 Message Contents IE, do not re-invoke the decode_cb() a second
time, but instead point to it from the ran_msg.cipher_mode_complete struct.

When the MSC-A decodes the Ciphering Mode Complete message, it always wants to
also decode the enclosed DTAP from the Layer 3 Message Contents IE. However,
when the MSC-I preliminarily decodes messages, it often just wants to identify
specific messages without fully acting on them, let alone dispatching RAN_UP_L2
events more than once. So leave it up to the supplied decode_cb passed to
ran_dec_l2() implementations to decide whether to decode the DTAP.

In msc_a.c hence evaluate the DTAP by passing a non-allocated msgb instance to
msc_a_up_l3(), which will evaluate the RR Ciphering Mode Complete message found
in the BSSMAP Cipher Mode Complete's Layer 3 Message Contents IE.

Particularly, the previous choice of calling the decode_cb a second time for
the enclosed DTAP caused a header/length parsing error: the second decode_cb
call tried to mimick DTAP by overwriting the l3h pointer and truncating the
length of the msgb, but subsequently ran_a_decode_l2() would again derive the
l3h from the l2h, obliterating the intended re-interpretation as DTAP, and
hence the previous truncation caused error messages on each and every Cipher
Mode Complete message, like:

DBSSAP ERROR libmsc/ran_msg_a.c:764 
msc_a(IMSI-26242340300:MSISDN-:TMSI-0xA73E055A:GERAN-A-77923:LU)[0x5563947521e0]{MSC_A_ST_AUTH_CIPH}:
 RAN decode: BSSMAP: BSSMAP data truncated, discarding message

This error was seen a lot at CCCamp2019.

Modifying the msgb was a bad idea to begin with, the approach taken in this
patch is much cleaner.

Note that apparently many phones include the IMEISV in the Cipher Mode Complete
message even though the BSSMAP Cipher Mode Command did not include the Cipher
Response Mode IE. So, even though we did not specifically ask for the Cipher
Mode Complete to include any identity, many MS default to including the IMEISV
of their own accord. Reproduce: attach to osmo-msc with ciphering enabled using
a Samsung Galaxy S4mini.

Related: OS#4168
Change-Id: Icd8dad18d6dda24d075dd8da72c3d6db1302090d
---
M include/osmocom/msc/ran_msg.h
M src/libmsc/msc_a.c
M src/libmsc/ran_msg_a.c
3 files changed, 22 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/17/15317/3
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/15317
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: Icd8dad18d6dda24d075dd8da72c3d6db1302090d
Gerrit-Change-Number: 15317
Gerrit-PatchSet: 3
Gerrit-Owner: neels 
Gerrit-Reviewer: Jenkins Builder
Gerrit-MessageType: newpatchset