linux-kernel - Re: [PATCH] sysctl: min-max range check is broken

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <m1zlh0txgj.fsf@fess.ebiederm.org>
Date:	Thu, 05 Feb 2009 12:55:56 -0800
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Shakesh Jain <shjain@...mai.com>, ShakeshJain@...mai.com,
	linux-kernel@...r.kernel.org, juhlenko@...mai.com,
	Alexey Dobriyan <adobriyan@...il.com>
Subject: Re: [PATCH] sysctl: min-max range check is broken

Andrew Morton <akpm@...ux-foundation.org> writes:

> On Wed, 4 Feb 2009 00:40:22 -0800
> Shakesh Jain <shjain@...mai.com>, ShakeshJain@...mai.com wrote:
>
>> do_proc_dointvec_minmax_conv() which gets callled from
>> proc_dointvec_minmax proc_handler doesn't increment the pointer to
>> the 'min' (extra1) and 'max' (extra2) after each range check which
>> results in doing the check against same set of min and max values.
>> 
>> This breaks the range checking for those sysctl's where you can
>> write multiple values to /proc with each variable having its own range
>> specification.

I just did a quick grep for .extra1 and I don't see anywhere we use
the code as described.

>> It seems to be implemented for the sysctl() system call strategy in
>> sysctl_intvec() where min and max are treated as arrays.

Yep.  There is an inconsistency here.  Given how sysctl is used and
tested, and the fact I could not find where it appears that anyone is
passing an array into min/max I would say that the proc version is
correct and the sysctl version is wrong.

The untested patch below looks like it will fix the this.

I don't know if there are any cases where we use minmax with an array
of integers but I don't see the point of using an array of minmax
values at this point.  For the original sysctl design it may have made
some sense.  New code should be one value per file.

Eric



diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 790f9d7..4050ce1 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2916,9 +2916,9 @@ int sysctl_intvec(struct ctl_table *table,
 			int value;
 			if (get_user(value, vec + i))
 				return -EFAULT;
-			if (min && value < min[i])
+			if (min && value < *min)
 				return -EINVAL;
-			if (max && value > max[i])
+			if (max && value > *max)
 				return -EINVAL;
 		}
 	}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/