neels has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-pfcp/+/29042 ) Change subject: gtlv: fix repeated IEIs to several struct members ...................................................................... gtlv: fix repeated IEIs to several struct members Coverity Scan has brought my attention to a problem with decoding repeated IEIs, where there are multiple struct members in the decoded struct that these are decoded to. Before this patch, gtlv aborts with an error as soon as the first struct member for a given tag is full, not parsing following IEIs into subsequent struct members. After this patch, gtlv continues to look whether subsequent entries in the message coding also decode the same tag, but to a different struct member. First commit without changing the gtlv regression test, to show that all current tests still succeed. The test for this particular issue follow in I994d0fb1f1435d2c27a8630a43fe106652ac6e41 Related: CID#275415 Related: SYS#5599 Change-Id: Ie37585178ff27306d425b75d8e407b71f92f1cdc --- M src/libosmo-gtlv/gtlv_dec_enc.c 1 file changed, 70 insertions(+), 20 deletions(-) diff --git a/src/libosmo-gtlv/gtlv_dec_enc.c b/src/libosmo-gtlv/gtlv_dec_enc.c index 58c0405..385ebfa 100644 --- a/src/libosmo-gtlv/gtlv_dec_enc.c +++ b/src/libosmo-gtlv/gtlv_dec_enc.c @@ -127,16 +127,19 @@ * that can store them are filled up. */ ie_max_allowed_count = 0; + /* Unordered: for every tag, look for matching IE definitions from the start. */ + iec = ie_coding; + /* There may be multiple subsequent occurences of the same tag, to be decoded into multiple members of + * the decoded struct. Iterate until encountering a still "free slot" for decoding. */ do { /* Find the IE coding for this tag */ - for (iec = ie_coding; - !osmo_gtlv_coding_end(iec) && osmo_gtlv_tag_inst_cmp(&iec->ti, >lv->ti); + for (; !osmo_gtlv_coding_end(iec) && osmo_gtlv_tag_inst_cmp(&iec->ti, >lv->ti); iec++); /* No such IE coding found. */ if (osmo_gtlv_coding_end(iec)) break; - /* Keep track how often this tag can occur */ + /* Keep track how often this tag can occur in total */ ie_max_allowed_count += iec->has_count ? iec->count_max : 1; /* Was this iec instance already decoded? Then skip to the next one, if any. */ @@ -145,23 +148,35 @@ multi_count_p = iec->has_count ? membof(obj, obj_maxlen, iec->count_ofs) : NULL; if ((presence_flag_p && *presence_flag_p) - || (multi_count_p && *multi_count_p >= iec->count_max)) + || (multi_count_p && *multi_count_p >= iec->count_max)) { + iec++; continue; + } + /* For IEs with a presence flag or a multi count, the decoded struct provides the information * whether the IE has already been decoded. Do the same for mandatory IEs, using local state in * seen_ie_coding_entries[]. */ - CHECK_SEEN(iec); - if (*seen_p) - continue; - } while (0); + if (!presence_flag_p && !multi_count_p) { + CHECK_SEEN(iec); + if (*seen_p) { + iec++; + continue; + } + } + + /* Found an IE coding that has not yet been decoded / still has unused struct members to decode + * to. */ + break; + } while (1); + if (osmo_gtlv_coding_end(iec)) { if (ie_max_allowed_count) { - /* There have been IE definitions for this IEI, but all slots to decode it are already - * filled. */ + /* This tag exists in the protocol definitions, but we've run out of struct members to + * decode the tag to. */ RETURN_ERROR(-ENOTSUP, gtlv->ti, "Only %u instances of this IE are supported per message", ie_max_allowed_count); } - /* No such IE defined in ie_coding, just skip the TLV. */ + /* This is an unknown tag, no such IEI defined in ie_coding, just skip the TLV. */ continue; } @@ -264,14 +279,22 @@ osmo_gtlv_load_start(gtlv); + /* Ordered: traverse the ie_coding exactly once, expecting tags to be decoded to appear in the same order. */ for (; !osmo_gtlv_coding_end(ie_coding); ie_coding++) { int rc; - bool *presence_flag = ie_coding->has_presence_flag ? - membof(obj, obj_maxlen, ie_coding->presence_flag_ofs) : NULL; - unsigned int *multi_count = ie_coding->has_count ? - membof(obj, obj_maxlen, ie_coding->count_ofs) : NULL; + bool *presence_flag; + unsigned int *multi_count; struct osmo_gtlv_tag_inst peek_ti; +#define UPDATE_STATE_FROM_IE_CODING() do { \ + presence_flag = ie_coding->has_presence_flag ? \ + membof(obj, obj_maxlen, ie_coding->presence_flag_ofs) : NULL; \ + multi_count = ie_coding->has_count ? \ + membof(obj, obj_maxlen, ie_coding->count_ofs) : NULL; \ + } while (0) + + UPDATE_STATE_FROM_IE_CODING(); + rc = osmo_gtlv_load_next_by_tag_inst(gtlv, &ie_coding->ti); switch (rc) { case 0: @@ -289,12 +312,39 @@ for (;;) { /* If this is a repeated IE, decode into the correct array index memb[idx], * next idx == (*multi_count) */ - unsigned int memb_next_array_idx = multi_count ? *multi_count : 0; - unsigned int memb_ofs = ie_coding->memb_ofs + memb_next_array_idx * ie_coding->memb_array_pitch; + unsigned int memb_next_array_idx; + unsigned int memb_ofs; - if (multi_count && memb_next_array_idx >= ie_coding->count_max) - RETURN_ERROR(-ENOTSUP, ie_coding->ti, "Only %u instances of this IE are supported per message", - ie_coding->count_max); +#define UPDATE_MEMB_IDX() do { \ + memb_next_array_idx = multi_count ? *multi_count : 0; \ + memb_ofs = ie_coding->memb_ofs + memb_next_array_idx * ie_coding->memb_array_pitch; \ + } while (0) + + UPDATE_MEMB_IDX(); + + if (multi_count && memb_next_array_idx >= ie_coding->count_max) { + /* This repeated IE is full. But the protocol may define this same IEI to follow again, + * with a different target struct member. See if this IEI appears again. */ + const struct osmo_gtlv_coding *iec = ie_coding; + bool iei_appears_again = false; + for (iec = ie_coding + 1; !osmo_gtlv_coding_end(iec); iec++) { + if (osmo_gtlv_tag_inst_cmp(>lv->ti, &iec->ti) == 0) { + iei_appears_again = true; + break; + } + } + + if (!iei_appears_again) + RETURN_ERROR(-ENOTSUP, ie_coding->ti, "Only %u instances of this IE are supported per message", + ie_coding->count_max); + + /* IEI does appear again. Skip forward to this ie_coding, and use that to decode the + * current TLV (do not call osmo_gtlv_load_next_by_tag_inst() again, or we'd skip the + * TLV in the message being decoded). */ + ie_coding = iec; + UPDATE_STATE_FROM_IE_CODING(); + UPDATE_MEMB_IDX(); + } /* Decode IE value part */ if (ie_coding->nested_ies) { null-- To view, visit https://gerrit.osmocom.org/c/libosmo-pfcp/+/29042 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-pfcp Gerrit-Branch: master Gerrit-Change-Id: Ie37585178ff27306d425b75d8e407b71f92f1cdc Gerrit-Change-Number: 29042 Gerrit-PatchSet: 5 Gerrit-Owner: neels <nhofmeyr@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: pespin <pespin@sysmocom.de> Gerrit-MessageType: merged