From b0a32e58d11f7dd79272a178eef838980b980d41 Mon Sep 17 00:00:00 2001 From: Philipp Date: Fri, 16 Dec 2016 16:03:28 +0100 Subject: sndcp: Allow empty SNDCP-XID indications In some rare cases the modem might send a xid indication that does not contain anything except the version number field. The sgsn ignors such SNDCP-XID indications by stripping the entire field from the response. We found a modem in the wild that started to act problematic when the empty SNDCP-XID was missing in the response. This patch changes the XID negotiation behaviour in a way that if a modem should send empty SNDCP-XID indications, the reply will also contain an empty SNDCP-XID indication. Apart from that the SNDCP-XID version number is now parsed and echoed in the response. This ensures that we always reply with the version number that the modem expects. (The version was 0 in all cases we observed so far) --- openbsc/include/openbsc/gprs_sndcp_xid.h | 14 +++++---- openbsc/src/gprs/gprs_llc.c | 14 ++++----- openbsc/src/gprs/gprs_sndcp.c | 25 +++++++-------- openbsc/src/gprs/gprs_sndcp_xid.c | 54 +++++++++++++++++++++----------- openbsc/tests/sndcp_xid/sndcp_xid_test.c | 10 +++--- 5 files changed, 68 insertions(+), 49 deletions(-) diff --git a/openbsc/include/openbsc/gprs_sndcp_xid.h b/openbsc/include/openbsc/gprs_sndcp_xid.h index 02904a7d2..e64bc5237 100644 --- a/openbsc/include/openbsc/gprs_sndcp_xid.h +++ b/openbsc/include/openbsc/gprs_sndcp_xid.h @@ -24,7 +24,7 @@ #include #include -#define CURRENT_SNDCP_VERSION 0 /* See 3GPP TS 44.065, clause 8 */ +#define DEFAULT_SNDCP_VERSION 0 /* See 3GPP TS 44.065, clause 8 */ #define MAX_ENTITIES 32 /* 3GPP TS 44.065 reserves 5 bit * for compression enitity number */ @@ -197,13 +197,15 @@ enum gprs_sndcp_dcomp_v44_dcomp { /* Transform a list with compression fields into an SNDCP-XID message (dst) */ int gprs_sndcp_compile_xid(uint8_t *dst, unsigned int dst_maxlen, - const struct llist_head *comp_fields); + const struct llist_head *comp_fields, int version); /* Transform an SNDCP-XID message (src) into a list of SNDCP-XID fields */ -struct llist_head *gprs_sndcp_parse_xid(const void *ctx, - const uint8_t * src, - unsigned int src_len, - const struct llist_head *comp_fields_req); +struct llist_head *gprs_sndcp_parse_xid(int *version, + const void *ctx, + const uint8_t *src, + unsigned int src_len, + const struct llist_head + *comp_fields_req); /* Find out to which compression class the specified comp-field belongs * (header compression or data compression?) */ diff --git a/openbsc/src/gprs/gprs_llc.c b/openbsc/src/gprs/gprs_llc.c index 021502932..eb4abfd76 100644 --- a/openbsc/src/gprs/gprs_llc.c +++ b/openbsc/src/gprs/gprs_llc.c @@ -136,7 +136,7 @@ static int gprs_llc_process_xid_conf(uint8_t *bytes, int bytes_len, struct gprs_llc_lle *lle) { /* Note: This function handles the response of a network originated - * XID-Request. There XID messages reflected by the phone are analyzed + * XID-Request. There XID messages reflected by the modem are analyzed * and processed here. The caller is called by rx_llc_xid(). */ struct llist_head *xid_fields; @@ -202,7 +202,7 @@ static int gprs_llc_process_xid_ind(uint8_t *bytes_request, struct gprs_llc_lle *lle) { /* Note: This function computes the response that is sent back to the - * phone when a phone originated XID is received. The function is + * modem when a modem originated XID is received. The function is * called by rx_llc_xid() */ int rc = -EINVAL; @@ -229,7 +229,7 @@ static int gprs_llc_process_xid_ind(uint8_t *bytes_request, * for validity. Currently we just blindly * accept all XID fields by just echoing them. * There is a remaining risk of malfunction - * when a phone submits values which defer from + * when a modem submits values which defer from * the default! */ LOGP(DLLC, LOGL_NOTICE, "Echoing XID-Field: XID: type=%d, data_len=%d, data=%s\n", @@ -271,7 +271,7 @@ static int gprs_llc_process_xid_ind(uint8_t *bytes_request, return rc; } -/* Dispatch XID indications and responses comming from the Phone */ +/* Dispatch XID indications and responses comming from the modem */ static void rx_llc_xid(struct gprs_llc_lle *lle, struct gprs_llc_hdr_parsed *gph) { @@ -281,7 +281,7 @@ static void rx_llc_xid(struct gprs_llc_lle *lle, /* FIXME: 8.5.3.3: check if XID is invalid */ if (gph->is_cmd) { LOGP(DLLC, LOGL_NOTICE, - "Received XID indication from phone.\n"); + "Received XID indication from modem.\n"); struct msgb *resp; uint8_t *xid; @@ -301,7 +301,7 @@ static void rx_llc_xid(struct gprs_llc_lle *lle, gprs_llc_tx_xid(lle, resp, 0); } else { LOGP(DLLC, LOGL_NOTICE, - "Received XID confirmation from phone.\n"); + "Received XID confirmation from modem.\n"); gprs_llc_process_xid_conf(gph->data, gph->data_len, lle); /* FIXME: if we had sent a XID reset, send * LLGMM-RESET.conf to GMM */ @@ -331,7 +331,7 @@ int gprs_ll_xid_req(struct gprs_llc_lle *lle, msg = msgb_alloc_headroom(4096, 1024, "LLC_XID"); xid = msgb_put(msg, xid_bytes_len); memcpy(xid, xid_bytes, xid_bytes_len); - LOGP(DLLC, LOGL_NOTICE, "Sending XID request to phone...\n"); + LOGP(DLLC, LOGL_NOTICE, "Sending XID request to modem...\n"); gprs_llc_tx_xid(lle, msg, 1); } else { LOGP(DLLC, LOGL_ERROR, diff --git a/openbsc/src/gprs/gprs_sndcp.c b/openbsc/src/gprs/gprs_sndcp.c index 1cbeede09..77c9507ad 100644 --- a/openbsc/src/gprs/gprs_sndcp.c +++ b/openbsc/src/gprs/gprs_sndcp.c @@ -960,8 +960,13 @@ static int gprs_llc_gen_sndcp_xid(uint8_t *bytes, int bytes_len, uint8_t nsapi) llist_add(&v42bis_comp_field.list, &comp_fields); } + /* Do not attempt to compile anything if there is no data in the list */ + if(llist_empty(&comp_fields)) + return 0; + /* Compile bytestream */ - return gprs_sndcp_compile_xid(bytes, bytes_len, &comp_fields); + return gprs_sndcp_compile_xid(bytes, bytes_len, &comp_fields, + DEFAULT_SNDCP_VERSION); } /* Set of SNDCP-XID bnegotiation (See also: TS 144 065, @@ -1106,6 +1111,7 @@ int sndcp_sn_xid_ind(struct gprs_llc_xid_field *xid_field_indication, int rc; int compclass; + int version; struct llist_head *comp_fields; struct gprs_sndcp_comp_field *comp_field; @@ -1115,22 +1121,13 @@ int sndcp_sn_xid_ind(struct gprs_llc_xid_field *xid_field_indication, OSMO_ASSERT(lle); /* Parse SNDCP-CID XID-Field */ - comp_fields = gprs_sndcp_parse_xid(lle->llme, + comp_fields = gprs_sndcp_parse_xid(&version, lle->llme, xid_field_indication->data, xid_field_indication->data_len, NULL); if (!comp_fields) return -EINVAL; - /* Don't bother with empty indications */ - if (llist_empty(comp_fields)) { - xid_field_response->data = NULL; - xid_field_response->data_len = 0; - DEBUGP(DSNDCP, - "SNDCP-XID indication did not contain any parameters!\n"); - return 0; - } - /* Handle compression entites */ DEBUGP(DSNDCP, "SNDCP-XID-IND (ms):\n"); gprs_sndcp_dump_comp_fields(comp_fields, LOGL_DEBUG); @@ -1168,7 +1165,7 @@ int sndcp_sn_xid_ind(struct gprs_llc_xid_field *xid_field_indication, /* Compile modified SNDCP-XID bytes */ rc = gprs_sndcp_compile_xid(xid_field_response->data, xid_field_indication->data_len, - comp_fields); + comp_fields, 0); if (rc > 0) xid_field_response->data_len = rc; @@ -1210,7 +1207,7 @@ int sndcp_sn_xid_conf(struct gprs_llc_xid_field *xid_field_conf, OSMO_ASSERT(xid_field_request); /* Parse SNDCP-CID XID-Field */ - comp_fields_req = gprs_sndcp_parse_xid(lle->llme, + comp_fields_req = gprs_sndcp_parse_xid(NULL, lle->llme, xid_field_request->data, xid_field_request->data_len, NULL); @@ -1221,7 +1218,7 @@ int sndcp_sn_xid_conf(struct gprs_llc_xid_field *xid_field_conf, gprs_sndcp_dump_comp_fields(comp_fields_req, LOGL_DEBUG); /* Parse SNDCP-CID XID-Field */ - comp_fields_conf = gprs_sndcp_parse_xid(lle->llme, + comp_fields_conf = gprs_sndcp_parse_xid(NULL, lle->llme, xid_field_conf->data, xid_field_conf->data_len, comp_fields_req); diff --git a/openbsc/src/gprs/gprs_sndcp_xid.c b/openbsc/src/gprs/gprs_sndcp_xid.c index bb43eab68..37551e462 100644 --- a/openbsc/src/gprs/gprs_sndcp_xid.c +++ b/openbsc/src/gprs/gprs_sndcp_xid.c @@ -549,26 +549,29 @@ static int gprs_sndcp_pack_fields(const struct llist_head *comp_fields, /* Transform a list with compression fields into an SNDCP-XID message (dst) */ int gprs_sndcp_compile_xid(uint8_t *dst, unsigned int dst_maxlen, - const struct llist_head *comp_fields) + const struct llist_head *comp_fields, int version) { int rc; int byte_counter = 0; uint8_t comp_bytes[512]; - uint8_t xid_version_number[1] = { CURRENT_SNDCP_VERSION }; + uint8_t xid_version_number[1]; OSMO_ASSERT(comp_fields); OSMO_ASSERT(dst); OSMO_ASSERT(dst_maxlen >= 2 + sizeof(xid_version_number)); - /* Bail if there is no input */ - if (llist_empty(comp_fields)) - return -EINVAL; + /* Prepend header with version number */ + if (version >= 0) { + xid_version_number[0] = (uint8_t) (version & 0xff); + dst = + tlv_put(dst, SNDCP_XID_VERSION_NUMBER, + sizeof(xid_version_number), xid_version_number); + byte_counter += (sizeof(xid_version_number) + 2); + } - /* Prepend header */ - dst = - tlv_put(dst, SNDCP_XID_VERSION_NUMBER, - sizeof(xid_version_number), xid_version_number); - byte_counter += (sizeof(xid_version_number) + 2); + /* Stop if there is no compression fields supplied */ + if (llist_empty(comp_fields)) + return byte_counter; /* Add data compression fields */ rc = gprs_sndcp_pack_fields(comp_fields, comp_bytes, @@ -1283,11 +1286,10 @@ static int decode_xid_block(struct llist_head *comp_fields, uint8_t tag, } /* Transform an SNDCP-XID message (src) into a list of SNDCP-XID fields */ -static int gprs_sndcp_decode_xid(struct llist_head *comp_fields, +static int gprs_sndcp_decode_xid(int *version, struct llist_head *comp_fields, const uint8_t *src, unsigned int src_len, - const struct - entity_algo_table - *lt, unsigned int lt_len) + const struct entity_algo_table *lt, + unsigned int lt_len) { int src_pos = 0; uint8_t tag; @@ -1297,6 +1299,10 @@ static int gprs_sndcp_decode_xid(struct llist_head *comp_fields, int rc; int tlv_count = 0; + /* Preset version value as invalid */ + if(version) + *version = -1; + /* Valid TLV-Tag and types */ static const struct tlv_definition sndcp_xid_def = { .def = { @@ -1327,6 +1333,10 @@ static int gprs_sndcp_decode_xid(struct llist_head *comp_fields, return -EINVAL; } + /* Decode sndcp xid version number */ + if (version && tag == SNDCP_XID_VERSION_NUMBER) + *version = val[0]; + /* Decode compression parameters */ if ((tag == SNDCP_XID_PROTOCOL_COMPRESSION) || (tag == SNDCP_XID_DATA_COMPRESSION)) { @@ -1548,7 +1558,8 @@ static int gprs_sndcp_complete_comp_fields(struct llist_head } /* Transform an SNDCP-XID message (src) into a list of SNDCP-XID fields */ -struct llist_head *gprs_sndcp_parse_xid(const void *ctx, +struct llist_head *gprs_sndcp_parse_xid(int *version, + const void *ctx, const uint8_t *src, unsigned int src_len, const struct llist_head @@ -1559,6 +1570,12 @@ struct llist_head *gprs_sndcp_parse_xid(const void *ctx, struct llist_head *comp_fields; struct entity_algo_table lt[MAX_ENTITIES * 2]; + /* In case of a zero length field, just exit */ + if(src_len == 0) + return NULL; + + /* We should go any further if we have a field length greater + * zero and a null pointer as buffer! */ OSMO_ASSERT(src); comp_fields = talloc_zero(ctx, struct llist_head); @@ -1575,8 +1592,8 @@ struct llist_head *gprs_sndcp_parse_xid(const void *ctx, } /* Parse SNDCP-CID XID-Field */ - rc = gprs_sndcp_decode_xid(comp_fields, src, src_len, lt, - lt_len); + rc = gprs_sndcp_decode_xid(version, comp_fields, src, src_len, + lt, lt_len); if (rc < 0) { talloc_free(comp_fields); return NULL; @@ -1591,7 +1608,8 @@ struct llist_head *gprs_sndcp_parse_xid(const void *ctx, } else { /* Parse SNDCP-CID XID-Field */ - rc = gprs_sndcp_decode_xid(comp_fields, src, src_len, NULL, 0); + rc = gprs_sndcp_decode_xid(version, comp_fields, src, src_len, + NULL, 0); if (rc < 0) { talloc_free(comp_fields); return NULL; diff --git a/openbsc/tests/sndcp_xid/sndcp_xid_test.c b/openbsc/tests/sndcp_xid/sndcp_xid_test.c index 3a3361970..151dd2bb5 100644 --- a/openbsc/tests/sndcp_xid/sndcp_xid_test.c +++ b/openbsc/tests/sndcp_xid/sndcp_xid_test.c @@ -47,13 +47,14 @@ static void test_xid_decode_realworld(const void *ctx) uint8_t xid_r[512]; /* Parse and show contained comp fields */ - comp_fields = gprs_sndcp_parse_xid(ctx, xid, sizeof(xid), NULL); + comp_fields = gprs_sndcp_parse_xid(NULL, ctx, xid, sizeof(xid), NULL); OSMO_ASSERT(comp_fields); printf("Decoded:\n"); gprs_sndcp_dump_comp_fields(comp_fields, DSNDCP); /* Encode comp-fields again */ - rc = gprs_sndcp_compile_xid(xid_r,sizeof(xid_r), comp_fields); + rc = gprs_sndcp_compile_xid(xid_r,sizeof(xid_r), comp_fields, + DEFAULT_SNDCP_VERSION); printf("Result length=%i\n",rc); printf("Encoded: %s\n", osmo_hexdump_nospc(xid, sizeof(xid))); printf("Rencoded: %s\n", osmo_hexdump_nospc(xid_r, rc)); @@ -226,13 +227,14 @@ static void test_xid_encode_decode(const void *ctx) gprs_sndcp_dump_comp_fields(&comp_fields, DSNDCP); /* Encode SNDCP-XID fields */ - rc = gprs_sndcp_compile_xid(xid, xid_len, &comp_fields); + rc = gprs_sndcp_compile_xid(xid, xid_len, &comp_fields, + DEFAULT_SNDCP_VERSION); OSMO_ASSERT(rc > 0); printf("Encoded: %s (%i bytes)\n", osmo_hexdump_nospc(xid, rc), rc); /* Parse and show contained comp fields */ - comp_fields_dec = gprs_sndcp_parse_xid(ctx, xid, rc, NULL); + comp_fields_dec = gprs_sndcp_parse_xid(NULL, ctx, xid, rc, NULL); OSMO_ASSERT(comp_fields_dec); printf("Decoded:\n"); -- cgit v1.2.3