[v1] ucounts: fix counter leak in inc_rlimit_get_ucounts()

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

Posted by Andrei Vagin 3 weeks, 3 days ago

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;
 }
-- 
2.47.0.163.g1226f6d8fa-goog

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

Posted by Roman Gushchin 3 weeks, 3 days ago

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>

Acked-by: Roman Gushchin <roman.gushchin@linux.dev>

Thanks!

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

Posted by Alexey Gladkov 3 weeks, 3 days ago

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.

Acked-by: Alexey Gladkov <legion@kernel.org>

-- 
Rgrds, legion

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

Posted by Andrei Vagin 3 weeks, 3 days ago

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?

Thanks,
Andrei

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

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

Posted by Roman Gushchin 3 weeks, 3 days ago

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!