[PATCH 1/4] ip.access: Fix bad call to patch_nm_tables when configuring channel
From: Sylvain Munaut <tnt@246tNt.com> The bts variable isn't even defined at that point causing crashes. The patch_nm_tables is called for BTS object class. Signed-off-by: Sylvain Munaut <tnt@246tNt.com> --- openbsc/src/bsc_init.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/openbsc/src/bsc_init.c b/openbsc/src/bsc_init.c index 1394dd8..0adc656 100644 --- a/openbsc/src/bsc_init.c +++ b/openbsc/src/bsc_init.c @@ -378,7 +378,6 @@ int nm_state_event(enum nm_evt evt, u_int8_t obj_class, void *obj, trx = ts->trx; if (new_state->operational == 1 && new_state->availability == NM_AVSTATE_DEPENDENCY) { - patch_nm_tables(bts); enum abis_nm_chan_comb ccomb = abis_nm_chcomb4pchan(ts->pchan); abis_nm_set_channel_attr(ts, ccomb); -- 1.6.4
From: Sylvain Munaut <tnt@246tNt.com> Signed-off-by: Sylvain Munaut <tnt@246tNt.com> --- openbsc/src/openbsc.cfg.nanobts | 38 ++++++++++++++++++++++++++++++++++++++ 1 files changed, 38 insertions(+), 0 deletions(-) create mode 100644 openbsc/src/openbsc.cfg.nanobts diff --git a/openbsc/src/openbsc.cfg.nanobts b/openbsc/src/openbsc.cfg.nanobts new file mode 100644 index 0000000..a12794f --- /dev/null +++ b/openbsc/src/openbsc.cfg.nanobts @@ -0,0 +1,38 @@ +! +! OpenBSC configuration saved from vty +! +password foo +! +line vty + no login +! +network + network country code 1 + mobile network code 1 + short name OpenBSC + long name OpenBSC + bts 0 + type nanobts + ip.access unit_id 1801 0 + band GSM1800 + location_area_code 1 + training_sequence_code 7 + base_station_id_code 63 + trx 0 + arfcn 514 + timeslot 0 + phys_chan_config CCCH+SDCCH4 + timeslot 1 + phys_chan_config SDCCH8 + timeslot 2 + phys_chan_config TCH/F + timeslot 3 + phys_chan_config TCH/F + timeslot 4 + phys_chan_config TCH/F + timeslot 5 + phys_chan_config TCH/F + timeslot 6 + phys_chan_config TCH/F + timeslot 7 + phys_chan_config TCH/F -- 1.6.4
From: Sylvain Munaut <tnt@246tNt.com> This is needed when you need to manually parse TLV blocks that don't follow the logic supported by tlv_parse but you still want to rely on working code and not fiddle with details. Signed-off-by: Sylvain Munaut <tnt@246tNt.com> --- openbsc/include/openbsc/tlv.h | 3 + openbsc/src/tlv_parser.c | 173 ++++++++++++++++++++++++----------------- 2 files changed, 104 insertions(+), 72 deletions(-) diff --git a/openbsc/include/openbsc/tlv.h b/openbsc/include/openbsc/tlv.h index 6da4fb1..0cf4388 100644 --- a/openbsc/include/openbsc/tlv.h +++ b/openbsc/include/openbsc/tlv.h @@ -207,6 +207,9 @@ struct tlv_parsed { extern struct tlv_definition tvlv_att_def; +int tlv_parse_one(u_int8_t *o_tag, u_int16_t *o_len, const u_int8_t **o_val, + const struct tlv_definition *def, + const u_int8_t *buf, int buf_len); int tlv_parse(struct tlv_parsed *dec, const struct tlv_definition *def, const u_int8_t *buf, int buf_len, u_int8_t lv_tag, u_int8_t lv_tag2); diff --git a/openbsc/src/tlv_parser.c b/openbsc/src/tlv_parser.c index 8321b88..fd0045f 100644 --- a/openbsc/src/tlv_parser.c +++ b/openbsc/src/tlv_parser.c @@ -16,6 +16,82 @@ int tlv_dump(struct tlv_parsed *dec) return 0; } +/* o_tag: output: tag found + * o_len: output: length of the data + * o_val: output: pointer to the data + * def: input: a structure defining the valid TLV tags / configurations + * buf: input: the input data buffer to be parsed + * buf_len: input: the length of the input data buffer + * + * Also, returns the number of bytes consumed by the TLV entry + */ +int tlv_parse_one(u_int8_t *o_tag, u_int16_t *o_len, const u_int8_t **o_val, + const struct tlv_definition *def, + const u_int8_t *buf, int buf_len) +{ + u_int8_t tag; + int len; + + tag = *buf; + *o_tag = tag; + + /* FIXME: use tables for knwon IEI */ + switch (def->def[tag].type) { + case TLV_TYPE_T: + /* GSM TS 04.07 11.2.4: Type 1 TV or Type 2 T */ + *o_val = buf; + *o_len = 0; + len = 1; + break; + case TLV_TYPE_TV: + *o_val = buf+1; + *o_len = 1; + len = 2; + break; + case TLV_TYPE_FIXED: + *o_val = buf+1; + *o_len = def->def[tag].fixed_len; + len = def->def[tag].fixed_len + 1; + break; + case TLV_TYPE_TLV: + /* GSM TS 04.07 11.2.4: Type 4 TLV */ + if (buf + 1 > buf + buf_len) + return -1; + *o_val = buf+2; + *o_len = *(buf+1); + len = *o_len + 2; + if (len > buf_len) + return -2; + break; + case TLV_TYPE_TvLV: + if (*(buf+1) & 0x80) { + /* like TLV, but without highest bit of len */ + if (buf + 1 > buf + buf_len) + return -1; + *o_val = buf+2; + *o_len = *(buf+1) & 0x7f; + len = *o_len + 2; + if (len > buf_len) + return -2; + break; + } + /* like TL16V, fallthrough */ + case TLV_TYPE_TL16V: + if (2 > buf_len) + return -1; + *o_val = buf+3; + *o_len = *(buf+1) << 8 | *(buf+2); + len = *o_len + 3; + if (len > buf_len) + return -2; + break; + default: + return -3; + } + + return len; +} + /* dec: output: a caller-allocated pointer to a struct tlv_parsed, * def: input: a structure defining the valid TLV tags / configurations * buf: input: the input data buffer to be parsed @@ -27,94 +103,47 @@ int tlv_parse(struct tlv_parsed *dec, const struct tlv_definition *def, const u_int8_t *buf, int buf_len, u_int8_t lv_tag, u_int8_t lv_tag2) { - u_int8_t tag, len = 1; - const u_int8_t *pos = buf; - int num_parsed = 0; + int ofs = 0, num_parsed = 0; + u_int16_t len; memset(dec, 0, sizeof(*dec)); if (lv_tag) { - if (pos > buf + buf_len) + if (ofs > buf_len) return -1; - dec->lv[lv_tag].val = pos+1; - dec->lv[lv_tag].len = *pos; + dec->lv[lv_tag].val = &buf[ofs+1]; + dec->lv[lv_tag].len = buf[ofs]; len = dec->lv[lv_tag].len + 1; - if (pos + len > buf + buf_len) + if (ofs + len > buf_len) return -2; num_parsed++; - pos += len; + ofs += len; } if (lv_tag2) { - if (pos > buf + buf_len) + if (ofs > buf_len) return -1; - dec->lv[lv_tag2].val = pos+1; - dec->lv[lv_tag2].len = *pos; + dec->lv[lv_tag2].val = &buf[ofs+1]; + dec->lv[lv_tag2].len = buf[ofs]; len = dec->lv[lv_tag2].len + 1; - if (pos + len > buf + buf_len) + if (ofs + len > buf_len) return -2; num_parsed++; - pos += len; + ofs += len; } - for (; pos < buf+buf_len; pos += len) { - tag = *pos; - /* FIXME: use tables for knwon IEI */ - switch (def->def[tag].type) { - case TLV_TYPE_T: - /* GSM TS 04.07 11.2.4: Type 1 TV or Type 2 T */ - dec->lv[tag].val = pos; - dec->lv[tag].len = 0; - len = 1; - num_parsed++; - break; - case TLV_TYPE_TV: - dec->lv[tag].val = pos+1; - dec->lv[tag].len = 1; - len = 2; - num_parsed++; - break; - case TLV_TYPE_FIXED: - dec->lv[tag].val = pos+1; - dec->lv[tag].len = def->def[tag].fixed_len; - len = def->def[tag].fixed_len + 1; - num_parsed++; - break; - case TLV_TYPE_TLV: - /* GSM TS 04.07 11.2.4: Type 4 TLV */ - if (pos + 1 > buf + buf_len) - return -1; - dec->lv[tag].val = pos+2; - dec->lv[tag].len = *(pos+1); - len = dec->lv[tag].len + 2; - if (pos + len > buf + buf_len) - return -2; - num_parsed++; - break; - case TLV_TYPE_TvLV: - if (*(pos+1) & 0x80) { - /* like TLV, but without highest bit of len */ - if (pos + 1 > buf + buf_len) - return -1; - dec->lv[tag].val = pos+2; - dec->lv[tag].len = *(pos+1) & 0x7f; - len = dec->lv[tag].len + 2; - if (pos + len > buf + buf_len) - return -2; - num_parsed++; - break; - } - /* like TL16V, fallthrough */ - case TLV_TYPE_TL16V: - if (pos + 2 > buf + buf_len) - return -1; - dec->lv[tag].val = pos+3; - dec->lv[tag].len = *(pos+1) << 8 | *(pos+2); - len = dec->lv[tag].len + 3; - if (pos + len > buf + buf_len) - return -2; - num_parsed++; - break; - } + while (ofs < buf_len) { + int rv; + u_int8_t tag; + const u_int8_t *val; + + rv = tlv_parse_one(&tag, &len, &val, def, + &buf[ofs], buf_len-ofs); + if (rv < 0) + return rv; + dec->lv[tag].val = val; + dec->lv[tag].len = len; + ofs += rv; + num_parsed++; } //tlv_dump(dec); return num_parsed; -- 1.6.4
From: Sylvain Munaut <tnt@246tNt.com> The previous code only sent the FILE_ID tag data part, but according to the GSM 12.21 spec, section 8.3.6, the full SW Description 'object' must be sent so that includes the NM_ATT_SW_DESCR tag, the whole FILE_ID and the whole FILE_VERSION (including tags & length fields). Note that functionnaly on a nanoBTS 139 I couldn't see any difference ... whatever I send in there it works ... Signed-off-by: Sylvain Munaut <tnt@246tNt.com> --- openbsc/src/abis_nm.c | 61 +++++++++++++++++++++++++++++++++++++++--------- 1 files changed, 49 insertions(+), 12 deletions(-) diff --git a/openbsc/src/abis_nm.c b/openbsc/src/abis_nm.c index 03d9def..c2641c4 100755 --- a/openbsc/src/abis_nm.c +++ b/openbsc/src/abis_nm.c @@ -878,16 +878,57 @@ static int ipacc_sw_activate(struct gsm_bts *bts, u_int8_t obj_class, u_int8_t i return abis_nm_sendmsg(bts, msg); } +static int abis_nm_parse_sw_descr(const u_int8_t *sw_descr, int sw_descr_len) +{ + static const struct tlv_definition sw_descr_def = { + .def = { + [NM_ATT_FILE_ID] = { TLV_TYPE_TL16V, }, + [NM_ATT_FILE_VERSION] = { TLV_TYPE_TL16V, }, + }, + }; + + u_int8_t tag; + u_int16_t tag_len; + const u_int8_t *val; + int ofs = 0, len; + + /* Classic TLV parsing doesn't work well with SW_DESCR because of it's + * nested nature and the fact you have to assume it contains only two sub + * tags NM_ATT_FILE_VERSION & NM_ATT_FILE_ID to parse it */ + + if (sw_descr[0] != NM_ATT_SW_DESCR) { + DEBUGP(DNM, "SW_DESCR attribute identifier not found!\n"); + return -1; + } + ofs += 1; + + len = tlv_parse_one(&tag, &tag_len, &val, + &sw_descr_def, &sw_descr[ofs], sw_descr_len-ofs); + if (len < 0 || (tag != NM_ATT_FILE_ID)) { + DEBUGP(DNM, "FILE_ID attribute identifier not found!\n"); + return -2; + } + ofs += len; + + len = tlv_parse_one(&tag, &tag_len, &val, + &sw_descr_def, &sw_descr[ofs], sw_descr_len-ofs); + if (len < 0 || (tag != NM_ATT_FILE_VERSION)) { + DEBUGP(DNM, "FILE_VERSION attribute identifier not found!\n"); + return -3; + } + ofs += len; + + return ofs; +} + static int abis_nm_rx_sw_act_req(struct msgb *mb) { struct abis_om_hdr *oh = msgb_l2(mb); struct abis_om_fom_hdr *foh = msgb_l3(mb); struct tlv_parsed tp; const u_int8_t *sw_config; - int sw_config_len; - int file_id_len; int nack = 0; - int ret; + int ret, sw_config_len, sw_descr_len; debugp_foh(foh); @@ -918,20 +959,16 @@ static int abis_nm_rx_sw_act_req(struct msgb *mb) DEBUGP(DNM, "Found SW config: %s\n", hexdump(sw_config, sw_config_len)); } - if (sw_config[0] != NM_ATT_SW_DESCR) - DEBUGP(DNM, "SW_DESCR attribute identifier not found!\n"); - if (sw_config[1] != NM_ATT_FILE_ID) - DEBUGP(DNM, "FILE_ID attribute identifier not found!\n"); - file_id_len = sw_config[2] * 256 + sw_config[3]; + /* Use the first SW_DESCR present in SW config */ + sw_descr_len = abis_nm_parse_sw_descr(sw_config, sw_config_len); + if (sw_descr_len < 0) + return -EINVAL; - /* Assumes first SW file in list is the one to be activated */ - /* sw_config + 4 to skip over 2 attribute ID bytes and 16-bit length field */ return ipacc_sw_activate(mb->trx->bts, foh->obj_class, foh->obj_inst.bts_nr, foh->obj_inst.trx_nr, foh->obj_inst.ts_nr, - sw_config + 4, - file_id_len); + sw_config, sw_descr_len); } /* Receive a CHANGE_ADM_STATE_ACK, parse the TLV and update local state */ -- 1.6.4
Hi Sylvain, thanks for your patch. On Sun, Oct 25, 2009 at 05:56:44PM +0100, Sylvain Munaut wrote:
The previous code only sent the FILE_ID tag data part, but according to the GSM 12.21 spec, section 8.3.6, the full SW Description 'object' must be sent so that includes the NM_ATT_SW_DESCR tag, the whole FILE_ID and the whole FILE_VERSION (including tags & length fields).
Can somebody test this with a BS11? I want to make sure we don't introduce regressions here. -- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
The previous code only sent the FILE_ID tag data part,
but according to the GSM 12.21 spec, section 8.3.6, the full SW Description 'object' must be sent so that includes the NM_ATT_SW_DESCR tag, the whole FILE_ID and the whole FILE_VERSION (including tags & length fields).
Can somebody test this with a BS11? I want to make sure we don't introduce regressions here.
Did anyone ever had the opportunity to test this with a BS-11 ? Sylvain
Thanks, applied. For the future, please don't mix two things in one patch. In addition to splitting the function, you have also changed the variables, i.e. no longer using 'pos' but the offset. That should have been a separate patch and made review much easier. Regards, -- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
Thanks, applied. -- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
Thanks, I had already fixed this, but apparently not in the master branch yet. The correct fix is to use trx->bts, rather than bts. Skipping/removing patch_nm_tables() can cause erroneous data from the templates to leak into the live system without reflecting the configuration file. I'll fix it in master right now. -- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
participants (2)
-
Harald Welte -
Sylvain Munaut