linux-kernel - Re: [RESEND PATCH v3] sysctl: simplify the min/max boundary check

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: <4cce154a-1115-4371-a9f2-8bdb4879ea16@linux.dev>
Date: Wed, 30 Oct 2024 00:26:17 +0800
From: Wen Yang <wen.yang@...ux.dev>
To: Joel Granados <joel.granados@...nel.org>
Cc: "Eric W . Biederman" <ebiederm@...ssion.com>,
 Luis Chamberlain <mcgrof@...nel.org>, Kees Cook <keescook@...omium.org>,
 Joel Granados <j.granados@...sung.com>,
 Christian Brauner <brauner@...nel.org>, Dave Young <dyoung@...hat.com>,
 linux-kernel@...r.kernel.org
Subject: Re: [RESEND PATCH v3] sysctl: simplify the min/max boundary check



On 2024/10/23 03:12, Joel Granados wrote:
> On Thu, Sep 05, 2024 at 09:48:18PM +0800, Wen Yang wrote:
>> The do_proc_dointvec_minmax_conv_param structure provides the minimum and
>> maximum values for doing range checking for the proc_dointvec_minmax()
>> handler, while the do_proc_douintvec_minmax_conv_param structure also
>> provides these min/max values for doing range checking for the
>> proc_douintvec_minmax()/proc_dou8vec_minmax() handlers.
> Finally got around to reviewing this. Sorry for the wait. Thx for the
> patch but I don't like how this looks in terms of Integer promotion in
> 32b architectures.
> 
Yes, thank you for pointing it out.
We will explain it later.

>>
>> To avoid duplicate code, a new proc_minmax_conv_param structure has been
> I'm not seeing duplicate code here as one is handling the int case and
> the other is handling the uint case. And it is making sure that all
> assignments and comparisons are without any Integer Promotion issues.
> I'm not saying that it cannot be done, but it has to address Integer
> Promotion issues in 32b architectures.
> 
>> introduced to replace both do_proc_dointvec_minmax_conv_param and
>> do_proc_douintvec_minmax_conv_param mentioned above.
>>
>> This also prepares for the removal of sysctl_vals and sysctl_long_vals.
> If I'm not mistaken this is another patchset that you sent separetly. Is
> it "sysctl: encode the min/max values directly in the table entry"?
> 

Yes.

> ...
> 
>> @@ -904,8 +890,7 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
>>   		return ret;
>>   
>>   	if (write) {
>> -		if ((param->min && *param->min > tmp) ||
>> -		    (param->max && *param->max < tmp))
>> +		if ((param->min > tmp) || (param->max < tmp))
>>   			return -ERANGE;
>>   
>>   		WRITE_ONCE(*valp, tmp);
>> @@ -936,10 +921,10 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
>>   int proc_douintvec_minmax(const struct ctl_table *table, int write,
>>   			  void *buffer, size_t *lenp, loff_t *ppos)
>>   {
>> -	struct do_proc_douintvec_minmax_conv_param param = {
>> -		.min = (unsigned int *) table->extra1,
>> -		.max = (unsigned int *) table->extra2,
>> -	};
>> +	struct proc_minmax_conv_param param;
>> +
>> +	param.min = (table->extra1) ? *(unsigned int *) table->extra1 : 0;
>> +	param.max = (table->extra2) ? *(unsigned int *) table->extra2 : UINT_MAX;
> This is one of the cases where there is potential issues. Here, if the
>  value of table->extra{1,2}'s value is greater than when
> the maximum value of a signed long, then the value assigned would be
> incorrect. Note that the problem does not go away if you remove the
> "unsigned" qualifier; it remains if table->extra{1,2} are originally
> unsigned.
> 

I set up a CentOS 7.9 32-bit VM on Virtuanbox:
# uname  -a
Linux osboxes.org 3.10.0-1160.2.2.el7.centos.plus.i686 #1 SMP Mon Oct 26 
11:56:29 UTC 2020 i686 i686 i386 GNU/Linux

And the following test code:

#include <stdio.h>
#include <stdlib.h>

int main()
{
	unsigned int i = 4294967294;
	long j = i;

	printf("original hex(i) = 0x%x\n", i);
	printf("unsigned int(i) = %lu\n", i);
	printf("---------------------\n");
	printf("hex(j) = 0x%x\n", j);
	printf("long(j) = %ld\n", j);
	printf("unsigned long(j) = %lu\n", j);
	printf("int(j) = %d\n", j);
	printf("unsigned int(j) = %lu\n", j);
	return 0;
}


./a.out

original hex(i) = 0xfffffffe
unsigned int(i) = 4294967294
---------------------
hex(j) = 0xfffffffe
long(j) = -2
unsigned long(j) = 4294967294
int(j) = -2
unsigned int(j) = 4294967294


The original hexadecimal values are the same, using unsigned int, int, 
unsigned long, or long is just interpreted in different ways.

We also ensure consistency in numerical writing and type conversion in 
the patch. For example, in proc_rointvec_jiffies, convert to int; And in 
proc_rouintvec_minmax, it is converted to unsigned int.


--
Best wishes,
Wen


> I'm not sure if there are more, but just having one of these things
> around make me uncomfortable. Please re-work the patch in order to
> remove this issue in order to continue review.
> 
> best
>