[PATCH] x86/mpx: Correctly report do_mpx_bt_fault() failures to user-space

[PATCH] x86/mpx: Correctly report do_mpx_bt_fault() failures to user-space

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

When this function fails it just sends a SIGSEGV signal to
user-space using force_sig(). This signal is missing
essential information about the cause, e.g. the trap_nr or
an error code.

Fix this by propagating the error to the only caller of
mpx_handle_bd_fault(), do_bounds(), which sends the correct
SIGSEGV signal to the process.

Fixes: fe3d197f84319 ('x86, mpx: On-demand kernel allocation of bounds tables')
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/mm/mpx.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
index cd44ae7..1c34b76 100644
--- a/arch/x86/mm/mpx.c
+++ b/arch/x86/mm/mpx.c
@@ -526,15 +526,7 @@ int mpx_handle_bd_fault(void)
 	if (!kernel_managing_mpx_tables(current->mm))
 		return -EINVAL;
 
-	if (do_mpx_bt_fault()) {
-		force_sig(SIGSEGV, current);
-		/*
-		 * The force_sig() is essentially "handling" this
-		 * exception, so we do not pass up the error
-		 * from do_mpx_bt_fault().
-		 */
-	}
-	return 0;
+	return do_mpx_bt_fault();
 }
 
 /*
-- 
1.9.1

[tip:x86/mm] x86/mpx: Correctly report do_mpx_bt_fault() failures to user-space

[tip-bot for Joerg Roedel]

Commit-ID:  5ed386ec09a5d75bcf073967e55e895c2607a5c3
Gitweb:     http://git.kernel.org/tip/5ed386ec09a5d75bcf073967e55e895c2607a5c3
Author:     Joerg Roedel <jroedel@suse.de>
AuthorDate: Thu, 6 Apr 2017 16:19:22 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 12 Apr 2017 08:40:58 +0200

x86/mpx: Correctly report do_mpx_bt_fault() failures to user-space

When this function fails it just sends a SIGSEGV signal to
user-space using force_sig(). This signal is missing
essential information about the cause, e.g. the trap_nr or
an error code.

Fix this by propagating the error to the only caller of
mpx_handle_bd_fault(), do_bounds(), which sends the correct
SIGSEGV signal to the process.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: fe3d197f84319 ('x86, mpx: On-demand kernel allocation of bounds tables')
Link: http://lkml.kernel.org/r/1491488362-27198-1-git-send-email-joro@8bytes.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/mm/mpx.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
index cd44ae7..1c34b76 100644
--- a/arch/x86/mm/mpx.c
+++ b/arch/x86/mm/mpx.c
@@ -526,15 +526,7 @@ int mpx_handle_bd_fault(void)
 	if (!kernel_managing_mpx_tables(current->mm))
 		return -EINVAL;
 
-	if (do_mpx_bt_fault()) {
-		force_sig(SIGSEGV, current);
-		/*
-		 * The force_sig() is essentially "handling" this
-		 * exception, so we do not pass up the error
-		 * from do_mpx_bt_fault().
-		 */
-	}
-	return 0;
+	return do_mpx_bt_fault();
 }
 
 /*

Re: [PATCH] x86/mpx: Correctly report do_mpx_bt_fault() failures to user-space

[Dave Hansen]

Hi Joerg,

> When this function fails it just sends a SIGSEGV signal to
> user-space using force_sig(). This signal is missing
> essential information about the cause, e.g. the trap_nr or
> an error code.
>
> Fix this by propagating the error to the only caller of
> mpx_handle_bd_fault(), do_bounds(), which sends the correct
> SIGSEGV signal to the process.

Just to be clear, the thing you're calling "correct" is this do_trap(),
right?

        do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, error_code, NULL);

> Fixes: fe3d197f84319 ('x86, mpx: On-demand kernel allocation of bounds
tables')
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  arch/x86/mm/mpx.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
> index cd44ae7..1c34b76 100644
> --- a/arch/x86/mm/mpx.c
> +++ b/arch/x86/mm/mpx.c
> @@ -526,15 +526,7 @@ int mpx_handle_bd_fault(void)
>  	if (!kernel_managing_mpx_tables(current->mm))
>  		return -EINVAL;
>
> -	if (do_mpx_bt_fault()) {
> -		force_sig(SIGSEGV, current);
> -		/*
> -		 * The force_sig() is essentially "handling" this
> -		 * exception, so we do not pass up the error
> -		 * from do_mpx_bt_fault().
> -		 */
> -	}
> -	return 0;
> +	return do_mpx_bt_fault();
>  }

do_mpx_bt_fault() can fail for a bunch of reasons:
 * unexpected or invalid value in BNDCSR
 * out of memory (physical or virtual)
 * unresolvable fault walking/filling bounds tables
 * !valid and non-empty bad entry in the bounds tables

This will end up sending a signal that *looks* like a X86_TRAP_BR for
all of those, including those that are not really bounds-related, like
unresolvable faults.  We also don't populate enough information in the
siginfo that gets delivered for userspace to resolve the fault.

I'm not sure this patch is the right thing.

Re: [PATCH] x86/mpx: Correctly report do_mpx_bt_fault() failures to user-space

[Joerg Roedel]

Hi Dave,

On Mon, Apr 17, 2017 at 08:38:03AM -0700, Dave Hansen wrote:
> Just to be clear, the thing you're calling "correct" is this do_trap(),
> right?
> 
>         do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, error_code, NULL);

Yes, because it signals the right trap_nr and error_code to user-space.

> do_mpx_bt_fault() can fail for a bunch of reasons:
>  * unexpected or invalid value in BNDCSR
>  * out of memory (physical or virtual)
>  * unresolvable fault walking/filling bounds tables
>  * !valid and non-empty bad entry in the bounds tables
> 
> This will end up sending a signal that *looks* like a X86_TRAP_BR for
> all of those, including those that are not really bounds-related, like
> unresolvable faults.  We also don't populate enough information in the
> siginfo that gets delivered for userspace to resolve the fault.
> 
> I'm not sure this patch is the right thing.

The problem is, without this patch the trap_nr reported to user-space is
0, which maps to divide-by-zero. I think this is wrong, and since all
failure cases from do_mpx_bt_fault() can only happen in the #BR
exception handler, I think that reporting X86_TRAP_BR for all failure
cases is the right thing to do.

I don't know whether user-space (with this patch) already gets enough
information from do_trap() to handle all of the above cases, but it is a
step in the right direction.


	Joerg

Re: [PATCH] x86/mpx: Correctly report do_mpx_bt_fault() failures to user-space

[Dave Hansen]

On 04/20/2017 05:08 AM, Joerg Roedel wrote:
>> do_mpx_bt_fault() can fail for a bunch of reasons:
>>  * unexpected or invalid value in BNDCSR
>>  * out of memory (physical or virtual)
>>  * unresolvable fault walking/filling bounds tables
>>  * !valid and non-empty bad entry in the bounds tables
>>
>> This will end up sending a signal that *looks* like a X86_TRAP_BR for
>> all of those, including those that are not really bounds-related, like
>> unresolvable faults.  We also don't populate enough information in the
>> siginfo that gets delivered for userspace to resolve the fault.
>>
>> I'm not sure this patch is the right thing.
> 
> The problem is, without this patch the trap_nr reported to user-space is
> 0, which maps to divide-by-zero. I think this is wrong, and since all
> failure cases from do_mpx_bt_fault() can only happen in the #BR
> exception handler, I think that reporting X86_TRAP_BR for all failure
> cases is the right thing to do.

Urg, that does sound bogus.

How about doing X86_TRAP_PF?  That would at least be consistent with
SIGBUS, which is probably the closest thing to a generic error code that
we have.

Re: [PATCH] x86/mpx: Correctly report do_mpx_bt_fault() failures to user-space

[Joerg Roedel]

On Thu, Apr 20, 2017 at 08:45:28AM -0700, Dave Hansen wrote:
> How about doing X86_TRAP_PF?  That would at least be consistent with
> SIGBUS, which is probably the closest thing to a generic error code that
> we have.

Correct me if I am wrong, but for SIGBUS this only happens in the
page-fault path, right? And this path is indeed entered on a #PF
exception.

I see no reason to lie to user-space about the trap_nr that caused the
SIGSEGV, especially since user-space software needs to be modified to
make use of MPX, including the signal handler. So there is no risk of
introducing any incompatibility or regression, no?


	Joerg

Re: [PATCH] x86/mpx: Correctly report do_mpx_bt_fault() failures to user-space

[Dave Hansen]

On 04/21/2017 05:19 AM, Joerg Roedel wrote:
> On Thu, Apr 20, 2017 at 08:45:28AM -0700, Dave Hansen wrote:
>> How about doing X86_TRAP_PF?  That would at least be consistent with
>> SIGBUS, which is probably the closest thing to a generic error code that
>> we have.
> Correct me if I am wrong, but for SIGBUS this only happens in the
> page-fault path, right? And this path is indeed entered on a #PF
> exception.

It can happen to programs for tons of reasons.  It definitely happens
outside page faults.

> I see no reason to lie to user-space about the trap_nr that caused the
> SIGSEGV, especially since user-space software needs to be modified to
> make use of MPX, including the signal handler. So there is no risk of
> introducing any incompatibility or regression, no?

I think it's pretty safe to change.