"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