Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 3 Aug 2020 11:29:38 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc:     Jason Gunthorpe <jgg@ziepe.ca>, Leon Romanovsky <leon@kernel.org>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Matthew Wilcox <willy@infradead.org>,
        linux-kernel@vger.kernel.org, kernel-hardening@lists.openwall.com
Subject: [RFC] saturate check_*_overflow() output?
Message-ID: <202008031118.36756FAD04@keescook>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

I wonder if we should explicitly saturate the output of the overflow
helpers as a side-effect of overflow detection? (That way the output
is never available with a "bad" value, if the caller fails to check the
result or forgets that *d was written...) since right now, *d will hold
the wrapped value.

Also, if we enable arithmetic overflow detection sanitizers, we're going
to trip over the fallback implementation (since it'll wrap and then do
the overflow test in the macro).

e.g. I'm think of something like this (showing only "mul" here, and
untested):

diff --git a/include/linux/overflow.h b/include/linux/overflow.h
index 93fcef105061..00baf3a75dc7 100644
--- a/include/linux/overflow.h
+++ b/include/linux/overflow.h
@@ -71,12 +71,16 @@
 })
 
 #define check_mul_overflow(a, b, d) ({		\
+	bool __result;				\
 	typeof(a) __a = (a);			\
 	typeof(b) __b = (b);			\
 	typeof(d) __d = (d);			\
 	(void) (&__a == &__b);			\
 	(void) (&__a == __d);			\
-	__builtin_mul_overflow(__a, __b, __d);	\
+	__result = __builtin_mul_overflow(__a, __b, __d);\
+	if (unlikely(__result))			\
+		*__d = type_max(__a);		\
+	__result;				\
 })
 
 #else
@@ -105,15 +109,20 @@
  * If one of a or b is a compile-time constant, this avoids a division.
  */
 #define __unsigned_mul_overflow(a, b, d) ({		\
+	bool __result;					\
 	typeof(a) __a = (a);				\
 	typeof(b) __b = (b);				\
 	typeof(d) __d = (d);				\
 	(void) (&__a == &__b);				\
 	(void) (&__a == __d);				\
-	*__d = __a * __b;				\
-	__builtin_constant_p(__b) ?			\
+	__result = __builtin_constant_p(__b) ?		\
 	  __b > 0 && __a > type_max(typeof(__a)) / __b : \
 	  __a > 0 && __b > type_max(typeof(__b)) / __a;	 \
+	if (unlikely(__result))				\
+		*__d = type_max(typeof(__a));		\
+	else						\
+		*__d = __a * __b;			\
+	__result;
 })
 
 /*
@@ -176,6 +185,7 @@
  */
 
 #define __signed_mul_overflow(a, b, d) ({				\
+	bool __result;							\
 	typeof(a) __a = (a);						\
 	typeof(b) __b = (b);						\
 	typeof(d) __d = (d);						\
@@ -183,10 +193,14 @@
 	typeof(a) __tmin = type_min(typeof(a));				\
 	(void) (&__a == &__b);						\
 	(void) (&__a == __d);						\
-	*__d = (u64)__a * (u64)__b;					\
-	(__b > 0   && (__a > __tmax/__b || __a < __tmin/__b)) ||	\
-	(__b < (typeof(__b))-1  && (__a > __tmin/__b || __a < __tmax/__b)) || \
-	(__b == (typeof(__b))-1 && __a == __tmin);			\
+	__result = (__b > 0   && (__a > __tmax/__b || __a < __tmin/__b)) || \
+		   (__b < (typeof(__b))-1  && (__a > __tmin/__b || __a < __tmax/__b)) || \
+		   (__b == (typeof(__b))-1 && __a == __tmin);		\
+	if (unlikely(__result))						\
+		*__d = type_max(__a);					\
+	else								\
+		*__d = (u64)__a * (u64)__b;				\
+	__result;							\
 })
 
 

Thoughts?

-- 
Kees Cook