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: <5yfnu64fqsuahcmifvqdaynvdesqvaehhikhjff46ndoaacxyd@jvjrd3ivdpyz>
Date: Tue, 22 Oct 2024 21:12:48 +0200
From: Joel Granados <joel.granados@...nel.org>
To: Wen Yang <wen.yang@...ux.dev>
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 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.

> 
> 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"?

...

> @@ -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
unsigned integer 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'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

-- 

Joel Granados