linux-kernel - Re: [PATCH] ucounts: fix counter leak in inc_rlimit_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEWA0a7W4u189wViEk2P9ZBgUe7DFGmSA8UKW0gKvCC8_pRiHw@mail.gmail.com>
Date: Thu, 31 Oct 2024 08:43:22 -0700
From: Andrei Vagin <avagin@...gle.com>
To: Alexey Gladkov <legion@...nel.org>
Cc: "Eric W. Biederman" <ebiederm@...ssion.com>, Kees Cook <kees@...nel.org>, 
	Roman Gushchin <roman.gushchin@...ux.dev>, linux-kernel@...r.kernel.org, 
	stable@...r.kernel.org
Subject: Re: [PATCH] ucounts: fix counter leak in inc_rlimit_get_ucounts()

On Thu, Oct 31, 2024 at 2:50 AM Alexey Gladkov <legion@...nel.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@...ux.dev>
> > Co-debugged-by: Roman Gushchin <roman.gushchin@...ux.dev>
> > Cc: Kees Cook <kees@...nel.org>
> > Cc: Andrei Vagin <avagin@...gle.com>
> > Cc: "Eric W. Biederman" <ebiederm@...ssion.com>
> > Cc: Alexey Gladkov <legion@...nel.org>
> > Cc: <stable@...r.kernel.org>
> > Signed-off-by: Andrei Vagin <avagin@...gle.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?

Thanks,
Andrei

>
> Acked-by: Alexey Gladkov <legion@...nel.org>
>
> --
> Rgrds, legion
>