[2/2] kernel/sysctl.c: define minmax conv functions in terms of non-minmax versions
diff mbox series

Message ID 20181227111231.12912-3-zev@bewilderbeest.net
State New
Headers show
Series
  • sysctl: fix range-checking in do_proc_dointvec_minmax_conv()
Related show

Commit Message

Zev Weiss Dec. 27, 2018, 11:12 a.m. UTC
do_proc_do[u]intvec_minmax_conv() had included open-coded versions of
do_proc_do[u]intvec_conv(), though the signed one omitted the check
that the value is in [INT_MIN, INT_MAX].  Rather than increase the
duplication further by copying the additional check, we can instead
refactor both to be defined in terms of their non-bounded counterparts
(plus the added check).

Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
 kernel/sysctl.c | 50 ++++++++++++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 23 deletions(-)

Comments

Luis Chamberlain Feb. 6, 2019, 7:58 p.m. UTC | #1
On Thu, Dec 27, 2018 at 05:12:30AM -0600, Zev Weiss wrote:
> do_proc_do[u]intvec_minmax_conv() had included open-coded versions of
> do_proc_do[u]intvec_conv(), though the signed one omitted the check
> that the value is in [INT_MIN, INT_MAX].  Rather than increase the
> duplication further by copying the additional check, we can instead
> refactor both to be defined in terms of their non-bounded counterparts
> (plus the added check).

The code below looks fine, however it is a rather intrusive check.
Let's isntead open code the new bound check and Cc stable, and then
after we can get creative with the wrapper use.

Can you confirm the open coded version fixes the issues, and then
the other change does not regress? If you can include an annotation
as to since *when* this was broken by annotating it on your CC stable
note it would be useful for stable maintainers. Likewise if you can
add a respective Fixes: tag that would be appreciated if you can easily
find it.

The stable tag annotation can be placed on top of all the tags, for
instance if the first broken commit was in v4.1 then:

	Cc: <stable@vger.kernel.org> # v4.1+

Thanks for the fix and expanding on the tests!

  Luis

Patch
diff mbox series

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 5fc724e4e454..4a0261d32401 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2562,23 +2562,26 @@  static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
 					int *valp,
 					int write, void *data)
 {
+	int tmp, ret;
 	struct do_proc_dointvec_minmax_conv_param *param = data;
+
+	/*
+	 * First write to a temporary local int so we can bounds-check it
+	 * before touching *valp.
+	 */
+	int *ip = write ? &tmp : valp;
+
+	ret = do_proc_dointvec_conv(negp, lvalp, ip, write, data);
+	if (ret)
+		return ret;
+
 	if (write) {
-		int val = *negp ? -*lvalp : *lvalp;
-		if ((param->min && *param->min > val) ||
-		    (param->max && *param->max < val))
+		if ((param->min && *param->min > tmp) ||
+		    (param->max && *param->max < tmp))
 			return -EINVAL;
-		*valp = val;
-	} else {
-		int val = *valp;
-		if (val < 0) {
-			*negp = true;
-			*lvalp = -(unsigned long)val;
-		} else {
-			*negp = false;
-			*lvalp = (unsigned long)val;
-		}
+		*valp = tmp;
 	}
+
 	return 0;
 }
 
@@ -2627,22 +2630,23 @@  static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
 					 unsigned int *valp,
 					 int write, void *data)
 {
+	int ret;
+	unsigned int tmp;
 	struct do_proc_douintvec_minmax_conv_param *param = data;
 
-	if (write) {
-		unsigned int val = *lvalp;
+	/* write via temporary local uint for bounds-checking */
+	unsigned int *up = write ? &tmp : valp;
 
-		if (*lvalp > UINT_MAX)
-			return -EINVAL;
+	ret = do_proc_douintvec_conv(lvalp, up, write, data);
+	if (ret)
+		return ret;
 
-		if ((param->min && *param->min > val) ||
-		    (param->max && *param->max < val))
+	if (write) {
+		if ((param->min && *param->min > tmp) ||
+		    (param->max && *param->max < tmp))
 			return -ERANGE;
 
-		*valp = val;
-	} else {
-		unsigned int val = *valp;
-		*lvalp = (unsigned long) val;
+		*valp = tmp;
 	}
 
 	return 0;