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] [day] [month] [year] [list]

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

Alexey Dobriyan <adobriyan@...il.com> writes:

>> 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.
>
> Multiple values, each one is bounded by it's own set of values.

Nack.  That is a change to the users of sysctl.   An array
of minmax values appears to be a brand new extension therefore
we should not make this change.

If we want new behavior we should introduce new helpers.

>> For the original sysctl design it may have made
>> some sense.  New code should be one value per file.
>
> So leave them alone.

Sure. My point is that we don't care about this for new code.
Old code was tested and used with the proc version.

NO ONE uses sys_sysctl NO ONE.

>> --- 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;
>
> Correct way is:
>
> 	min[i] <= val[i] <= max[i]
>
> This is what sysctl_intvec() does.

Therefore sysctl_intvec is wrong because it does not match
proc_dointvec_minmax.  proc_dointvec_minmax is definitive as that what
people use and test.  sysctl_intvec is a vestigial appendage that
was never really used, and it is already deprecated and scheduled for
remove because of this.

The only way to support the case of arrays of minmax values is to
show code that cares.  I did not see anyone passing in an array
in .extra one.  So you are advocating code that will walk around
and read random values from the kernels .data section.  Which
is VERY broken.

Eric

--
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/