linux-kernel - Re: [PATCH] signal: restore the override

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: <ZyU8UNKLNfAi-U8F@google.com>
Date: Fri, 1 Nov 2024 20:38:40 +0000
From: Roman Gushchin <roman.gushchin@...ux.dev>
To: "Eric W. Biederman" <ebiederm@...ssion.com>
Cc: linux-kernel@...r.kernel.org, Andrei Vagin <avagin@...gle.com>,
	Kees Cook <kees@...nel.org>, Alexey Gladkov <legion@...nel.org>,
	stable@...r.kernel.org
Subject: Re: [PATCH] signal: restore the override_rlimit logic

On Fri, Nov 01, 2024 at 02:51:00PM -0500, Eric W. Biederman wrote:
> Roman Gushchin <roman.gushchin@...ux.dev> writes:
> 
> > Prior to commit d64696905554 ("Reimplement RLIMIT_SIGPENDING on top of
> > ucounts") UCOUNT_RLIMIT_SIGPENDING rlimit was not enforced for a class
> > of signals. However now it's enforced unconditionally, even if
> > override_rlimit is set.
> 
> Not true.
> 
> It added a limit on the number of siginfo structures that
> a container may allocate.  Have you tried not limiting your
> container?
> 
> >This behavior change caused production issues.
> 
> > 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.
> 
> Note.  There are always conditions when the allocation may fail.
> The structure is allocated with __GFP_ATOMIC so it is much more likely
> to fail than a typical kernel memory allocation.
> 
> But I agree it does look like there is a quality of implementation issue
> here.
> 
> > Fix this by passing override_rlimit into inc_rlimit_get_ucounts() and
> > skip the comparison to max there if override_rlimit is set. This
> > effectively restores the old behavior.
> 
> Instead please just give the container and unlimited number of siginfo
> structures it can play with.

Well, personally I'd not use this limit too, but I don't think
"it's broken, userspace shouldn't use it" argument is valid.

> 
> The maximum for rlimit(RLIM_SIGPENDING) is the rlimit(RLIM_SIGPENDING)
> value when the user namespace is created.
> 
> Given that it took 3 and half years to report this.  I am going to
> say this really looks like a userspace bug.

The trick here is another bug fixed by https://lkml.org/lkml/2024/10/31/185.
Basically it's a leak of the rlimit value.
If a limit is set and reached in the reality, all following signals
will not have a siginfo attached, causing applications which depend on
handling SIGSEGV to crash.

> Beyond that your patch is actually buggy, and should not be applied.
> 
> If we want to change the semantics and ignore the maximum number of
> pending signals in a container (when override_rlimit is set) then
> the code should change the computation of the max value (pegging it at
> LONG_MAX) and not ignore it.

Hm, isn't the unconditional (new < 0) enough to capture the overflow?
Actually I'm not sure I understand how "long new" can be "> LONG_MAX"
anyway.

Thanks!