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