From d094320e24e089a139f6ddddb14c7b836d8054a4 Mon Sep 17 00:00:00 2001 From: erj Date: Thu, 21 Nov 2019 19:36:11 +0000 Subject: [PATCH] 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 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 Submitted by: Jacob Keller Reviewed by: asomers@, erj@ MFC after: 1 week Sponsored by: Intel Corporation Differential Revision: https://reviews.freebsd.org/D22398 --- sys/sys/bitstring.h | 10 ++++++++++ tests/sys/sys/bitstring_test.c | 22 ++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/sys/sys/bitstring.h b/sys/sys/bitstring.h index f15c34e61de3..a8c8a4e2a396 100644 --- a/sys/sys/bitstring.h +++ b/sys/sys/bitstring.h @@ -202,6 +202,11 @@ bit_ffs_at(bitstr_t *_bitstr, int _start, int _nbits, int *_result) 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); @@ -231,6 +236,11 @@ bit_ffc_at(bitstr_t *_bitstr, int _start, int _nbits, int *_result) 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); diff --git a/tests/sys/sys/bitstring_test.c b/tests/sys/sys/bitstring_test.c index fbfe78275b23..66cb8b559b1a 100644 --- a/tests/sys/sys/bitstring_test.c +++ b/tests/sys/sys/bitstring_test.c @@ -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)