[MERGED] openbsc[master]: Fix BTS features length check
Max has submitted this change and it was merged. Change subject: Fix BTS features length check .. Fix BTS features length check While fixing potentially incorrect memory access, the check for maximum number of supported BTS features was incorrectly adjusted instead of feature vectore length check next to it. Fix this by adjusting checks properly and adding comments to avoid future confusion. The error was introduced in a60bb3dd28ce9e3720f8ee1b262893f3e233e2e6. Change-Id: I06d2498d730624d5da535f6add6fa98d004714ae --- M openbsc/src/libbsc/abis_nm.c 1 file changed, 4 insertions(+), 4 deletions(-) Approvals: Harald Welte: Looks good to me, approved Jenkins Builder: Verified diff --git a/openbsc/src/libbsc/abis_nm.c b/openbsc/src/libbsc/abis_nm.c index 1715688..019d039 100644 --- a/openbsc/src/libbsc/abis_nm.c +++ b/openbsc/src/libbsc/abis_nm.c @@ -484,13 +484,13 @@ if (TLVP_PRES_LEN(, NM_ATT_MANUF_ID, 2)) { m_id_len = TLVP_LEN(, NM_ATT_MANUF_ID); - if (m_id_len > MAX_BTS_FEATURES/8 + 1) { + /* log potential BTS feature vector overflow */ + if (m_id_len > sizeof(bts->_features_data)) LOGP(DNM, LOGL_NOTICE, "BTS%u Get Attributes Response: feature vector is truncated to %u bytes\n", bts->nr, MAX_BTS_FEATURES/8); - m_id_len = MAX_BTS_FEATURES/8; - } - if (m_id_len > sizeof(bts->_features_data)) + /* check that max. expected BTS attribute is above given feature vector length */ + if (m_id_len > OSMO_BYTES_FOR_BITS(_NUM_BTS_FEAT)) LOGP(DNM, LOGL_NOTICE, "BTS%u Get Attributes Response: reported unexpectedly long (%u bytes) " "feature vector - most likely it was compiled against newer BSC headers. " "Consider upgrading your BSC to later version.\n", -- To view, visit https://gerrit.osmocom.org/2900 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: I06d2498d730624d5da535f6add6fa98d004714ae Gerrit-PatchSet: 3 Gerrit-Project: openbsc Gerrit-Branch: master Gerrit-Owner: MaxGerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Max Gerrit-Reviewer: Neels Hofmeyr
openbsc[master]: Fix BTS features length check
Patch Set 2: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/2900 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I06d2498d730624d5da535f6add6fa98d004714ae Gerrit-PatchSet: 2 Gerrit-Project: openbsc Gerrit-Branch: master Gerrit-Owner: MaxGerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr Gerrit-HasComments: No
openbsc[master]: Fix BTS features length check
Patch Set 1: (1 comment) https://gerrit.osmocom.org/#/c/2900/1/openbsc/src/libbsc/abis_nm.c File openbsc/src/libbsc/abis_nm.c: Line 493: if (m_id_len > _NUM_BTS_FEAT/8 + 1) > shouldn't this be looking at enum gsm_bts_features that has _NUM_BTS_FEAT as last enum entry the /8 seems to be wrong entirely, and it seems that you need (m_id_len > _NUM_BTS_FEAT)? -- To view, visit https://gerrit.osmocom.org/2900 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I06d2498d730624d5da535f6add6fa98d004714ae Gerrit-PatchSet: 1 Gerrit-Project: openbsc Gerrit-Branch: master Gerrit-Owner: MaxGerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr Gerrit-HasComments: Yes
openbsc[master]: Fix BTS features length check
Patch Set 1: Code-Review-1 (1 comment) https://gerrit.osmocom.org/#/c/2900/1/openbsc/src/libbsc/abis_nm.c File openbsc/src/libbsc/abis_nm.c: Line 493: if (m_id_len > _NUM_BTS_FEAT/8 + 1) shouldn't this be if (m_id_len > _NUM_BTS_FEAT/8) ? This patch allows m_id_len to be one larger than _NUM_BTS_FEAT/8. Should the /8 also get a comment? "NUM" sounds like an actual amount; in contrast, division by 8 seems to be some buffer size with each feature taking 8 bytes? Just asking because I'm not familiar... -- To view, visit https://gerrit.osmocom.org/2900 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I06d2498d730624d5da535f6add6fa98d004714ae Gerrit-PatchSet: 1 Gerrit-Project: openbsc Gerrit-Branch: master Gerrit-Owner: MaxGerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr Gerrit-HasComments: Yes
[PATCH] openbsc[master]: Fix BTS features length check
Review at https://gerrit.osmocom.org/2900 Fix BTS features length check While fixing potentially incorrect memory access, the check for maximum number of supported BTS features was incorrectly adjusted instead of feature vectore length check next to it. Fix this by adjusting checks properly and adding comments to avoid future confusion. The error was introduced in a60bb3dd28ce9e3720f8ee1b262893f3e233e2e6. Change-Id: I06d2498d730624d5da535f6add6fa98d004714ae --- M openbsc/src/libbsc/abis_nm.c 1 file changed, 4 insertions(+), 4 deletions(-) git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/00/2900/1 diff --git a/openbsc/src/libbsc/abis_nm.c b/openbsc/src/libbsc/abis_nm.c index 1715688..b0f3428 100644 --- a/openbsc/src/libbsc/abis_nm.c +++ b/openbsc/src/libbsc/abis_nm.c @@ -484,13 +484,13 @@ if (TLVP_PRES_LEN(, NM_ATT_MANUF_ID, 2)) { m_id_len = TLVP_LEN(, NM_ATT_MANUF_ID); - if (m_id_len > MAX_BTS_FEATURES/8 + 1) { + /* log potential BTS feature vector overflow */ + if (m_id_len > sizeof(bts->_features_data)) LOGP(DNM, LOGL_NOTICE, "BTS%u Get Attributes Response: feature vector is truncated to %u bytes\n", bts->nr, MAX_BTS_FEATURES/8); - m_id_len = MAX_BTS_FEATURES/8; - } - if (m_id_len > sizeof(bts->_features_data)) + /* check that max. expected BTS attribute is above given feature vector length */ + if (m_id_len > _NUM_BTS_FEAT/8 + 1) LOGP(DNM, LOGL_NOTICE, "BTS%u Get Attributes Response: reported unexpectedly long (%u bytes) " "feature vector - most likely it was compiled against newer BSC headers. " "Consider upgrading your BSC to later version.\n", -- To view, visit https://gerrit.osmocom.org/2900 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I06d2498d730624d5da535f6add6fa98d004714ae Gerrit-PatchSet: 1 Gerrit-Project: openbsc Gerrit-Branch: master Gerrit-Owner: Max