Attention is currently required from: pespin. osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc-nat/+/28582 )
Change subject: bssap_conn: fix missing length check ...................................................................... Patch Set 2: (1 comment) File src/osmo-bsc-nat/bssap_conn.c: https://gerrit.osmocom.org/c/osmo-bsc-nat/+/28582/comment/97cfde7b_f706dd80 PS2, Line 60: len = IP_V4_ADDR_LEN; Thanks for the suggestion! > gsm0808_enc_aoip_trasp_addr should take care of checking internally whether > there's space in msg_new. I read related libosmocore code, and now understand that all msgb code works like this, using msgb_put(msg, len) to get a pointer to a buffer to write len bytes to, and then actually write it with e.g. memcpy afterwards. msgb_put already checks that enough space is available: https://gitea.osmocom.org/osmocom/libosmocore/src/commit/0578c288ec9e33512cfcfa0f83f669ca98a469e6/include/osmocom/core/msgb.h#L237 So that's how the check for gsm0808_enc_aoip_trasp_addr is performed. tlv_encode_one is also using msgb functions so the length check should also work there. I think CID#273004 is a false positive now, it seems coverity can't follow that msgb_tvlv_put() would already check the length through msgb_put(msg, TVLV_GROSS_LEN(len)) correctly before writing to it. Do you agree that it's a false positive? Regarding whether to check length here: I think we should either check the length for both cases (copy IE over and replacing it) like it's done in the current version of the patch, or not check the length at all and rely on msgb_put to do it. By adding the check here we could avoid osmo_panic() if there's not enough space, but it makes the code here more complicated. Probably not worth it? -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc-nat/+/28582 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc-nat Gerrit-Branch: master Gerrit-Change-Id: I1fc4c81e139bab3d7d977ef9467f62d8088884db Gerrit-Change-Number: 28582 Gerrit-PatchSet: 2 Gerrit-Owner: osmith <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Comment-Date: Wed, 13 Jul 2022 09:13:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: osmith <[email protected]> Comment-In-Reply-To: pespin <[email protected]> Gerrit-MessageType: comment
