Re: [PATCH] ucounts: fix counter leak in inc_rlimit_get_ucounts()

[Roman Gushchin <roman.gushchin@linux.dev>]

On Thu, Oct 31, 2024 at 08:43:22AM -0700, Andrei Vagin wrote:
> On Thu, Oct 31, 2024 at 2:50 AM Alexey Gladkov <legion@kernel.org> wrote:
> >
> > On Thu, Oct 31, 2024 at 04:56:01AM +0000, Andrei Vagin wrote:
> > > The inc_rlimit_get_ucounts() increments the specified rlimit counter and
> > > then checks its limit. If the value exceeds the limit, the function
> > > returns an error without decrementing the counter.
> > >
> > > Fixes: 15bc01effefe ("ucounts: Fix signal ucount refcounting")
> > > Tested-by: Roman Gushchin <roman.gushchin@linux.dev>
> > > Co-debugged-by: Roman Gushchin <roman.gushchin@linux.dev>
> > > Cc: Kees Cook <kees@kernel.org>
> > > Cc: Andrei Vagin <avagin@google.com>
> > > Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> > > Cc: Alexey Gladkov <legion@kernel.org>
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Andrei Vagin <avagin@google.com>
> > > ---
> > >  kernel/ucount.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/ucount.c b/kernel/ucount.c
> > > index 8c07714ff27d..16c0ea1cb432 100644
> > > --- a/kernel/ucount.c
> > > +++ b/kernel/ucount.c
> > > @@ -328,13 +328,12 @@ long inc_rlimit_get_ucounts(struct ucounts *ucounts, enum rlimit_type type)
> > >               if (new != 1)
> > >                       continue;
> > >               if (!get_ucounts(iter))
> > > -                     goto dec_unwind;
> > > +                     goto unwind;
> > >       }
> > >       return ret;
> > > -dec_unwind:
> > > +unwind:
> > >       dec = atomic_long_sub_return(1, &iter->rlimit[type]);
> > >       WARN_ON_ONCE(dec < 0);
> > > -unwind:
> > >       do_dec_rlimit_put_ucounts(ucounts, iter, type);
> > >       return 0;
> > >  }
> >
> > Agree. The do_dec_rlimit_put_ucounts() decreases rlimit up to iter but
> > does not include it.
> >
> > Except for a small NAK because the patch changes goto for get_ucounts()
> > and not for rlimit overflow check.
> 
> Do you think it is better to rename the label and use dec_unwind? I don't
> think it makes a big difference, but if you think it does, I can send
> this version.
> 
> BTW, while investigating this, we found another one. Currently,
> sigqueue_alloc enforces a counter limit even when override_rlimit is set
> to true. This was introduced by commit f3791f4df569ea ("Fix
> UCOUNT_RLIMIT_SIGPENDING counter leak"). This change in behavior has
> introduced regressions, causing failures in applications that previously
> functioned correctly.
> 
> For example, if the limit is reached and a process receives a SIGSEGV
> signal, sigqueue_alloc fails to allocate the necessary resources for the
> signal delivery, preventing the signal from being delivered with
> siginfo. This prevents the process from correctly identifying the fault
> address and handling the error. From the user-space perspective,
> applications are unaware that the limit has been reached and that the
> siginfo is effectively 'corrupted'. This can lead to unpredictable
> behavior and crashes, as we observed with java applications.
> 
> To address this, we think to restore the original logic for
> override_rlimit. This will ensure that kernel signals are always
> delivered correctly, regardless of the counter limit.  Does this
> approach seem reasonable? Do you have any concerns?

I think override_rlimit argument should be passed into inc_rlimit_get_ucounts()
and be handled there properly by ignoring the comparison with max.

I gonna prepare a patch later today, if there are no objections for this
approach.

Thanks!