Re: [PATCH] signal/x86: Don't send SIGSEGV twice on SEGV_PKUERR

From: ebiederm@xmission.com (Eric W. Biederman)
To: Borislav Petkov <bp@alien8.de>
Cc: Jiashuo Liang <liangjs@pku.edu.cn>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Andy Lutomirski <luto@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>,
	x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] signal/x86: Don't send SIGSEGV twice on SEGV_PKUERR
Date: Fri, 04 Jun 2021 09:33:12 -0500	[thread overview]
Message-ID: <878s3plmxj.fsf@disp2133> (raw)
In-Reply-To: <YLolc7z64h9yHNao@zn.tnic> (Borislav Petkov's message of "Fri, 4 Jun 2021 15:06:59 +0200")

Borislav Petkov <bp@alien8.de> writes:

> On Thu, Jun 03, 2021 at 04:31:46PM -0500, Eric W. Biederman wrote:
>> There are two ways signals get delivered.  The old fashioned way in the
>> signal bitmap, and the new fangled way by queuing sigqueue_info.
>
> By that you mean that third arg siginfo_t to
>
> SYSCALL_DEFINE3(rt_sigqueueinfo, pid_t, pid, int, sig,
>                 siginfo_t __user *, uinfo)
>
> I presume?

Oh yes.  siginfo_t.  I don't know why I was thinking sigqueue_info.

> Which, as sigqueue(3) says, is what is called on Linux.
>
>> In the old fashioned way there is no information except that the
>> signal itself was delivered, and if the signal is sent twice it
>> is impossible to find out. In the new fangled way because the
>> sigqueue_info can vary between different times a signal is sent you
>> can both see that a signal was delivered twice (because there are two
>> distinct entries in the queue), but also possibly tell those two times
>> a signal was sent apart.
>>
>> The new real time signals can queue as many sigqueue_info's as their
>> rlimit allows.  The old signals are limited to exactly one sigqueue_info
>> per signal number.
>
> Aha.
>
>> In this case the legacy_queue check tests to see if the signal is
>> already pending (present in the signal bitmap) and not a new real time
>> signal (which means only one sigqueue_info entry is allowed in the
>> signal queue).
>
> Aha, that sigismember() call in legacy_queue().
>
>> Or in short I think everything turns out ok because the first signal is
>> delivered, and the second just happens to get dropped as a duplicate by
>> __send_signal.
>
> Right, it is a SIGSEGV in both cases. So it is a legacy signal, and
> that'll get marked in that sigset->sig array. Which is per task... ok.
>
>> That is fragile and confusing to depend on so we should just fix the
>> code to not send the wrong signal.
>
> Yap.
>
>> I hope that clears things up.
>
> Very much so, thanks for taking the time!

No worries.  It was my bug in the first place.

At some point I just figured someone needs to take the time to
understand the linux signal handling and get as many bugs out as we
can.  It may not be flashy but it is one of those core things
that everything is built on so we need code that works.

Eric