linux-kernel - Re: [PATCH 5/8] ucounts: Handle wrapping in is_ucounts

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: <20220212223638.GB29214@openwall.com>
Date:   Sat, 12 Feb 2022 23:36:39 +0100
From:   Solar Designer <solar@...nwall.com>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     linux-kernel@...r.kernel.org, Alexey Gladkov <legion@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Shuah Khan <shuah@...nel.org>,
        Christian Brauner <brauner@...nel.org>,
        Ran Xiaokai <ran.xiaokai@....com.cn>,
        Michal Koutn?? <mkoutny@...e.com>, stable@...r.kernel.org
Subject: Re: [PATCH 5/8] ucounts: Handle wrapping in is_ucounts_overlimit

On Thu, Feb 10, 2022 at 08:13:21PM -0600, Eric W. Biederman wrote:
> While examining is_ucounts_overlimit and reading the various messages
> I realized that is_ucounts_overlimit fails to deal with counts that
> may have wrapped.
> 
> Being wrapped should be a transitory state for counts and they should
> never be wrapped for long, but it can happen so handle it.
> 
> Cc: stable@...r.kernel.org
> Fixes: 21d1c5e386bc ("Reimplement RLIMIT_NPROC on top of ucounts")
> Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.com>
> ---
>  kernel/ucount.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/ucount.c b/kernel/ucount.c
> index 65b597431c86..06ea04d44685 100644
> --- a/kernel/ucount.c
> +++ b/kernel/ucount.c
> @@ -350,7 +350,8 @@ bool is_ucounts_overlimit(struct ucounts *ucounts, enum ucount_type type, unsign
>  	if (rlimit > LONG_MAX)
>  		max = LONG_MAX;
>  	for (iter = ucounts; iter; iter = iter->ns->ucounts) {
> -		if (get_ucounts_value(iter, type) > max)
> +		long val = get_ucounts_value(iter, type);
> +		if (val < 0 || val > max)
>  			return true;
>  		max = READ_ONCE(iter->ns->ucount_max[type]);
>  	}

You probably deliberately assume "gcc -fwrapv", but otherwise:

As you're probably aware, a signed integer wrapping is undefined
behavior in C.  In the function above, "val" having wrapped to negative
assumes we had occurred UB elsewhere.  Further, there's an instance of
UB in the function itself:

bool is_ucounts_overlimit(struct ucounts *ucounts, enum ucount_type type, unsigned long rlimit)
{
	struct ucounts *iter;
	long max = rlimit;
	if (rlimit > LONG_MAX)
		max = LONG_MAX;

The assignment on "long max = rlimit;" would have already been UB if
"rlimit > LONG_MAX", which is only checked afterwards.  I think the
above would be better written as:

	if (rlimit > LONG_MAX)
		rlimit = LONG_MAX;
	long max = rlimit;

considering that "rlimit" is never used further in that function.

And to more likely avoid wraparound of "val", perhaps have the limit at
a value significantly lower than LONG_MAX, like half that?  So:

	if (rlimit > LONG_MAX / 2)
		rlimit = LONG_MAX / 2;
	long max = rlimit;

And sure, also keep the "val < 0" check as defensive programming, or you
can do:

	if (rlimit > LONG_MAX / 2)
		rlimit = LONG_MAX / 2;
[...]
		if ((unsigned long)get_ucounts_value(iter, type) > rlimit)
			return true;

and drop both "val" and "max".  However, this also assumes the return
type of get_ucounts_value() doesn't become larger than "unsigned long".

I assume that once is_ucounts_overlimit() returned true, it is expected
the value would almost not grow further (except a little due to races).

I also assume there's some reason a signed type is used there.

Alexander