Change in libosmocore[master]: bitvec_read_field(): fix incorrect bit-shift issue found by UBSan

[fixeria]

fixeria has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/26308 )

Change subject: bitvec_read_field(): fix incorrect bit-shift issue found by UBSan
......................................................................

bitvec_read_field(): fix incorrect bit-shift issue found by UBSan

While running a sanitized version of the bitvec_test I get:

  bitvec.c:492:24: runtime error: shift exponent 64 is too large
                                  for 64-bit type 'long unsigned int'

This error is triggered by the following line in the bitvec_test:

  _bitvec_read_field(0, 8 * 8 + 1); /* too many bits */

which basically tries to parse more bits (65) than the test vector
actually has (64).  The problem is that we don't check if the
given vector has enough data *before* entering the parsing loop,
so we end up doing weird bit-shifts and getting weird values:

  bitvec_read_field(idx=0, len=65) => bd5b7ddffdd7b5db (error)

Unfortunately, this problem remained unnoticed so far because in
'tests/testsuite.at' we don't check if stderr is empty.  This is
fixed in a follow up change [1].

Rather than checking for errors in every loop iteration, do this
once and return early if the overrun is possible with the given
offset and length arguments.

Change-Id: I4deeabba7ebb720cdbe7c85b37bc011d05bdfa65
Related: [1] Ia82b92eddb18dc596881abcef2f098dc7385538b
---
M src/bitvec.c
M tests/bitvec/bitvec_test.ok
2 files changed, 8 insertions(+), 5 deletions(-)

Approvals:
  laforge: Looks good to me, approved
  pespin: Looks good to me, but someone else must approve
  Jenkins Builder: Verified



diff --git a/src/bitvec.c b/src/bitvec.c
index b411a72..2303a0d 100644
--- a/src/bitvec.c
+++ b/src/bitvec.c
@@ -480,15 +480,18 @@
 {
 	unsigned int i;
 	uint64_t ui = 0;
+
+	/* Prevent bitvec overrun due to incorrect index and/or length */
+	if (len && bytenum_from_bitnum(*read_index + len - 1) >= bv->data_len) {
+		errno = EOVERFLOW;
+		return 0;
+	}
+
 	bv->cur_bit = *read_index;
 	errno = 0;
 
 	for (i = 0; i < len; i++) {
 		int bit = bitvec_get_bit_pos((const struct bitvec *)bv, bv->cur_bit);
-		if (bit < 0) {
-			errno = -bit;
-			break;
-		}
 		if (bit)
 			ui |= ((uint64_t)1 << (len - i - 1));
 		bv->cur_bit++;
diff --git a/tests/bitvec/bitvec_test.ok b/tests/bitvec/bitvec_test.ok
index a0e31d3..d87ac7e 100644
--- a/tests/bitvec/bitvec_test.ok
+++ b/tests/bitvec/bitvec_test.ok
@@ -185,7 +185,7 @@
 bitvec_read_field(idx=10, len=3) => 5 (success)
 bitvec_read_field(idx=10, len=1) => 1 (success)
 bitvec_read_field(idx=512, len=16) => 0 (error)
-bitvec_read_field(idx=0, len=65) => bd5b7ddffdd7b5db (error)
+bitvec_read_field(idx=0, len=65) => 0 (error)
 bitvec_read_field(idx=64, len=16) => 0 (error)
 
 bitvec ok.

-- 
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/26308
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I4deeabba7ebb720cdbe7c85b37bc011d05bdfa65
Gerrit-Change-Number: 26308
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20211118/7910c02d/attachment.htm>