[PATCH] find: Do not read beyond variable boundaries on small sizes

* [PATCH] find: Do not read beyond variable boundaries on small sizes
@ 2021-12-03 10:08 Kees Cook
  2021-12-03 12:30 ` Andy Shevchenko
  2021-12-08 23:23 ` Rasmus Villemoes
  0 siblings, 2 replies; 15+ messages in thread
From: Kees Cook @ 2021-12-03 10:08 UTC (permalink / raw)
  To: Yury Norov
  Cc: Kees Cook, Andy Shevchenko, Rasmus Villemoes, Wolfram Sang,
	linux-kernel, linux-hardening

It's common practice to cast small variable arguments to the find_*_bit()
helpers to unsigned long and then use a size argument smaller than
sizeof(unsigned long):

	unsigned int bits;
	...
	out = find_first_bit((unsigned long *)&bits, 32);

This leads to the find helper dereferencing a full unsigned long,
regardless of the size of the actual variable. The unwanted bits
get masked away, but strictly speaking, a read beyond the end of
the target variable happens. Builds under -Warray-bounds complain
about this situation, for example:

In file included from ./include/linux/bitmap.h:9,
                 from drivers/iommu/intel/iommu.c:17:
drivers/iommu/intel/iommu.c: In function 'domain_context_mapping_one':
./include/linux/find.h:119:37: error: array subscript 'long unsigned int[0]' is partly outside array bounds of 'int[1]' [-Werror=array-bounds]
  119 |                 unsigned long val = *addr & GENMASK(size - 1, 0);
      |                                     ^~~~~
drivers/iommu/intel/iommu.c:2115:18: note: while referencing 'max_pde'
 2115 |         int pds, max_pde;
      |                  ^~~~~~~

Instead, just carefully read the correct variable size, all of which
happens at compile time since small_const_nbits(size) has already
determined that arguments are constant expressions.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/find.h | 62 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 55 insertions(+), 7 deletions(-)

diff --git a/include/linux/find.h b/include/linux/find.h
index 5bb6db213bcb..5708d188b9cb 100644
--- a/include/linux/find.h
+++ b/include/linux/find.h
@@ -17,6 +17,41 @@ extern unsigned long _find_first_and_bit(const unsigned long *addr1,
 extern unsigned long _find_first_zero_bit(const unsigned long *addr, unsigned long size);
 extern unsigned long _find_last_bit(const unsigned long *addr, unsigned long size);
 
+static __always_inline
+unsigned long __find_bits_deref(const void *addr, unsigned long size)
+{
+	unsigned long val;
+
+	BUILD_BUG_ON(!small_const_nbits(size));
+	/*
+	 * This part of the routine will be evaluated at
+	 * compile-time (due to small_const_nbits()), and only
+	 * for values of "size" <= sizeof(unsigned long). As
+	 * such, the compiler can see when the dereference of
+	 * "addr" may be reading past the end of the variable
+	 * (when it is smaller than unsigned long). While the
+	 * GENMASK will clobber those bits for no exposure, it
+	 * is still technically an OOB read. Instead, pick a
+	 * better dereference width to avoid it entirely.
+	 */
+	switch (size) {
+	case 0 ... 8:
+		val = *(const unsigned char *)addr;
+		break;
+	case 9 ... 16:
+		val = *(const unsigned short *)addr;
+		break;
+	case 17 ... 32:
+		val = *(const unsigned int *)addr;
+		break;
+	default:
+		val = *(const unsigned long *)addr;
+		break;
+	}
+
+	return val;
+}
+
 #ifndef find_next_bit
 /**
  * find_next_bit - find the next set bit in a memory region
@@ -37,7 +72,8 @@ unsigned long find_next_bit(const unsigned long *addr, unsigned long size,
 		if (unlikely(offset >= size))
 			return size;
 
-		val = *addr & GENMASK(size - 1, offset);
+		val =  __find_bits_deref(addr, size);
+		val &= GENMASK(size - 1, offset);
 		return val ? __ffs(val) : size;
 	}
 
@@ -67,7 +103,9 @@ unsigned long find_next_and_bit(const unsigned long *addr1,
 		if (unlikely(offset >= size))
 			return size;
 
-		val = *addr1 & *addr2 & GENMASK(size - 1, offset);
+		val =  __find_bits_deref(addr1, size);
+		val &= __find_bits_deref(addr2, size);
+		val &= GENMASK(size - 1, offset);
 		return val ? __ffs(val) : size;
 	}
 
@@ -95,7 +133,8 @@ unsigned long find_next_zero_bit(const unsigned long *addr, unsigned long size,
 		if (unlikely(offset >= size))
 			return size;
 
-		val = *addr | ~GENMASK(size - 1, offset);
+		val =  __find_bits_deref(addr, size);
+		val |= ~GENMASK(size - 1, offset);
 		return val == ~0UL ? size : ffz(val);
 	}
 
@@ -116,8 +155,10 @@ static inline
 unsigned long find_first_bit(const unsigned long *addr, unsigned long size)
 {
 	if (small_const_nbits(size)) {
-		unsigned long val = *addr & GENMASK(size - 1, 0);
+		unsigned long val;
 
+		val =  __find_bits_deref(addr, size);
+		val &= GENMASK(size - 1, 0);
 		return val ? __ffs(val) : size;
 	}
 
@@ -141,8 +182,11 @@ unsigned long find_first_and_bit(const unsigned long *addr1,
 				 unsigned long size)
 {
 	if (small_const_nbits(size)) {
-		unsigned long val = *addr1 & *addr2 & GENMASK(size - 1, 0);
+		unsigned long val;
 
+		val =  __find_bits_deref(addr1, size);
+		val &= __find_bits_deref(addr2, size);
+		val &= GENMASK(size - 1, 0);
 		return val ? __ffs(val) : size;
 	}
 
@@ -163,8 +207,10 @@ static inline
 unsigned long find_first_zero_bit(const unsigned long *addr, unsigned long size)
 {
 	if (small_const_nbits(size)) {
-		unsigned long val = *addr | ~GENMASK(size - 1, 0);
+		unsigned long val;
 
+		val =  __find_bits_deref(addr, size);
+		val |= ~GENMASK(size - 1, 0);
 		return val == ~0UL ? size : ffz(val);
 	}
 
@@ -184,8 +230,10 @@ static inline
 unsigned long find_last_bit(const unsigned long *addr, unsigned long size)
 {
 	if (small_const_nbits(size)) {
-		unsigned long val = *addr & GENMASK(size - 1, 0);
+		unsigned long val;
 
+		val =  __find_bits_deref(addr, size);
+		val &= GENMASK(size - 1, 0);
 		return val ? __fls(val) : size;
 	}
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 15+ messages in thread