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: [email protected]
Fixes: 21d1c5e386bc ("Reimplement RLIMIT_NPROC on top of ucounts")
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
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]);
}
--
2.29.2
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: [email protected]
> Fixes: 21d1c5e386bc ("Reimplement RLIMIT_NPROC on top of ucounts")
> Signed-off-by: "Eric W. Biederman" <[email protected]>
> ---
> 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
From: Solar Designer
> Sent: 12 February 2022 22:37
...
> 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:
I'm pretty sure assignments and casts of negative values to unsigned
types are actually well defined.
Although the actual value may differ for ones-compliment and
sign-overpunch systems.
But I suspect Linux requires twos-compliment negative numbers.
(In much the same way as it requires that NULL be the all zero
bit pattern - although a load of annoying compiler warnings are only
relevant if that isn't the case.)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Solar Designer <[email protected]> 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: [email protected]
>> Fixes: 21d1c5e386bc ("Reimplement RLIMIT_NPROC on top of ucounts")
>> Signed-off-by: "Eric W. Biederman" <[email protected]>
>> ---
>> 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
"Eric W. Biederman" <[email protected]> writes:
> Solar Designer <[email protected]> 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: [email protected]
>>> Fixes: 21d1c5e386bc ("Reimplement RLIMIT_NPROC on top of ucounts")
>>> Signed-off-by: "Eric W. Biederman" <[email protected]>
>>> ---
>>> 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
On Mon, Feb 14, 2022 at 09:23:09AM -0600, "Eric W. Biederman" <[email protected]> wrote:
> 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.
The fork path could make do with some kind of atomic add+check (similar
to inc_ucounts) and overflows would be sanitized by that. (Seems to
apply to other former RLIMIT_* per-user counters too.)
The is_ucounts_overlimit() and overflowable increment indeed appears
necessary only to satisfy the set*uid+execve pair.
For the sake of bug-fixing, both the patches 5/8 and 6/8 can have
Reviewed-by: Michal Koutn? <[email protected]>
Michal