svn commit: r354975 - in head: sys/sys tests/sys/sys

[Eric Joyner]

Author: erj
Date: Thu Nov 21 19:36:11 2019
New Revision: 354975
URL: https://svnweb.freebsd.org/changeset/base/354975

Log:
  bitstring: exit early if _start is past size of the bitstring
  
  bit_ffs_at and bit_ffc_at both take _start parameters which indicate to
  start searching from _start onwards.
  
  If the given _start index is past the size of the bit string, these
  functions will calculate an address of the current bitstring which is
  after the expected size. The function will also dereference the memory,
  resulting in a read buffer overflow.
  
  The output of the function remains correct, because the tests ensure to
  stop the loop if the current bitstring chunk passes the stop bitstring
  chunk, and because of a check to ensure the reported _value is never
  past _nbits.
  
  However, if <sys/bitstring.h> is ever used in code which is checked by
  -fsanitize=undefined, or similar static analysis, it can produce
  warnings about reading past the buffer size.
  
  Because of the above mentioned checks, these buffer overflows do not
  occur as long as _start is less than _nbits. Additionally, by definition
  bit_ffs_at and bif_ffc_at should set _result to -1 in any case where the
  _start is after the _nbits.
  
  Check for this case at the start of the function and exit early if so,
  preventing the buffer read overflow, and reducing the amount of
  computation that occurs.
  
  Note that it may seem odd to ever have code that could call bit_ffc_at
  or bit_ffs_at with a _start value greater than _nbits. However, consider
  a for-loop that used bit_ffs and bit_ffs_at to loop over a bit string
  and perform some operation on each bit that was set. If the last bit of
  the bit string was set, the simplest loop implementation would call
  bit_ffs_at with a start of _nbits, and expect that to return -1. While
  it does infact perform correctly, this is what ultimately triggers the
  unexpected buffer read overflow.
  
  Signed-off-by: Jacob Keller <jacob.e.keller at intel.com>
  
  Submitted by:	Jacob Keller <jacob.e.keller at intel.com>
  Reviewed by:	asomers@, erj@
  MFC after:	1 week
  Sponsored by:	Intel Corporation
  Differential Revision:	https://reviews.freebsd.org/D22398

Modified:
  head/sys/sys/bitstring.h
  head/tests/sys/sys/bitstring_test.c

Modified: head/sys/sys/bitstring.h
==============================================================================
--- head/sys/sys/bitstring.h	Thu Nov 21 19:30:31 2019	(r354974)
+++ head/sys/sys/bitstring.h	Thu Nov 21 19:36:11 2019	(r354975)
@@ -202,6 +202,11 @@ bit_ffs_at(bitstr_t *_bitstr, int _start, int _nbits, 
 	bitstr_t _test;
 	int _value, _offset;
 
+	if (_start >= _nbits) {
+		*_result = -1;
+		return;
+	}
+
 	if (_nbits > 0) {
 		_curbitstr = _bitstr + _bit_idx(_start);
 		_stopbitstr = _bitstr + _bit_idx(_nbits - 1);
@@ -230,6 +235,11 @@ bit_ffc_at(bitstr_t *_bitstr, int _start, int _nbits, 
 	bitstr_t *_stopbitstr;
 	bitstr_t _test;
 	int _value, _offset;
+
+	if (_start >= _nbits) {
+		*_result = -1;
+		return;
+	}
 
 	if (_nbits > 0) {
 		_curbitstr = _bitstr + _bit_idx(_start);

Modified: head/tests/sys/sys/bitstring_test.c
==============================================================================
--- head/tests/sys/sys/bitstring_test.c	Thu Nov 21 19:30:31 2019	(r354974)
+++ head/tests/sys/sys/bitstring_test.c	Thu Nov 21 19:36:11 2019	(r354975)
@@ -246,6 +246,17 @@ BITSTRING_TC_DEFINE(bit_ffs_at)
 			    nbits, memloc, i, found_set_bit);
 		}
 	}
+
+	/* Pass a start value beyond the size of the bit string */
+	bit_ffs_at(bitstr, nbits, nbits, &found_set_bit);
+	ATF_REQUIRE_MSG(found_set_bit == -1,
+			"bit_ffs_at_%d_%s: Failed with high start value of %d, Result %d",
+			nbits, memloc, nbits, found_set_bit);
+
+	bit_ffs_at(bitstr, nbits + 3, nbits, &found_set_bit);
+	ATF_REQUIRE_MSG(found_set_bit == -1,
+			"bit_ffs_at_%d_%s: Failed with high start value of %d, Result %d",
+			nbits, memloc, nbits + 3, found_set_bit);
 }
 
 BITSTRING_TC_DEFINE(bit_ffc_at)
@@ -297,6 +308,17 @@ BITSTRING_TC_DEFINE(bit_ffc_at)
 			    nbits, memloc, i, found_clear_bit);
 		}
 	}
+
+	/* Pass a start value beyond the size of the bit string */
+	bit_ffc_at(bitstr, nbits, nbits, &found_clear_bit);
+	ATF_REQUIRE_MSG(found_clear_bit == -1,
+			"bit_ffc_at_%d_%s: Failed with high start value, Result %d",
+			nbits, memloc, found_clear_bit);
+
+	bit_ffc_at(bitstr, nbits + 3, nbits, &found_clear_bit);
+	ATF_REQUIRE_MSG(found_clear_bit == -1,
+			"bit_ffc_at_%d_%s: Failed with high start value of %d, Result %d",
+			nbits, memloc, nbits + 3, found_clear_bit);
 }
 
 BITSTRING_TC_DEFINE(bit_nclear)