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

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

[Jiashuo Liang]

Before this patch, the __bad_area_nosemaphore function calls both
force_sig_pkuerr and force_sig_fault when handling SEGV_PKUERR. This does
not cause problems because the second signal is filtered by the
legacy_queue check in __send_signal. But it causes the kernel to do
unnecessary work.

This patch should fix it.

Fixes: 9db812dbb29d ("signal/x86: Call force_sig_pkuerr from __bad_area_nosemaphore")
Suggested-by: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Jiashuo Liang <liangjs@pku.edu.cn>
---
 arch/x86/mm/fault.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 1c548ad00752..6bda7f67d737 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -836,8 +836,8 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 
 	if (si_code == SEGV_PKUERR)
 		force_sig_pkuerr((void __user *)address, pkey);
-
-	force_sig_fault(SIGSEGV, si_code, (void __user *)address);
+	else
+		force_sig_fault(SIGSEGV, si_code, (void __user *)address);
 
 	local_irq_disable();
 }
-- 
2.31.1

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

[Eric W. Biederman]

Jiashuo Liang <liangjs@pku.edu.cn> writes:

> Before this patch, the __bad_area_nosemaphore function calls both
> force_sig_pkuerr and force_sig_fault when handling SEGV_PKUERR. This does
> not cause problems because the second signal is filtered by the
> legacy_queue check in __send_signal. But it causes the kernel to do
> unnecessary work.
>
> This patch should fix it.

Have you been able to test this patch?

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

Does one of the x86 maintainers want to pick up this trivial fix or
should I pick it up?

Eric


> Fixes: 9db812dbb29d ("signal/x86: Call force_sig_pkuerr from __bad_area_nosemaphore")
> Suggested-by: "Eric W. Biederman" <ebiederm@xmission.com>
> Signed-off-by: Jiashuo Liang <liangjs@pku.edu.cn>
> ---
>  arch/x86/mm/fault.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 1c548ad00752..6bda7f67d737 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -836,8 +836,8 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
>  
>  	if (si_code == SEGV_PKUERR)
>  		force_sig_pkuerr((void __user *)address, pkey);
> -
> -	force_sig_fault(SIGSEGV, si_code, (void __user *)address);
> +	else
> +		force_sig_fault(SIGSEGV, si_code, (void __user *)address);
>  
>  	local_irq_disable();
>  }

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

[Borislav Petkov]

On Wed, Jun 02, 2021 at 01:42:22PM -0500, Eric W. Biederman wrote:
> Does one of the x86 maintainers want to pick up this trivial fix or
> should I pick it up?

I can, tomorrow.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Borislav Petkov]

On Tue, Jun 01, 2021 at 04:52:03PM +0800, Jiashuo Liang wrote:
> Before this patch, the __bad_area_nosemaphore function calls both
> force_sig_pkuerr and force_sig_fault when handling SEGV_PKUERR. This does
> not cause problems because the second signal is filtered by the
> legacy_queue check in __send_signal.

I'm likely missing something but the first signal gets filtered by that
same legacy_queue() check too, no?

Because both calls end up in

	force_sig_info_to_task(info, current);

with the info struct populated with:

	info.si_signo = SIGSEGV;
        info.si_errno = 0;
        info.si_code  = SEGV_PKUERR;
        info.si_addr  = addr;
        info.si_pkey  = pkey;

except the second call - force_sig_fault() - doesn't put pkey in
->si_pkey.

So what's up?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Eric W. Biederman]

Borislav Petkov <bp@alien8.de> writes:

> On Tue, Jun 01, 2021 at 04:52:03PM +0800, Jiashuo Liang wrote:
>> Before this patch, the __bad_area_nosemaphore function calls both
>> force_sig_pkuerr and force_sig_fault when handling SEGV_PKUERR. This does
>> not cause problems because the second signal is filtered by the
>> legacy_queue check in __send_signal.
>
> I'm likely missing something but the first signal gets filtered by that
> same legacy_queue() check too, no?
>
> Because both calls end up in
>
> 	force_sig_info_to_task(info, current);
>
> with the info struct populated with:
>
> 	info.si_signo = SIGSEGV;
>         info.si_errno = 0;
>         info.si_code  = SEGV_PKUERR;
>         info.si_addr  = addr;
>         info.si_pkey  = pkey;
>
> except the second call - force_sig_fault() - doesn't put pkey in
> ->si_pkey.
>
> So what's up?

There are two ways signals get delivered.  The old fashioned way in the
signal bitmap, and the new fangled way by queuing sigqueue_info.  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.

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).

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.  That is fragile and confusing to depend on so we should
just fix the code to not send the wrong signal.

> Thx.

I hope that clears things up.

Eric

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

[Borislav Petkov]

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?

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!

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Eric W. Biederman]

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

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

[Borislav Petkov]

On Fri, Jun 04, 2021 at 09:33:12AM -0500, Eric W. Biederman wrote:
> 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.

Oh yeah, good idea. Signals and their handling make most people cringe.

Also, selftests. I wonder if it would be a good idea to make it a
kernelnewbies project for people to do short programs, each exercising
an API from the Linux manpages.

Or maybe even get Michael (CCed) to donate some of the examples from his
book:

https://man7.org/tlpi/code/online/all_files_by_chapter.html

in this case, chapters 20-22, as selftests for the kernel. Or maybe even
all examples. :-)

Just an idea anyway.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[tip: x86/urgent] x86/fault: Don't send SIGSEGV twice on SEGV_PKUERR

[tip-bot2 for Jiashuo Liang]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     5405b42c2f08efe67b531799ba2fdb35bac93e70
Gitweb:        https://git.kernel.org/tip/5405b42c2f08efe67b531799ba2fdb35bac93e70
Author:        Jiashuo Liang <liangjs@pku.edu.cn>
AuthorDate:    Tue, 01 Jun 2021 16:52:03 +08:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Fri, 04 Jun 2021 15:23:28 +02:00

x86/fault: Don't send SIGSEGV twice on SEGV_PKUERR

__bad_area_nosemaphore() calls both force_sig_pkuerr() and
force_sig_fault() when handling SEGV_PKUERR. This does not cause
problems because the second signal is filtered by the legacy_queue()
check in __send_signal() because in both cases, the signal is SIGSEGV,
the second one seeing that the first one is already pending.

This causes the kernel to do unnecessary work so send the signal only
once for SEGV_PKUERR.

 [ bp: Massage commit message. ]

Fixes: 9db812dbb29d ("signal/x86: Call force_sig_pkuerr from __bad_area_nosemaphore")
Suggested-by: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Jiashuo Liang <liangjs@pku.edu.cn>
Signed-off-by: Borislav Petkov <bp@suse.de>
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
Link: https://lkml.kernel.org/r/20210601085203.40214-1-liangjs@pku.edu.cn
---
 arch/x86/mm/fault.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 1c548ad..6bda7f6 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -836,8 +836,8 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
 
 	if (si_code == SEGV_PKUERR)
 		force_sig_pkuerr((void __user *)address, pkey);
-
-	force_sig_fault(SIGSEGV, si_code, (void __user *)address);
+	else
+		force_sig_fault(SIGSEGV, si_code, (void __user *)address);
 
 	local_irq_disable();
 }