"Eric W. Biederman" <ebiederm@xxxxxxxxxxxx> writes:
> Solar Designer <solar@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
>>> Fixes: 21d1c5e386bc ("Reimplement RLIMIT_NPROC on top of ucounts")
>>> Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
>>> ---
>>> 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
