| Message ID | 1458141243-8236-1-git-send-email-axilirator@gmail.com |
|---|---|
| State | New |
| Headers | show |
Hi Vadim, I've merged both of your patches, thanks! If you have time, please check if there are other occurrences in OpenBSC or OsmocomBB where the TMSI is printed as integer. Thanks! Regards, Harald
> On 17 Mar 2016, at 14:17, Harald Welte <laforge@gnumonks.org> wrote: > > Hi Vadim, Hi Guys, > I've merged both of your patches, thanks! I think it was a bit too quick. I foresee one problem. Let's assume someone is using TMSIs and now upgrade the sourcecode. All CM Service Requests will fail because the TMSI is not known. What is the migration/mitigation plan? -#define tmsi_from_string(str) strtoul(str, NULL, 10) +#define tmsi_from_string(str) strtoul(str + 2, NULL, 16) is problematic too: 1.) E.g. gsm48_mi_to_string(mi_string, sizeof(mi_string), idi->mi, idi->mi_len); ... subscr = subscr_get_by_tmsi(bts->network->subscr_group, tmsi_from_string(mi_string)); tmsi_from_string will not work for this anymore. 2.) there is no length check but that doesn't seem to be a big issue right now. * I think the above hunk should be reverted * a DB schema upgrade and store the TMSI as uint32_t * Use hex presentation in VTY What do you think?
Hi Holger, On Thu, Mar 17, 2016 at 02:29:33PM +0100, Holger Freyther wrote: > I think it was a bit too quick. I foresee one problem. Let's assume > someone is using TMSIs and now upgrade the sourcecode. All CM Service > Requests will fail because the TMSI is not known. What is the > migration/mitigation plan? ugh. I failed to see that for some strange reason we store the TMSI as "TEXT" in the database. That's really odd, especailly as we use %u formatting when searching for TMSIs in the database. > * I think the above hunk should be reverted done. > * a DB schema upgrade and store the TMSI as uint32_t correct.
Hi Guys! > If you have time, please check if there are other occurrences in OpenBSC > or OsmocomBB where the TMSI is printed as integer. Thanks! No problem! :) > I think it was a bit too quick. I foresee one problem. Let's assume someone > is using TMSIs and now upgrade the sourcecode. All CM Service Requests > will fail because the TMSI is not known. What is the migration/mitigation plan? I absolutely agree with Holger. Maybe we can add some code that will check if database still stores TMSIs in old representation style and convert them to uint32_t? We can change the libmsc/db.c:db_prepare() for this purpose. > tmsi_from_string will not work for this anymore. Yes, I forgot to change the gsm_subscriber.c ... Sorry. > there is no length check but that doesn't seem to be a big issue right now. We can just write a function that will do this check instead of using #define. > * a DB schema upgrade and store the TMSI as uint32_t > * Use hex presentation in VTY +1 С наилучшими пожеланиями, Яницкий Вадим. 2016-03-17 19:43 GMT+06:00 Harald Welte <laforge@gnumonks.org>: > Hi Holger, > > On Thu, Mar 17, 2016 at 02:29:33PM +0100, Holger Freyther wrote: > > I think it was a bit too quick. I foresee one problem. Let's assume > > someone is using TMSIs and now upgrade the sourcecode. All CM Service > > Requests will fail because the TMSI is not known. What is the > > migration/mitigation plan? > > ugh. I failed to see that for some strange reason we store the TMSI as > "TEXT" in the database. That's really odd, especailly as we use %u > formatting when searching for TMSIs in the database. > > > * I think the above hunk should be reverted > > done. > > > * a DB schema upgrade and store the TMSI as uint32_t > > correct. > > -- > - 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) >
> On 17 Mar 2016, at 17:21, Вадим Яницкий <axilirator@gmail.com> wrote: > > Hi Guys! > > > If you have time, please check if there are other occurrences in OpenBSC > > or OsmocomBB where the TMSI is printed as integer. Thanks! > > No problem! :) > > > I think it was a bit too quick. I foresee one problem. Let's assume someone > > is using TMSIs and now upgrade the sourcecode. All CM Service Requests > > will fail because the TMSI is not known. What is the migration/mitigation plan? > > I absolutely agree with Holger. Maybe we can add some code that will check > if database still stores TMSIs in old representation style and convert them to > uint32_t? We can change the libmsc/db.c:db_prepare() for this purpose. yes, we have a schema version and can just increase it. E.g. have a look at how we migrate SMS. > > > tmsi_from_string will not work for this anymore. > > Yes, I forgot to change the gsm_subscriber.c ... Sorry. it happens. thanks for contributing > > > there is no length check but that doesn't seem to be a big issue right now. > > We can just write a function that will do this check instead of using #define. I think you will not need to touch this define at all. We might want to change the name to _from_mi_string. > > > * a DB schema upgrade and store the TMSI as uint32_t > > * Use hex presentation in VTY > > +1 looking forward for the follow up. holger
I just fixed some things (gsm_subscriber.c, VTY tmsi parsing) and did some tests. It works! Location Update Request with TMSI from previous network: <0002> gsm_04_08.c:1127 LOCATION UPDATING REQUEST: MI(TMSI)=0x68044a6c type=NORMAL <0001> gsm_04_08.c:144 (bts 0 trx 0 ts 0 pd 05) Sending 0x18 to MS. <0001> gsm_04_08.c:144 (bts 0 trx 0 ts 0 pd 05) Sending 0x18 to MS. Outgoing SMS from OpenBSC VTY: OpenBSC> subscriber tmsi 0xb7f861b6 sms sender tmsi 0xb7f861b6 send TEST <0002> gsm_subscriber.c:175 Subscriber <IMSI> not paged yet. <0004> abis_rsl.c:1465 (bts=0,trx=0,ts=0,ss=0) Activating ARFCN(***) SS(0) lctype SDCCH r=OTHER ra=0x1a ta=1 <0004> abis_rsl.c:1199 (bts=0,trx=0,ts=0,ss=0) CHANNEL ACTIVATE ACK <0000> abis_rsl.c:1653 (bts=0,trx=0,ts=0,ss=0) SAPI=0 ESTABLISH INDICATION <0000> gsm_04_08.c:3573 Dispatching 04.08 message, pdisc=6 <0003> gsm_04_08.c:1180 PAGING RESPONSE: MI(TMSI)=0xb7f861b6 <0003> gsm_04_08.c:1198 <- Channel was requested by <IMSI> <0003> gsm_04_08.c:1259 TX APPLICATION INFO id=0x00, len=4 <0001> gsm_04_08.c:144 (bts 0 trx 0 ts 0 pd 06) Sending 0x38 to MS. <0001> transaction.c:71 subscr=0x2363f00, net=0x2333e90 USSD request also works: <0002> gsm_04_08.c:956 <- CM SERVICE REQUEST serv_type=0x08 MI(TMSI)=0xb7f861b6 <0002> gsm_04_08_utils.c:692 -> CM SERVICE ACK As you can see, now TMSI displays in hex. I'll provide a new patch soon. С наилучшими пожеланиями, Яницкий Вадим. 2016-03-22 18:58 GMT+06:00 Вадим Яницкий <axilirator@gmail.com>: > Hello, Harald! Good day, Holger! > > It looks like SQLite3 doesn't have support of unsigned 64-bit integers. :( > Of course, we can write some custom functions, which can emulate it, > but it isn't good solution, I think. > > My suggestion is to keep the TMSI column type in string format: 0xffffffff. > I need to know your opinions before starting to write a new patch. > > > С наилучшими пожеланиями, > Яницкий Вадим. > > 2016-03-17 22:34 GMT+06:00 Holger Freyther <holger@freyther.de>: > >> >> > On 17 Mar 2016, at 17:21, Вадим Яницкий <axilirator@gmail.com> wrote: >> > >> > Hi Guys! >> > >> > > If you have time, please check if there are other occurrences in >> OpenBSC >> > > or OsmocomBB where the TMSI is printed as integer. Thanks! >> > >> > No problem! :) >> > >> > > I think it was a bit too quick. I foresee one problem. Let's assume >> someone >> > > is using TMSIs and now upgrade the sourcecode. All CM Service Requests >> > > will fail because the TMSI is not known. What is the >> migration/mitigation plan? >> > >> > I absolutely agree with Holger. Maybe we can add some code that will >> check >> > if database still stores TMSIs in old representation style and convert >> them to >> > uint32_t? We can change the libmsc/db.c:db_prepare() for this purpose. >> >> >> yes, we have a schema version and can just increase it. E.g. have a look >> at how we migrate SMS. >> >> >> >> > >> > > tmsi_from_string will not work for this anymore. >> > >> > Yes, I forgot to change the gsm_subscriber.c ... Sorry. >> >> >> it happens. thanks for contributing >> >> >> >> > >> > > there is no length check but that doesn't seem to be a big issue >> right now. >> > >> > We can just write a function that will do this check instead of using >> #define. >> >> I think you will not need to touch this define at all. We might want to >> change the name to _from_mi_string. >> >> >> > >> > > * a DB schema upgrade and store the TMSI as uint32_t >> > > * Use hex presentation in VTY >> > >> > +1 >> >> >> looking forward for the follow up. >> >> >> holger > > >
diff --git a/openbsc/include/openbsc/gsm_subscriber.h b/openbsc/include/openbsc/gsm_subscriber.h index 7d6c776..785dc36 100644 --- a/openbsc/include/openbsc/gsm_subscriber.h +++ b/openbsc/include/openbsc/gsm_subscriber.h @@ -14,7 +14,7 @@ #define GSM_SUBSCRIBER_FIRST_CONTACT 0x00000001 /* gprs_sgsn.h defines additional flags including and above bit 16 (0x10000) */ -#define tmsi_from_string(str) strtoul(str, NULL, 10) +#define tmsi_from_string(str) strtoul(str + 2, NULL, 16) #define GSM_SUBSCRIBER_NO_EXPIRATION 0x0 diff --git a/openbsc/src/libmsc/db.c b/openbsc/src/libmsc/db.c index 0935fc5..952151e 100644 --- a/openbsc/src/libmsc/db.c +++ b/openbsc/src/libmsc/db.c @@ -893,9 +893,10 @@ struct gsm_subscriber *db_get_subscriber(enum gsm_subscriber_field field, subscr->id = dbi_result_get_ulonglong(result, "id"); db_set_from_query(subscr, result); - DEBUGP(DDB, "Found Subscriber: ID %llu, IMSI %s, NAME '%s', TMSI %u, EXTEN '%s', LAC %hu, AUTH %u\n", - subscr->id, subscr->imsi, subscr->name, subscr->tmsi, subscr->extension, - subscr->lac, subscr->authorized); + DEBUGP(DDB, "Found Subscriber: ID %llu, IMSI %s, NAME '%s', " + "TMSI 0x%08x, EXTEN '%s', LAC %hu, AUTH %u\n", + subscr->id, subscr->imsi, subscr->name, subscr->tmsi, + subscr->extension, subscr->lac, subscr->authorized); dbi_result_free(result); get_equipment_by_subscr(subscr); @@ -935,7 +936,7 @@ int db_subscriber_update(struct gsm_subscriber *subscr) int db_sync_subscriber(struct gsm_subscriber *subscriber) { dbi_result result; - char tmsi[14]; + char tmsi[11]; char *q_tmsi, *q_name, *q_extension; dbi_conn_quote_string_copy(conn, @@ -944,7 +945,7 @@ int db_sync_subscriber(struct gsm_subscriber *subscriber) subscriber->extension, &q_extension); if (subscriber->tmsi != GSM_RESERVED_TMSI) { - sprintf(tmsi, "%u", subscriber->tmsi); + sprintf(tmsi, "0x%08x", subscriber->tmsi); dbi_conn_quote_string_copy(conn, tmsi, &q_tmsi); @@ -1194,7 +1195,7 @@ int db_subscriber_expire(void *priv, void (*callback)(void *priv, long long unsi int db_subscriber_alloc_tmsi(struct gsm_subscriber *subscriber) { dbi_result result = NULL; - char tmsi[14]; + char tmsi[11]; char *tmsi_quoted; for (;;) { @@ -1205,7 +1206,7 @@ int db_subscriber_alloc_tmsi(struct gsm_subscriber *subscriber) if (subscriber->tmsi == GSM_RESERVED_TMSI) continue; - sprintf(tmsi, "%u", subscriber->tmsi); + sprintf(tmsi, "0x%08x", subscriber->tmsi); dbi_conn_quote_string_copy(conn, tmsi, &tmsi_quoted); result = dbi_conn_queryf(conn, "SELECT * FROM Subscriber " diff --git a/openbsc/tests/db/db_test.c b/openbsc/tests/db/db_test.c index a02d1f8..faea820 100644 --- a/openbsc/tests/db/db_test.c +++ b/openbsc/tests/db/db_test.c @@ -200,7 +200,7 @@ int main() alice->lac=42; db_sync_subscriber(alice); /* Get by TMSI */ - snprintf(scratch_str, sizeof(scratch_str), "%"PRIu32, alice->tmsi); + snprintf(scratch_str, sizeof(scratch_str), "0x%08x", alice->tmsi); alice_db = db_get_subscriber(GSM_SUBSCRIBER_TMSI, scratch_str); COMPARE(alice, alice_db); SUBSCR_PUT(alice_db); @@ -227,7 +227,7 @@ int main() db_subscriber_assoc_imei(alice, "1234567890"); db_subscriber_assoc_imei(alice, "6543560920"); /* Get by TMSI */ - snprintf(scratch_str, sizeof(scratch_str), "%"PRIu32, alice->tmsi); + snprintf(scratch_str, sizeof(scratch_str), "0x%08x", alice->tmsi); alice_db = db_get_subscriber(GSM_SUBSCRIBER_TMSI, scratch_str); COMPARE(alice, alice_db); SUBSCR_PUT(alice_db); diff --git a/openbsc/tests/gsm0408/gsm0408_test.c b/openbsc/tests/gsm0408/gsm0408_test.c index 781ef61..8ed57ca 100644 --- a/openbsc/tests/gsm0408/gsm0408_test.c +++ b/openbsc/tests/gsm0408/gsm0408_test.c @@ -93,7 +93,7 @@ static void test_mi_functionality(void) /* tmsi code */ mi_len = gsm48_generate_mid_from_tmsi(mi, tmsi); gsm48_mi_to_string(mi_parsed, sizeof(mi_parsed), mi + 2, mi_len - 2); - COMPARE((uint32_t)strtoul(mi_parsed, NULL, 10), ==, tmsi); + COMPARE((uint32_t)tmsi_from_string(mi_parsed), ==, tmsi); /* imsi code */ mi_len = gsm48_generate_mid_from_imsi(mi, imsi_odd);