Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
From:   "Eric W. Biederman" <ebiederm@xmission.com>
To:     Solar Designer <solar@openwall.com>
Cc:     linux-kernel@vger.kernel.org, Alexey Gladkov <legion@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        Shuah Khan <shuah@kernel.org>,
        Christian Brauner <brauner@kernel.org>,
        Ran Xiaokai <ran.xiaokai@zte.com.cn>,
        Michal Koutn?? <mkoutny@suse.com>, stable@vger.kernel.org
References: <87o83e2mbu.fsf@email.froward.int.ebiederm.org>
        <20220211021324.4116773-5-ebiederm@xmission.com>
        <20220212223638.GB29214@openwall.com>
        <87k0dxv5eq.fsf@email.froward.int.ebiederm.org>
Date:   Mon, 14 Feb 2022 09:23:40 -0600
In-Reply-To: <87k0dxv5eq.fsf@email.froward.int.ebiederm.org> (Eric
        W. Biederman's message of "Mon, 14 Feb 2022 09:23:09 -0600")
Message-ID: <87ee45v5dv.fsf@email.froward.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH 5/8] ucounts: Handle wrapping in is_ucounts_overlimit
Precedence: bulk

"Eric W. Biederman" <ebiederm@xmission.com> writes:

> Solar Designer <solar@openwall.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@vger.kernel.org
>>> Fixes: 21d1c5e386bc ("Reimplement RLIMIT_NPROC on top of ucounts")
>>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.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.
                                   ^^^^ execve

Eric