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; }
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;-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
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
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!
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!
linux-stable-mirror@lists.linaro.org
-
Alexey Gladkov -
Andrei Vagin -
Roman Gushchin