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: <87k0dxv5eq.fsf@email.froward.int.ebiederm.org>
Date:   Mon, 14 Feb 2022 09:23:09 -0600
From:   "Eric W. Biederman" <ebiederm@...ssion.com>
To:     Solar Designer <solar@...nwall.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

Solar Designer <solar@...nwall.com> writes:

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

While in cases like this we pass the value in a long, the operations on
the value occur in an atomic_long_t.  As atomic_long_t is implemented in
assembly we do escape the problems of undefined behavior.


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

Thank you for spotting that.  That looks like a good idea.  Even if it
works in this case it is better to establish patterns that are not
problematic if copy and pasted elsewhere.

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

For the case of RLIMIT_NPROC the real world limit is PID_MAX_LIMIT
which is 2^22.

Beyond that the code deliberately uses all values with the high bit/sign
bit set to flag that things went too high.  So the code already reserves
half of the values.

> 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).

Pretty much. The function essentially only exists so that we can
handle the weirdness of RLIMIT_NPROC.  Now that I have discovered the
weirdness of RLIMIT_NPROC is old historical sloppiness I expect the
proper solution is to rework how RLIMIT_NPROC operates and to remove
is_ucounts_overlimit all together.   I have to figure out what a proper
RLIMIT_NPROC check looks like in proc.

Eric