Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1213721 > unrolled thread

Re: [PATCH v6 3/6] task_isolation: support PR_TASK_ISOLATION_STRICT mode

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 3/6] task_isolation: support PR_TASK_ISOLATION_STRICT  mode Will Deacon <will.deacon@arm.com> - 2015-08-26 12:40 +0200
    Re: [PATCH v6 3/6] task_isolation: support PR_TASK_ISOLATION_STRICT  mode Chris Metcalf <cmetcalf@ezchip.com> - 2015-08-26 17:20 +0200
      Re: [PATCH v6 3/6] task_isolation: support PR_TASK_ISOLATION_STRICT  mode Will Deacon <will.deacon@arm.com> - 2015-09-02 12:20 +0200

#1213721 — Re: [PATCH v6 3/6] task_isolation: support PR_TASK_ISOLATION_STRICT mode

Hi Chris,

On Tue, Aug 25, 2015 at 08:55:52PM +0100, Chris Metcalf wrote:
> With task_isolation mode, the task is in principle guaranteed not to
> be interrupted by the kernel, but only if it behaves.  In particular,
> if it enters the kernel via system call, page fault, or any of a
> number of other synchronous traps, it may be unexpectedly exposed
> to long latencies.  Add a simple flag that puts the process into
> a state where any such kernel entry is fatal.
> 
> To allow the state to be entered and exited, we ignore the prctl()
> syscall so that we can clear the bit again later, and we ignore
> exit/exit_group to allow exiting the task without a pointless signal
> killing you as you try to do so.
> 
> This change adds the syscall-detection hooks only for x86, arm64,
> and tile.
> 
> The signature of context_tracking_exit() changes to report whether
> we, in fact, are exiting back to user space, so that we can track
> user exceptions properly separately from other kernel entries.
> 
> Signed-off-by: Chris Metcalf <cmetcalf@ezchip.com>
> ---
>  arch/arm64/kernel/ptrace.c       |  5 +++++
>  arch/tile/kernel/ptrace.c        |  5 ++++-
>  arch/x86/kernel/ptrace.c         |  2 ++
>  include/linux/context_tracking.h | 11 ++++++++---
>  include/linux/isolation.h        | 16 ++++++++++++++++
>  include/uapi/linux/prctl.h       |  1 +
>  kernel/context_tracking.c        |  9 ++++++---
>  kernel/isolation.c               | 38 ++++++++++++++++++++++++++++++++++++++
>  8 files changed, 80 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index d882b833dbdb..e3d83a12f3cf 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -37,6 +37,7 @@
>  #include <linux/regset.h>
>  #include <linux/tracehook.h>
>  #include <linux/elf.h>
> +#include <linux/isolation.h>
>  
>  #include <asm/compat.h>
>  #include <asm/debug-monitors.h>
> @@ -1150,6 +1151,10 @@ static void tracehook_report_syscall(struct pt_regs *regs,
>  
>  asmlinkage int syscall_trace_enter(struct pt_regs *regs)
>  {
> +	/* Ensure we report task_isolation violations in all circumstances. */
> +	if (test_thread_flag(TIF_NOHZ) && task_isolation_strict())

This is going to force us to check TIF_NOHZ on the syscall slowpath even
when CONFIG_TASK_ISOLATION=n.

> +		task_isolation_syscall(regs->syscallno);
> +
>  	/* Do the secure computing check first; failures should be fast. */

Here we have the usual priority problems with all the subsystems that
hook into the syscall path. If a prctl is later rewritten to a different
syscall, do you care about catching it? Either way, the comment about
doing secure computing "first" needs fixing.

Cheers,

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1213955

On 08/26/2015 06:36 AM, Will Deacon wrote:
> Hi Chris,
>
> On Tue, Aug 25, 2015 at 08:55:52PM +0100, Chris Metcalf wrote:
>> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
>> index d882b833dbdb..e3d83a12f3cf 100644
>> --- a/arch/arm64/kernel/ptrace.c
>> +++ b/arch/arm64/kernel/ptrace.c
>> @@ -37,6 +37,7 @@
>>   #include <linux/regset.h>
>>   #include <linux/tracehook.h>
>>   #include <linux/elf.h>
>> +#include <linux/isolation.h>
>>   
>>   #include <asm/compat.h>
>>   #include <asm/debug-monitors.h>
>> @@ -1150,6 +1151,10 @@ static void tracehook_report_syscall(struct pt_regs *regs,
>>   
>>   asmlinkage int syscall_trace_enter(struct pt_regs *regs)
>>   {
>> +	/* Ensure we report task_isolation violations in all circumstances. */
>> +	if (test_thread_flag(TIF_NOHZ) && task_isolation_strict())
> This is going to force us to check TIF_NOHZ on the syscall slowpath even
> when CONFIG_TASK_ISOLATION=n.

Yes, good catch.  I was thinking the "&& false" would suppress the TIF
test but I forgot that test_bit() takes a volatile argument, so it gets
evaluated even though the result isn't actually used.

But I don't want to just reorder the two tests, because when isolation
is enabled, testing TIF_NOHZ first is better.  I think probably the right
solution is just to put an #ifdef CONFIG_TASK_ISOLATION around that
test, even though that is a little crufty.  The alternative is to provide
a task_isolation_configured() macro that just returns true or false, and
make it a three-part "&&" test with that new macro first, but
that seems a little crufty as well.  Do you have a preference?

>> +		task_isolation_syscall(regs->syscallno);
>> +
>>   	/* Do the secure computing check first; failures should be fast. */
> Here we have the usual priority problems with all the subsystems that
> hook into the syscall path. If a prctl is later rewritten to a different
> syscall, do you care about catching it? Either way, the comment about
> doing secure computing "first" needs fixing.

I admit I am unclear on the utility of rewriting prctl.  My instinct is that
we are trying to catch userspace invocations of prctl and allow them,
and fail most everything else, so doing it pre-rewrite seems OK.

I'm not sure if it makes sense to catch it before or after the
secure computing check, though.  On reflection maybe doing it
afterwards makes more sense - what do you think?

Thanks!

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1217489

On Wed, Aug 26, 2015 at 04:10:34PM +0100, Chris Metcalf wrote:
> On 08/26/2015 06:36 AM, Will Deacon wrote:
> > On Tue, Aug 25, 2015 at 08:55:52PM +0100, Chris Metcalf wrote:
> >> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> >> index d882b833dbdb..e3d83a12f3cf 100644
> >> --- a/arch/arm64/kernel/ptrace.c
> >> +++ b/arch/arm64/kernel/ptrace.c
> >> @@ -37,6 +37,7 @@
> >>   #include <linux/regset.h>
> >>   #include <linux/tracehook.h>
> >>   #include <linux/elf.h>
> >> +#include <linux/isolation.h>
> >>   
> >>   #include <asm/compat.h>
> >>   #include <asm/debug-monitors.h>
> >> @@ -1150,6 +1151,10 @@ static void tracehook_report_syscall(struct pt_regs *regs,
> >>   
> >>   asmlinkage int syscall_trace_enter(struct pt_regs *regs)
> >>   {
> >> +	/* Ensure we report task_isolation violations in all circumstances. */
> >> +	if (test_thread_flag(TIF_NOHZ) && task_isolation_strict())
> > This is going to force us to check TIF_NOHZ on the syscall slowpath even
> > when CONFIG_TASK_ISOLATION=n.
> 
> Yes, good catch.  I was thinking the "&& false" would suppress the TIF
> test but I forgot that test_bit() takes a volatile argument, so it gets
> evaluated even though the result isn't actually used.
> 
> But I don't want to just reorder the two tests, because when isolation
> is enabled, testing TIF_NOHZ first is better.  I think probably the right
> solution is just to put an #ifdef CONFIG_TASK_ISOLATION around that
> test, even though that is a little crufty.  The alternative is to provide
> a task_isolation_configured() macro that just returns true or false, and
> make it a three-part "&&" test with that new macro first, but
> that seems a little crufty as well.  Do you have a preference?

Maybe use IS_ENABLED(CONFIG_TASK_ISOLATION) ?

> >> +		task_isolation_syscall(regs->syscallno);
> >> +
> >>   	/* Do the secure computing check first; failures should be fast. */
> > Here we have the usual priority problems with all the subsystems that
> > hook into the syscall path. If a prctl is later rewritten to a different
> > syscall, do you care about catching it? Either way, the comment about
> > doing secure computing "first" needs fixing.
> 
> I admit I am unclear on the utility of rewriting prctl.  My instinct is that
> we are trying to catch userspace invocations of prctl and allow them,
> and fail most everything else, so doing it pre-rewrite seems OK.
> 
> I'm not sure if it makes sense to catch it before or after the
> secure computing check, though.  On reflection maybe doing it
> afterwards makes more sense - what do you think?

I don't have a strong preference (I really hate all these hooks we have
on the syscall entry/exit path), but we do need to make sure that the
behaviour is consistent across architectures.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web