Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/12349 )
Change subject: improve handling of BSC-chosen algo in CIPHER MODE COMPLETE ...................................................................... Patch Set 3: Code-Review-1 (11 comments) https://gerrit.osmocom.org/#/c/12349/2/include/osmocom/msc/gsm_04_08.h File include/osmocom/msc/gsm_04_08.h: https://gerrit.osmocom.org/#/c/12349/2/include/osmocom/msc/gsm_04_08.h@80 PS2, Line 80: int gsm48_conn_sendmsg(struct msgb *msg, struct ran_conn *conn, struct gsm_trans *trans); > Yes that works, too. Done in next patch set. Actually in .h files, in general, please rather define opaque structs. Headers grow less dependency spaghetti and side effects if we just define opaque structs. #include ran_conn.h would rather belong in the .c file. It's not really a problem, no need to change it back again. But for next time: @Max, an opaque struct is good, not bad; and @stsp, next time refuse to #include lots of stuff if all you need is a single struct pointer. https://gerrit.osmocom.org/#/c/12349/2/src/libmsc/a_iface_bssap.c File src/libmsc/a_iface_bssap.c: https://gerrit.osmocom.org/#/c/12349/2/src/libmsc/a_iface_bssap.c@410 PS2, Line 410: int supported; > This was copied from existing code in gsm_04_08.c. I'll fix it there, too. it's because there is A5/0 .. A5/7. That's eight. This is a fact throughout the encryption code. BTW, A5/4...7 don't even exist, only as placeholders / defined bits in the specs. https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c File src/libmsc/a_iface_bssap.c: https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@409 PS3, Line 409: for (i = 0; i < ENCRY_INFO_PERM_ALGO_MAXLEN; i++) { This loop wants to iterate the a5_encryption_mask, so it should actually be exactly i < 8. This constant instead defines how many perm_algo[] items are allowed. Those are theoretically two different pairs of shoes, but incidentally they are the same. Consider, even though 8 encryption types exist, the amount of permitted algos passed in that bssmap ie could have been chosen as three, or five, or two... Right. Both are eight. So it comes down to the same thing. At least now you know what things mean. https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@419 PS3, Line 419: ei.perm_algo[j++] = vlr_ciph_to_gsm0808_alg_id(i); this here could instead make sure that j < ENCRY_INFO_PERM_ALGO_MAXLEN https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@426 PS3, Line 426: vlr_ciph = ei.perm_algo[ei.perm_algo_len - 1] - 1; do we have a reverse function for vlr_ciph_to_gsm0808_alg_id()? The -1 might be correct, but it's a magic number here. https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@429 PS3, Line 429: LOGPCONN(conn, LOGL_NOTICE, "BSC didn't specify algorithm in CIHPER MODE COMPLETE; falling back to %s\n", You are parsing the "Ciphering is complete" message. Ciphering is already in place between MS and BTS, it's not like you could fall back or anything. Either you know what cipher is being used, or you don't. But definitely ciphering is being used now. https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@430 PS3, Line 430: get_value_string(vlr_ciph_names, vlr_ciph)); just use vlr_ciph_name(vlr_ciph) https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@440 PS3, Line 440: LOGPCONN(conn, LOGL_ERROR, "Unsupported encryption algorithm in CIHPER MODE COMPLETE: %s\n", "Non-permitted"? https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/a_iface_bssap.c@456 PS3, Line 456: ran_conn_cipher_mode_compl(conn, msg, vlr_ciph); maybe this function needs to be taught to not pass a cipher algo to the MSC when the BTS didn't send an IE with the chosen cipher. This entire "which cipher is it" is merely informational, also towards the MSC. All this is in the end after all ciphering negotiation, and nothing can be changed about it at this point. https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/gsm_04_08.c File src/libmsc/gsm_04_08.c: https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/gsm_04_08.c@125 PS3, Line 125: int gsm48_classmark_supports_a5(const struct gsm_classmark *cm, uint8_t a5) gsm48 is a prefix commonly used by old libosmocore API. Would prefer to not use that here as well. Since there is only one kind of classmark, 'classmark_..' is fine even for a non-static function, it is still used only in the msc. https://gerrit.osmocom.org/#/c/12349/3/src/libmsc/gsm_04_08.c@1613 PS3, Line 1613: for (i = 0; i < ENCRY_INFO_PERM_ALGO_MAXLEN; i++) { again, 8 is accurate here, because of (1 << i), and this is not a gsm0808 array of perm algo bytes. -- To view, visit https://gerrit.osmocom.org/12349 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: I3260bee43cfe135ebfc33c13aee3c4ba43466c81 Gerrit-Change-Number: 12349 Gerrit-PatchSet: 3 Gerrit-Owner: Stefan Sperling <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Max <[email protected]> Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Stefan Sperling <[email protected]> Gerrit-Comment-Date: Fri, 21 Dec 2018 02:52:10 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
