Re: [PATCH] arm64: Clear the stack

2018-07-19 Thread Mark Rutland
Hi,

On Tue, Jul 17, 2018 at 03:58:19PM -0700, Laura Abbott wrote:
> On 07/03/2018 05:14 AM, Mark Rutland wrote:
> > > It might be cleaner just to use on_accessible_stack and then another
> > > function to get the top of stack. This also might just be
> > > reimplementing what x86 already has? (Mark, Ard?)
> > It looks like we could build a get_stack_info() as they have.
> > 
> > We could probably clean up our stack traced atop of that, too.
> 
> So I spent some time looking at this and I'm not 100% clear
> if there would actually be much benefit to re-writing with
> get_stack_info. Most of that design seems to come from x86
> needing to handle multiple unwind options which arm64 doesn't
> need to worry about. Any rework ended up with roughly
> the same code without any notable benefit that I could see.
> It's possible I'm missing what kind of cleanup you're suggesting
> but I think just going with a tweaked version of on_accessible_stack
> would be fine.

I was mostly thinking that a struct stack_info with stack type
enumeration would also be helpful for ensuring that we terminated stack
traces when we had a loop.

I'll reply on your new thread.

Thanks,
Mark.


Re: [PATCH] arm64: Clear the stack

2018-07-19 Thread Mark Rutland
Hi,

On Tue, Jul 17, 2018 at 03:58:19PM -0700, Laura Abbott wrote:
> On 07/03/2018 05:14 AM, Mark Rutland wrote:
> > > It might be cleaner just to use on_accessible_stack and then another
> > > function to get the top of stack. This also might just be
> > > reimplementing what x86 already has? (Mark, Ard?)
> > It looks like we could build a get_stack_info() as they have.
> > 
> > We could probably clean up our stack traced atop of that, too.
> 
> So I spent some time looking at this and I'm not 100% clear
> if there would actually be much benefit to re-writing with
> get_stack_info. Most of that design seems to come from x86
> needing to handle multiple unwind options which arm64 doesn't
> need to worry about. Any rework ended up with roughly
> the same code without any notable benefit that I could see.
> It's possible I'm missing what kind of cleanup you're suggesting
> but I think just going with a tweaked version of on_accessible_stack
> would be fine.

I was mostly thinking that a struct stack_info with stack type
enumeration would also be helpful for ensuring that we terminated stack
traces when we had a loop.

I'll reply on your new thread.

Thanks,
Mark.


Re: [PATCH] arm64: Clear the stack

2018-07-17 Thread Laura Abbott

On 07/03/2018 05:14 AM, Mark Rutland wrote:

It might be cleaner just to use on_accessible_stack and then another
function to get the top of stack. This also might just be
reimplementing what x86 already has? (Mark, Ard?)

It looks like we could build a get_stack_info() as they have.

We could probably clean up our stack traced atop of that, too.


So I spent some time looking at this and I'm not 100% clear
if there would actually be much benefit to re-writing with
get_stack_info. Most of that design seems to come from x86
needing to handle multiple unwind options which arm64 doesn't
need to worry about. Any rework ended up with roughly
the same code without any notable benefit that I could see.
It's possible I'm missing what kind of cleanup you're suggesting
but I think just going with a tweaked version of on_accessible_stack
would be fine.

Thanks,
Laura


Re: [PATCH] arm64: Clear the stack

2018-07-17 Thread Laura Abbott

On 07/03/2018 05:14 AM, Mark Rutland wrote:

It might be cleaner just to use on_accessible_stack and then another
function to get the top of stack. This also might just be
reimplementing what x86 already has? (Mark, Ard?)

It looks like we could build a get_stack_info() as they have.

We could probably clean up our stack traced atop of that, too.


So I spent some time looking at this and I'm not 100% clear
if there would actually be much benefit to re-writing with
get_stack_info. Most of that design seems to come from x86
needing to handle multiple unwind options which arm64 doesn't
need to worry about. Any rework ended up with roughly
the same code without any notable benefit that I could see.
It's possible I'm missing what kind of cleanup you're suggesting
but I think just going with a tweaked version of on_accessible_stack
would be fine.

Thanks,
Laura


Re: [PATCH] arm64: Clear the stack

2018-07-12 Thread Will Deacon
On Wed, Jul 11, 2018 at 05:05:32PM -0700, Kees Cook wrote:
> On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott  wrote:
> >  include/linux/stackleak.h |  1 +
> > [...]
> > diff --git a/include/linux/stackleak.h b/include/linux/stackleak.h
> > index e2da99b3a191..00d62b302efb 100644
> > --- a/include/linux/stackleak.h
> > +++ b/include/linux/stackleak.h
> > @@ -5,6 +5,7 @@
> >  #include 
> >  #include 
> >
> > +#include 
> >  /*
> >   * Check that the poison value points to the unused hole in the
> >   * virtual memory map for your platform.
> 
> FYI, I squashed this change into my copy of the stackleak v14 series.
> (And I just sent this arm64 patch with the adjustments for it to be
> stand-alone.)

I can't find that -- can you point me to it, please?

Will


Re: [PATCH] arm64: Clear the stack

2018-07-12 Thread Will Deacon
On Wed, Jul 11, 2018 at 05:05:32PM -0700, Kees Cook wrote:
> On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott  wrote:
> >  include/linux/stackleak.h |  1 +
> > [...]
> > diff --git a/include/linux/stackleak.h b/include/linux/stackleak.h
> > index e2da99b3a191..00d62b302efb 100644
> > --- a/include/linux/stackleak.h
> > +++ b/include/linux/stackleak.h
> > @@ -5,6 +5,7 @@
> >  #include 
> >  #include 
> >
> > +#include 
> >  /*
> >   * Check that the poison value points to the unused hole in the
> >   * virtual memory map for your platform.
> 
> FYI, I squashed this change into my copy of the stackleak v14 series.
> (And I just sent this arm64 patch with the adjustments for it to be
> stand-alone.)

I can't find that -- can you point me to it, please?

Will


Re: [PATCH] arm64: Clear the stack

2018-07-11 Thread Kees Cook
On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott  wrote:
>  include/linux/stackleak.h |  1 +
> [...]
> diff --git a/include/linux/stackleak.h b/include/linux/stackleak.h
> index e2da99b3a191..00d62b302efb 100644
> --- a/include/linux/stackleak.h
> +++ b/include/linux/stackleak.h
> @@ -5,6 +5,7 @@
>  #include 
>  #include 
>
> +#include 
>  /*
>   * Check that the poison value points to the unused hole in the
>   * virtual memory map for your platform.

FYI, I squashed this change into my copy of the stackleak v14 series.
(And I just sent this arm64 patch with the adjustments for it to be
stand-alone.)

-Kees

-- 
Kees Cook
Pixel Security


Re: [PATCH] arm64: Clear the stack

2018-07-11 Thread Kees Cook
On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott  wrote:
>  include/linux/stackleak.h |  1 +
> [...]
> diff --git a/include/linux/stackleak.h b/include/linux/stackleak.h
> index e2da99b3a191..00d62b302efb 100644
> --- a/include/linux/stackleak.h
> +++ b/include/linux/stackleak.h
> @@ -5,6 +5,7 @@
>  #include 
>  #include 
>
> +#include 
>  /*
>   * Check that the poison value points to the unused hole in the
>   * virtual memory map for your platform.

FYI, I squashed this change into my copy of the stackleak v14 series.
(And I just sent this arm64 patch with the adjustments for it to be
stand-alone.)

-Kees

-- 
Kees Cook
Pixel Security


Re: [PATCH] arm64: Clear the stack

2018-07-04 Thread Will Deacon
Hi Kees,

On Mon, Jul 02, 2018 at 10:29:24AM -0700, Kees Cook wrote:
> On Mon, Jul 2, 2018 at 2:59 AM, Will Deacon  wrote:
> > On Fri, Jun 29, 2018 at 01:25:20PM -0700, Kees Cook wrote:
> >> No worries! I've made the change locally and will push this out to
> >> -next unless there are objections?
> >
> > I'm a bit wary of conflicts in entry.S, since it's likely that we're going
> > to have a lot going on in there for 4.19.
> >
> > Could I take this via arm64 instead, please, or are there dependencies
> > on other parts of your tree?
> 
> It depends on the plugin existing, but we could split it up so the
> arm64 part could go separately. It would just be a no-op in the arm64
> tree since CONFIG_GCC_PLUGIN_STACKLEAK won't exist there. Whatever
> works best for you!

If you could split it up then that would be really helpful, please.

Thanks,

Will


Re: [PATCH] arm64: Clear the stack

2018-07-04 Thread Will Deacon
Hi Kees,

On Mon, Jul 02, 2018 at 10:29:24AM -0700, Kees Cook wrote:
> On Mon, Jul 2, 2018 at 2:59 AM, Will Deacon  wrote:
> > On Fri, Jun 29, 2018 at 01:25:20PM -0700, Kees Cook wrote:
> >> No worries! I've made the change locally and will push this out to
> >> -next unless there are objections?
> >
> > I'm a bit wary of conflicts in entry.S, since it's likely that we're going
> > to have a lot going on in there for 4.19.
> >
> > Could I take this via arm64 instead, please, or are there dependencies
> > on other parts of your tree?
> 
> It depends on the plugin existing, but we could split it up so the
> arm64 part could go separately. It would just be a no-op in the arm64
> tree since CONFIG_GCC_PLUGIN_STACKLEAK won't exist there. Whatever
> works best for you!

If you could split it up then that would be really helpful, please.

Thanks,

Will


Re: [PATCH] arm64: Clear the stack

2018-07-03 Thread Alexander Popov
On 03.07.2018 18:03, Catalin Marinas wrote:
> On Tue, Jul 03, 2018 at 01:14:41PM +0100, Mark Rutland wrote:
>> On Mon, Jul 02, 2018 at 11:48:05AM -0700, Laura Abbott wrote:
>>> On 07/02/2018 06:02 AM, Alexander Popov wrote:
 Could you rename the macro to STACKLEAK_ERASE for similarity with x86?
>>>
>>> Mark Rutland had previously asked for this to be lowercase.
>>> I really don't care one way or the other so I'll defer to
>>> someone else to have the final word.
>>
>> Will, Catalin, could you chime in either way?
>>
>> I'd previously asked for lower-case for consistency with our other
>> assembly macros.
> 
> I'd keep it lowercase as the other arm64 macros in this file.

Ok, thanks, I'm fine with it.

Best regards,
Alexander


Re: [PATCH] arm64: Clear the stack

2018-07-03 Thread Alexander Popov
On 03.07.2018 18:03, Catalin Marinas wrote:
> On Tue, Jul 03, 2018 at 01:14:41PM +0100, Mark Rutland wrote:
>> On Mon, Jul 02, 2018 at 11:48:05AM -0700, Laura Abbott wrote:
>>> On 07/02/2018 06:02 AM, Alexander Popov wrote:
 Could you rename the macro to STACKLEAK_ERASE for similarity with x86?
>>>
>>> Mark Rutland had previously asked for this to be lowercase.
>>> I really don't care one way or the other so I'll defer to
>>> someone else to have the final word.
>>
>> Will, Catalin, could you chime in either way?
>>
>> I'd previously asked for lower-case for consistency with our other
>> assembly macros.
> 
> I'd keep it lowercase as the other arm64 macros in this file.

Ok, thanks, I'm fine with it.

Best regards,
Alexander


Re: [PATCH] arm64: Clear the stack

2018-07-03 Thread Catalin Marinas
On Tue, Jul 03, 2018 at 01:14:41PM +0100, Mark Rutland wrote:
> On Mon, Jul 02, 2018 at 11:48:05AM -0700, Laura Abbott wrote:
> > On 07/02/2018 06:02 AM, Alexander Popov wrote:
> > > On 29.06.2018 22:05, Laura Abbott wrote:
> > > > Implementation of stackleak based heavily on the x86 version
> > > > 
> > > > Signed-off-by: Laura Abbott 
> > > > ---
> > > > Changes since last time:
> > > > - Minor name change in entry.S
> > > > - Converted to use the generic interfaces so there's minimal additions.
> > > > - Added the fast syscall path.
> > > > - Addition of on_thread_stack and current_top_of_stack
> > > > - Disable stackleak on hyp per suggestion
> > > > - Added a define for check_alloca. I'm still not sure about keeping it
> > > >since the x86 version got reworked?
> > > > 
> > > > I've mostly kept this as one patch with a minimal commit text. I can
> > > > split it up and elaborate more before final merging.
> > > > ---
> 
> [...]
> 
> > > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > > > index ec2ee720e33e..31c9da7d401e 100644
> > > > --- a/arch/arm64/kernel/entry.S
> > > > +++ b/arch/arm64/kernel/entry.S
> > > > @@ -401,6 +401,11 @@ tsk.reqx28 // current 
> > > > thread_info
> > > > .text
> > > > +   .macro  stackleak_erase
> > > 
> > > Could you rename the macro to STACKLEAK_ERASE for similarity with x86?
> > > 
> > 
> > Mark Rutland had previously asked for this to be lowercase.
> > I really don't care one way or the other so I'll defer to
> > someone else to have the final word.
> 
> Will, Catalin, could you chime in either way?
> 
> I'd previously asked for lower-case for consistency with our other
> assembly macros.

I'd keep it lowercase as the other arm64 macros in this file.

-- 
Catalin


Re: [PATCH] arm64: Clear the stack

2018-07-03 Thread Catalin Marinas
On Tue, Jul 03, 2018 at 01:14:41PM +0100, Mark Rutland wrote:
> On Mon, Jul 02, 2018 at 11:48:05AM -0700, Laura Abbott wrote:
> > On 07/02/2018 06:02 AM, Alexander Popov wrote:
> > > On 29.06.2018 22:05, Laura Abbott wrote:
> > > > Implementation of stackleak based heavily on the x86 version
> > > > 
> > > > Signed-off-by: Laura Abbott 
> > > > ---
> > > > Changes since last time:
> > > > - Minor name change in entry.S
> > > > - Converted to use the generic interfaces so there's minimal additions.
> > > > - Added the fast syscall path.
> > > > - Addition of on_thread_stack and current_top_of_stack
> > > > - Disable stackleak on hyp per suggestion
> > > > - Added a define for check_alloca. I'm still not sure about keeping it
> > > >since the x86 version got reworked?
> > > > 
> > > > I've mostly kept this as one patch with a minimal commit text. I can
> > > > split it up and elaborate more before final merging.
> > > > ---
> 
> [...]
> 
> > > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > > > index ec2ee720e33e..31c9da7d401e 100644
> > > > --- a/arch/arm64/kernel/entry.S
> > > > +++ b/arch/arm64/kernel/entry.S
> > > > @@ -401,6 +401,11 @@ tsk.reqx28 // current 
> > > > thread_info
> > > > .text
> > > > +   .macro  stackleak_erase
> > > 
> > > Could you rename the macro to STACKLEAK_ERASE for similarity with x86?
> > > 
> > 
> > Mark Rutland had previously asked for this to be lowercase.
> > I really don't care one way or the other so I'll defer to
> > someone else to have the final word.
> 
> Will, Catalin, could you chime in either way?
> 
> I'd previously asked for lower-case for consistency with our other
> assembly macros.

I'd keep it lowercase as the other arm64 macros in this file.

-- 
Catalin


Re: [PATCH] arm64: Clear the stack

2018-07-03 Thread Mark Rutland
On Mon, Jul 02, 2018 at 11:48:05AM -0700, Laura Abbott wrote:
> On 07/02/2018 06:02 AM, Alexander Popov wrote:
> > On 29.06.2018 22:05, Laura Abbott wrote:
> > > Implementation of stackleak based heavily on the x86 version
> > > 
> > > Signed-off-by: Laura Abbott 
> > > ---
> > > Changes since last time:
> > > - Minor name change in entry.S
> > > - Converted to use the generic interfaces so there's minimal additions.
> > > - Added the fast syscall path.
> > > - Addition of on_thread_stack and current_top_of_stack
> > > - Disable stackleak on hyp per suggestion
> > > - Added a define for check_alloca. I'm still not sure about keeping it
> > >since the x86 version got reworked?
> > > 
> > > I've mostly kept this as one patch with a minimal commit text. I can
> > > split it up and elaborate more before final merging.
> > > ---

[...]

> > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > > index ec2ee720e33e..31c9da7d401e 100644
> > > --- a/arch/arm64/kernel/entry.S
> > > +++ b/arch/arm64/kernel/entry.S
> > > @@ -401,6 +401,11 @@ tsk  .reqx28 // current thread_info
> > >   .text
> > > + .macro  stackleak_erase
> > 
> > Could you rename the macro to STACKLEAK_ERASE for similarity with x86?
> > 
> 
> Mark Rutland had previously asked for this to be lowercase.
> I really don't care one way or the other so I'll defer to
> someone else to have the final word.

Will, Catalin, could you chime in either way?

I'd previously asked for lower-case for consistency with our other
assembly macros.

[...]

> > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > > index f08a2ed9db0d..9f0f135f8b66 100644
> > > --- a/arch/arm64/kernel/process.c
> > > +++ b/arch/arm64/kernel/process.c
> > > @@ -493,3 +493,19 @@ void arch_setup_new_exec(void)
> > >   {
> > >   current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 
> > > 0;
> > >   }
> > > +
> > > +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> > > +#define MIN_STACK_LEFT   256
> > > +
> > > +void __used stackleak_check_alloca(unsigned long size)
> > > +{
> > > + unsigned long sp, stack_left;
> > > +
> > > + sp = current_stack_pointer;
> > > +
> > > + stack_left = sp & (THREAD_SIZE - 1);
> > > + BUG_ON(stack_left < MIN_STACK_LEFT ||
> > > + size >= stack_left - MIN_STACK_LEFT);
> > > +}
> > > +EXPORT_SYMBOL(stackleak_check_alloca);
> > > +#endif
> > 
> > This code should be updated.
> > You may remember the troubles I had with MIN_STACK_LEFT:
> > http://openwall.com/lists/kernel-hardening/2018/05/11/12
> > Please see that thread where Mark Rutland and I worked out the solution.
> > 
> 
> Ah yeah, I missed the details in that thread. Thanks for
> that pointer.
> 
> > By the way, different stacks on x86_64 have different sizes. Is it false 
> > for arm64?
> 
> Currently everything except the overflow stack looks to be
> the same size but there's also another stack I missed.

Assuming I've followed the code correctly, we currently have:

stack   sizealignment (minimum)
---
taskTHREAD_SIZE THREAD_ALIGN
irq THREAD_SIZE 16
overflowSZ_4K   16
sdei_normal THREAD_SIZE THREAD_ALIGN
sdei_critical   THREAD_SIZE THREAD_ALIGN

... since IRQ_STACK_SIZE is defined as THREAD_SIZE, and SDEI_STACK_SIZE
is defined as IRQ_STACK_SIZE.

So we can't just mask the sp, unfortunately.

> It might be cleaner just to use on_accessible_stack and then another
> function to get the top of stack. This also might just be
> reimplementing what x86 already has? (Mark, Ard?)

It looks like we could build a get_stack_info() as they have.

We could probably clean up our stack traced atop of that, too.

Thanks,
Mark.


Re: [PATCH] arm64: Clear the stack

2018-07-03 Thread Mark Rutland
On Mon, Jul 02, 2018 at 11:48:05AM -0700, Laura Abbott wrote:
> On 07/02/2018 06:02 AM, Alexander Popov wrote:
> > On 29.06.2018 22:05, Laura Abbott wrote:
> > > Implementation of stackleak based heavily on the x86 version
> > > 
> > > Signed-off-by: Laura Abbott 
> > > ---
> > > Changes since last time:
> > > - Minor name change in entry.S
> > > - Converted to use the generic interfaces so there's minimal additions.
> > > - Added the fast syscall path.
> > > - Addition of on_thread_stack and current_top_of_stack
> > > - Disable stackleak on hyp per suggestion
> > > - Added a define for check_alloca. I'm still not sure about keeping it
> > >since the x86 version got reworked?
> > > 
> > > I've mostly kept this as one patch with a minimal commit text. I can
> > > split it up and elaborate more before final merging.
> > > ---

[...]

> > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > > index ec2ee720e33e..31c9da7d401e 100644
> > > --- a/arch/arm64/kernel/entry.S
> > > +++ b/arch/arm64/kernel/entry.S
> > > @@ -401,6 +401,11 @@ tsk  .reqx28 // current thread_info
> > >   .text
> > > + .macro  stackleak_erase
> > 
> > Could you rename the macro to STACKLEAK_ERASE for similarity with x86?
> > 
> 
> Mark Rutland had previously asked for this to be lowercase.
> I really don't care one way or the other so I'll defer to
> someone else to have the final word.

Will, Catalin, could you chime in either way?

I'd previously asked for lower-case for consistency with our other
assembly macros.

[...]

> > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > > index f08a2ed9db0d..9f0f135f8b66 100644
> > > --- a/arch/arm64/kernel/process.c
> > > +++ b/arch/arm64/kernel/process.c
> > > @@ -493,3 +493,19 @@ void arch_setup_new_exec(void)
> > >   {
> > >   current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 
> > > 0;
> > >   }
> > > +
> > > +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> > > +#define MIN_STACK_LEFT   256
> > > +
> > > +void __used stackleak_check_alloca(unsigned long size)
> > > +{
> > > + unsigned long sp, stack_left;
> > > +
> > > + sp = current_stack_pointer;
> > > +
> > > + stack_left = sp & (THREAD_SIZE - 1);
> > > + BUG_ON(stack_left < MIN_STACK_LEFT ||
> > > + size >= stack_left - MIN_STACK_LEFT);
> > > +}
> > > +EXPORT_SYMBOL(stackleak_check_alloca);
> > > +#endif
> > 
> > This code should be updated.
> > You may remember the troubles I had with MIN_STACK_LEFT:
> > http://openwall.com/lists/kernel-hardening/2018/05/11/12
> > Please see that thread where Mark Rutland and I worked out the solution.
> > 
> 
> Ah yeah, I missed the details in that thread. Thanks for
> that pointer.
> 
> > By the way, different stacks on x86_64 have different sizes. Is it false 
> > for arm64?
> 
> Currently everything except the overflow stack looks to be
> the same size but there's also another stack I missed.

Assuming I've followed the code correctly, we currently have:

stack   sizealignment (minimum)
---
taskTHREAD_SIZE THREAD_ALIGN
irq THREAD_SIZE 16
overflowSZ_4K   16
sdei_normal THREAD_SIZE THREAD_ALIGN
sdei_critical   THREAD_SIZE THREAD_ALIGN

... since IRQ_STACK_SIZE is defined as THREAD_SIZE, and SDEI_STACK_SIZE
is defined as IRQ_STACK_SIZE.

So we can't just mask the sp, unfortunately.

> It might be cleaner just to use on_accessible_stack and then another
> function to get the top of stack. This also might just be
> reimplementing what x86 already has? (Mark, Ard?)

It looks like we could build a get_stack_info() as they have.

We could probably clean up our stack traced atop of that, too.

Thanks,
Mark.


Re: [PATCH] arm64: Clear the stack

2018-07-02 Thread Laura Abbott

On 07/02/2018 06:02 AM, Alexander Popov wrote:

Hello Laura,

Thanks for your work!
Please see my comments below.

On 29.06.2018 22:05, Laura Abbott wrote:

Implementation of stackleak based heavily on the x86 version

Signed-off-by: Laura Abbott 
---
Changes since last time:
- Minor name change in entry.S
- Converted to use the generic interfaces so there's minimal additions.
- Added the fast syscall path.
- Addition of on_thread_stack and current_top_of_stack
- Disable stackleak on hyp per suggestion
- Added a define for check_alloca. I'm still not sure about keeping it
   since the x86 version got reworked?

I've mostly kept this as one patch with a minimal commit text. I can
split it up and elaborate more before final merging.
---
  arch/arm64/Kconfig|  1 +
  arch/arm64/include/asm/processor.h|  9 +
  arch/arm64/kernel/entry.S |  7 +++
  arch/arm64/kernel/process.c   | 16 
  arch/arm64/kvm/hyp/Makefile   |  3 ++-
  drivers/firmware/efi/libstub/Makefile |  3 ++-
  include/linux/stackleak.h |  1 +
  scripts/Makefile.gcc-plugins  |  5 -
  8 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index eb2cf4938f6d..b0221db95dc9 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -92,6 +92,7 @@ config ARM64
select HAVE_ARCH_MMAP_RND_BITS
select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
select HAVE_ARCH_SECCOMP_FILTER
+   select HAVE_ARCH_STACKLEAK
select HAVE_ARCH_THREAD_STRUCT_WHITELIST
select HAVE_ARCH_TRACEHOOK
select HAVE_ARCH_TRANSPARENT_HUGEPAGE
diff --git a/arch/arm64/include/asm/processor.h 
b/arch/arm64/include/asm/processor.h
index 767598932549..73914fd7e142 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -248,5 +248,14 @@ void cpu_clear_disr(const struct arm64_cpu_capabilities 
*__unused);
  #define SVE_SET_VL(arg)   sve_set_current_vl(arg)
  #define SVE_GET_VL()  sve_get_current_vl()
  
+/*

+ * For stackleak


May I ask you to call it STACKLEAK here and in other places for easier grep?



Sure


+ *
+ * These need to be macros because otherwise we get stuck in a nightmare
+ * of header definitions for the use of task_stack_page.
+ */
+#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE)
+#define on_thread_stack()  (on_task_stack(current, current_stack_pointer))
+
  #endif /* __ASSEMBLY__ */
  #endif /* __ASM_PROCESSOR_H */
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index ec2ee720e33e..31c9da7d401e 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -401,6 +401,11 @@ tsk.reqx28 // current thread_info
  
  	.text
  
+	.macro	stackleak_erase


Could you rename the macro to STACKLEAK_ERASE for similarity with x86?



Mark Rutland had previously asked for this to be lowercase.
I really don't care one way or the other so I'll defer to
someone else to have the final word.


+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+   bl  stackleak_erase_kstack
+#endif
+   .endm
  /*
   * Exception vectors.
   */
@@ -880,6 +885,7 @@ ret_fast_syscall:
and x2, x1, #_TIF_WORK_MASK
cbnzx2, work_pending
enable_step_tsk x1, x2
+   stackleak_erase
kernel_exit 0
  ret_fast_syscall_trace:
enable_daif
@@ -906,6 +912,7 @@ ret_to_user:
cbnzx2, work_pending
  finish_ret_to_user:
enable_step_tsk x1, x2
+   stackleak_erase
kernel_exit 0
  ENDPROC(ret_to_user)
  
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c

index f08a2ed9db0d..9f0f135f8b66 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -493,3 +493,19 @@ void arch_setup_new_exec(void)
  {
current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
  }
+
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+#define MIN_STACK_LEFT 256
+
+void __used stackleak_check_alloca(unsigned long size)
+{
+   unsigned long sp, stack_left;
+
+   sp = current_stack_pointer;
+
+   stack_left = sp & (THREAD_SIZE - 1);
+   BUG_ON(stack_left < MIN_STACK_LEFT ||
+   size >= stack_left - MIN_STACK_LEFT);
+}
+EXPORT_SYMBOL(stackleak_check_alloca);
+#endif


This code should be updated.
You may remember the troubles I had with MIN_STACK_LEFT:
http://openwall.com/lists/kernel-hardening/2018/05/11/12
Please see that thread where Mark Rutland and I worked out the solution.



Ah yeah, I missed the details in that thread. Thanks for
that pointer.


By the way, different stacks on x86_64 have different sizes. Is it false for 
arm64?



Currently everything except the overflow stack looks to be
the same size but there's also another stack I missed. It might
be cleaner just to use on_accessible_stack and then another
function to get the top of stack. This also might just be
reimplementing what x86 

Re: [PATCH] arm64: Clear the stack

2018-07-02 Thread Laura Abbott

On 07/02/2018 06:02 AM, Alexander Popov wrote:

Hello Laura,

Thanks for your work!
Please see my comments below.

On 29.06.2018 22:05, Laura Abbott wrote:

Implementation of stackleak based heavily on the x86 version

Signed-off-by: Laura Abbott 
---
Changes since last time:
- Minor name change in entry.S
- Converted to use the generic interfaces so there's minimal additions.
- Added the fast syscall path.
- Addition of on_thread_stack and current_top_of_stack
- Disable stackleak on hyp per suggestion
- Added a define for check_alloca. I'm still not sure about keeping it
   since the x86 version got reworked?

I've mostly kept this as one patch with a minimal commit text. I can
split it up and elaborate more before final merging.
---
  arch/arm64/Kconfig|  1 +
  arch/arm64/include/asm/processor.h|  9 +
  arch/arm64/kernel/entry.S |  7 +++
  arch/arm64/kernel/process.c   | 16 
  arch/arm64/kvm/hyp/Makefile   |  3 ++-
  drivers/firmware/efi/libstub/Makefile |  3 ++-
  include/linux/stackleak.h |  1 +
  scripts/Makefile.gcc-plugins  |  5 -
  8 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index eb2cf4938f6d..b0221db95dc9 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -92,6 +92,7 @@ config ARM64
select HAVE_ARCH_MMAP_RND_BITS
select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
select HAVE_ARCH_SECCOMP_FILTER
+   select HAVE_ARCH_STACKLEAK
select HAVE_ARCH_THREAD_STRUCT_WHITELIST
select HAVE_ARCH_TRACEHOOK
select HAVE_ARCH_TRANSPARENT_HUGEPAGE
diff --git a/arch/arm64/include/asm/processor.h 
b/arch/arm64/include/asm/processor.h
index 767598932549..73914fd7e142 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -248,5 +248,14 @@ void cpu_clear_disr(const struct arm64_cpu_capabilities 
*__unused);
  #define SVE_SET_VL(arg)   sve_set_current_vl(arg)
  #define SVE_GET_VL()  sve_get_current_vl()
  
+/*

+ * For stackleak


May I ask you to call it STACKLEAK here and in other places for easier grep?



Sure


+ *
+ * These need to be macros because otherwise we get stuck in a nightmare
+ * of header definitions for the use of task_stack_page.
+ */
+#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE)
+#define on_thread_stack()  (on_task_stack(current, current_stack_pointer))
+
  #endif /* __ASSEMBLY__ */
  #endif /* __ASM_PROCESSOR_H */
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index ec2ee720e33e..31c9da7d401e 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -401,6 +401,11 @@ tsk.reqx28 // current thread_info
  
  	.text
  
+	.macro	stackleak_erase


Could you rename the macro to STACKLEAK_ERASE for similarity with x86?



Mark Rutland had previously asked for this to be lowercase.
I really don't care one way or the other so I'll defer to
someone else to have the final word.


+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+   bl  stackleak_erase_kstack
+#endif
+   .endm
  /*
   * Exception vectors.
   */
@@ -880,6 +885,7 @@ ret_fast_syscall:
and x2, x1, #_TIF_WORK_MASK
cbnzx2, work_pending
enable_step_tsk x1, x2
+   stackleak_erase
kernel_exit 0
  ret_fast_syscall_trace:
enable_daif
@@ -906,6 +912,7 @@ ret_to_user:
cbnzx2, work_pending
  finish_ret_to_user:
enable_step_tsk x1, x2
+   stackleak_erase
kernel_exit 0
  ENDPROC(ret_to_user)
  
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c

index f08a2ed9db0d..9f0f135f8b66 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -493,3 +493,19 @@ void arch_setup_new_exec(void)
  {
current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
  }
+
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+#define MIN_STACK_LEFT 256
+
+void __used stackleak_check_alloca(unsigned long size)
+{
+   unsigned long sp, stack_left;
+
+   sp = current_stack_pointer;
+
+   stack_left = sp & (THREAD_SIZE - 1);
+   BUG_ON(stack_left < MIN_STACK_LEFT ||
+   size >= stack_left - MIN_STACK_LEFT);
+}
+EXPORT_SYMBOL(stackleak_check_alloca);
+#endif


This code should be updated.
You may remember the troubles I had with MIN_STACK_LEFT:
http://openwall.com/lists/kernel-hardening/2018/05/11/12
Please see that thread where Mark Rutland and I worked out the solution.



Ah yeah, I missed the details in that thread. Thanks for
that pointer.


By the way, different stacks on x86_64 have different sizes. Is it false for 
arm64?



Currently everything except the overflow stack looks to be
the same size but there's also another stack I missed. It might
be cleaner just to use on_accessible_stack and then another
function to get the top of stack. This also might just be
reimplementing what x86 

Re: [PATCH] arm64: Clear the stack

2018-07-02 Thread Kees Cook
Hi Will,

On Mon, Jul 2, 2018 at 2:59 AM, Will Deacon  wrote:
> On Fri, Jun 29, 2018 at 01:25:20PM -0700, Kees Cook wrote:
>> No worries! I've made the change locally and will push this out to
>> -next unless there are objections?
>
> I'm a bit wary of conflicts in entry.S, since it's likely that we're going
> to have a lot going on in there for 4.19.
>
> Could I take this via arm64 instead, please, or are there dependencies
> on other parts of your tree?

It depends on the plugin existing, but we could split it up so the
arm64 part could go separately. It would just be a no-op in the arm64
tree since CONFIG_GCC_PLUGIN_STACKLEAK won't exist there. Whatever
works best for you!

-Kees

-- 
Kees Cook
Pixel Security


Re: [PATCH] arm64: Clear the stack

2018-07-02 Thread Kees Cook
Hi Will,

On Mon, Jul 2, 2018 at 2:59 AM, Will Deacon  wrote:
> On Fri, Jun 29, 2018 at 01:25:20PM -0700, Kees Cook wrote:
>> No worries! I've made the change locally and will push this out to
>> -next unless there are objections?
>
> I'm a bit wary of conflicts in entry.S, since it's likely that we're going
> to have a lot going on in there for 4.19.
>
> Could I take this via arm64 instead, please, or are there dependencies
> on other parts of your tree?

It depends on the plugin existing, but we could split it up so the
arm64 part could go separately. It would just be a no-op in the arm64
tree since CONFIG_GCC_PLUGIN_STACKLEAK won't exist there. Whatever
works best for you!

-Kees

-- 
Kees Cook
Pixel Security


Re: [PATCH] arm64: Clear the stack

2018-07-02 Thread Alexander Popov
Hello Laura,

Thanks for your work!
Please see my comments below.

On 29.06.2018 22:05, Laura Abbott wrote:
> Implementation of stackleak based heavily on the x86 version
> 
> Signed-off-by: Laura Abbott 
> ---
> Changes since last time:
> - Minor name change in entry.S
> - Converted to use the generic interfaces so there's minimal additions.
> - Added the fast syscall path.
> - Addition of on_thread_stack and current_top_of_stack
> - Disable stackleak on hyp per suggestion
> - Added a define for check_alloca. I'm still not sure about keeping it
>   since the x86 version got reworked?
> 
> I've mostly kept this as one patch with a minimal commit text. I can
> split it up and elaborate more before final merging.
> ---
>  arch/arm64/Kconfig|  1 +
>  arch/arm64/include/asm/processor.h|  9 +
>  arch/arm64/kernel/entry.S |  7 +++
>  arch/arm64/kernel/process.c   | 16 
>  arch/arm64/kvm/hyp/Makefile   |  3 ++-
>  drivers/firmware/efi/libstub/Makefile |  3 ++-
>  include/linux/stackleak.h |  1 +
>  scripts/Makefile.gcc-plugins  |  5 -
>  8 files changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index eb2cf4938f6d..b0221db95dc9 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -92,6 +92,7 @@ config ARM64
>   select HAVE_ARCH_MMAP_RND_BITS
>   select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
>   select HAVE_ARCH_SECCOMP_FILTER
> + select HAVE_ARCH_STACKLEAK
>   select HAVE_ARCH_THREAD_STRUCT_WHITELIST
>   select HAVE_ARCH_TRACEHOOK
>   select HAVE_ARCH_TRANSPARENT_HUGEPAGE
> diff --git a/arch/arm64/include/asm/processor.h 
> b/arch/arm64/include/asm/processor.h
> index 767598932549..73914fd7e142 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -248,5 +248,14 @@ void cpu_clear_disr(const struct arm64_cpu_capabilities 
> *__unused);
>  #define SVE_SET_VL(arg)  sve_set_current_vl(arg)
>  #define SVE_GET_VL() sve_get_current_vl()
>  
> +/*
> + * For stackleak

May I ask you to call it STACKLEAK here and in other places for easier grep?

> + *
> + * These need to be macros because otherwise we get stuck in a nightmare
> + * of header definitions for the use of task_stack_page.
> + */
> +#define current_top_of_stack()   (task_stack_page(current) + THREAD_SIZE)
> +#define on_thread_stack()(on_task_stack(current, current_stack_pointer))
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* __ASM_PROCESSOR_H */
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index ec2ee720e33e..31c9da7d401e 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -401,6 +401,11 @@ tsk  .reqx28 // current thread_info
>  
>   .text
>  
> + .macro  stackleak_erase

Could you rename the macro to STACKLEAK_ERASE for similarity with x86?

> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> + bl  stackleak_erase_kstack
> +#endif
> + .endm
>  /*
>   * Exception vectors.
>   */
> @@ -880,6 +885,7 @@ ret_fast_syscall:
>   and x2, x1, #_TIF_WORK_MASK
>   cbnzx2, work_pending
>   enable_step_tsk x1, x2
> + stackleak_erase
>   kernel_exit 0
>  ret_fast_syscall_trace:
>   enable_daif
> @@ -906,6 +912,7 @@ ret_to_user:
>   cbnzx2, work_pending
>  finish_ret_to_user:
>   enable_step_tsk x1, x2
> + stackleak_erase
>   kernel_exit 0
>  ENDPROC(ret_to_user)
>  
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index f08a2ed9db0d..9f0f135f8b66 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -493,3 +493,19 @@ void arch_setup_new_exec(void)
>  {
>   current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
>  }
> +
> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> +#define MIN_STACK_LEFT   256
> +
> +void __used stackleak_check_alloca(unsigned long size)
> +{
> + unsigned long sp, stack_left;
> +
> + sp = current_stack_pointer;
> +
> + stack_left = sp & (THREAD_SIZE - 1);
> + BUG_ON(stack_left < MIN_STACK_LEFT ||
> + size >= stack_left - MIN_STACK_LEFT);
> +}
> +EXPORT_SYMBOL(stackleak_check_alloca);
> +#endif

This code should be updated.
You may remember the troubles I had with MIN_STACK_LEFT:
http://openwall.com/lists/kernel-hardening/2018/05/11/12
Please see that thread where Mark Rutland and I worked out the solution.

By the way, different stacks on x86_64 have different sizes. Is it false for 
arm64?

> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> index 4313f7475333..2fabc2dc1966 100644
> --- a/arch/arm64/kvm/hyp/Makefile
> +++ b/arch/arm64/kvm/hyp/Makefile
> @@ -3,7 +3,8 @@
>  # Makefile for Kernel-based Virtual Machine module, HYP part
>  #
>  
> -ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING
> +ccflags-y += -fno-stack-protector 

Re: [PATCH] arm64: Clear the stack

2018-07-02 Thread Alexander Popov
Hello Laura,

Thanks for your work!
Please see my comments below.

On 29.06.2018 22:05, Laura Abbott wrote:
> Implementation of stackleak based heavily on the x86 version
> 
> Signed-off-by: Laura Abbott 
> ---
> Changes since last time:
> - Minor name change in entry.S
> - Converted to use the generic interfaces so there's minimal additions.
> - Added the fast syscall path.
> - Addition of on_thread_stack and current_top_of_stack
> - Disable stackleak on hyp per suggestion
> - Added a define for check_alloca. I'm still not sure about keeping it
>   since the x86 version got reworked?
> 
> I've mostly kept this as one patch with a minimal commit text. I can
> split it up and elaborate more before final merging.
> ---
>  arch/arm64/Kconfig|  1 +
>  arch/arm64/include/asm/processor.h|  9 +
>  arch/arm64/kernel/entry.S |  7 +++
>  arch/arm64/kernel/process.c   | 16 
>  arch/arm64/kvm/hyp/Makefile   |  3 ++-
>  drivers/firmware/efi/libstub/Makefile |  3 ++-
>  include/linux/stackleak.h |  1 +
>  scripts/Makefile.gcc-plugins  |  5 -
>  8 files changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index eb2cf4938f6d..b0221db95dc9 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -92,6 +92,7 @@ config ARM64
>   select HAVE_ARCH_MMAP_RND_BITS
>   select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
>   select HAVE_ARCH_SECCOMP_FILTER
> + select HAVE_ARCH_STACKLEAK
>   select HAVE_ARCH_THREAD_STRUCT_WHITELIST
>   select HAVE_ARCH_TRACEHOOK
>   select HAVE_ARCH_TRANSPARENT_HUGEPAGE
> diff --git a/arch/arm64/include/asm/processor.h 
> b/arch/arm64/include/asm/processor.h
> index 767598932549..73914fd7e142 100644
> --- a/arch/arm64/include/asm/processor.h
> +++ b/arch/arm64/include/asm/processor.h
> @@ -248,5 +248,14 @@ void cpu_clear_disr(const struct arm64_cpu_capabilities 
> *__unused);
>  #define SVE_SET_VL(arg)  sve_set_current_vl(arg)
>  #define SVE_GET_VL() sve_get_current_vl()
>  
> +/*
> + * For stackleak

May I ask you to call it STACKLEAK here and in other places for easier grep?

> + *
> + * These need to be macros because otherwise we get stuck in a nightmare
> + * of header definitions for the use of task_stack_page.
> + */
> +#define current_top_of_stack()   (task_stack_page(current) + THREAD_SIZE)
> +#define on_thread_stack()(on_task_stack(current, current_stack_pointer))
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* __ASM_PROCESSOR_H */
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index ec2ee720e33e..31c9da7d401e 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -401,6 +401,11 @@ tsk  .reqx28 // current thread_info
>  
>   .text
>  
> + .macro  stackleak_erase

Could you rename the macro to STACKLEAK_ERASE for similarity with x86?

> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> + bl  stackleak_erase_kstack
> +#endif
> + .endm
>  /*
>   * Exception vectors.
>   */
> @@ -880,6 +885,7 @@ ret_fast_syscall:
>   and x2, x1, #_TIF_WORK_MASK
>   cbnzx2, work_pending
>   enable_step_tsk x1, x2
> + stackleak_erase
>   kernel_exit 0
>  ret_fast_syscall_trace:
>   enable_daif
> @@ -906,6 +912,7 @@ ret_to_user:
>   cbnzx2, work_pending
>  finish_ret_to_user:
>   enable_step_tsk x1, x2
> + stackleak_erase
>   kernel_exit 0
>  ENDPROC(ret_to_user)
>  
> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> index f08a2ed9db0d..9f0f135f8b66 100644
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -493,3 +493,19 @@ void arch_setup_new_exec(void)
>  {
>   current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
>  }
> +
> +#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> +#define MIN_STACK_LEFT   256
> +
> +void __used stackleak_check_alloca(unsigned long size)
> +{
> + unsigned long sp, stack_left;
> +
> + sp = current_stack_pointer;
> +
> + stack_left = sp & (THREAD_SIZE - 1);
> + BUG_ON(stack_left < MIN_STACK_LEFT ||
> + size >= stack_left - MIN_STACK_LEFT);
> +}
> +EXPORT_SYMBOL(stackleak_check_alloca);
> +#endif

This code should be updated.
You may remember the troubles I had with MIN_STACK_LEFT:
http://openwall.com/lists/kernel-hardening/2018/05/11/12
Please see that thread where Mark Rutland and I worked out the solution.

By the way, different stacks on x86_64 have different sizes. Is it false for 
arm64?

> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> index 4313f7475333..2fabc2dc1966 100644
> --- a/arch/arm64/kvm/hyp/Makefile
> +++ b/arch/arm64/kvm/hyp/Makefile
> @@ -3,7 +3,8 @@
>  # Makefile for Kernel-based Virtual Machine module, HYP part
>  #
>  
> -ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING
> +ccflags-y += -fno-stack-protector 

Re: [PATCH] arm64: Clear the stack

2018-07-02 Thread Will Deacon
Hi Kees,

On Fri, Jun 29, 2018 at 01:25:20PM -0700, Kees Cook wrote:
> On Fri, Jun 29, 2018 at 1:22 PM, Laura Abbott  wrote:
> > On 06/29/2018 01:19 PM, Kees Cook wrote:
> >>
> >> On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott  wrote:
> >>>
> >>> Implementation of stackleak based heavily on the x86 version
> >>>
> >>> Signed-off-by: Laura Abbott 
> >>> [...]
> >>> +#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE)
> >>> +#define on_thread_stack()  (on_task_stack(current,
> >>> current_stack_pointer))
> >>
> >>
> >> nit on types here. I get some warnings:
> >>
> >> kernel/stackleak.c:55:12: warning: assignment makes integer from
> >> pointer without a cast [-Wint-conversion]
> >> boundary = current_top_of_stack();
> >>  ^
> >> kernel/stackleak.c:65:24: warning: assignment makes integer from
> >> pointer without a cast [-Wint-conversion]
> >>current->lowest_stack = current_top_of_stack() - THREAD_SIZE / 64;
> >>  ^
> >>
> >> So I think this needs to be:
> >>
> >> +#define current_top_of_stack() ((unsigned long)task_stack_page(current) +
> >> \
> >> +THREAD_SIZE)
> >>
> >
> > Argh, missed that in an amend, can fix for next version if there
> > are no other objections to this approach.
> 
> No worries! I've made the change locally and will push this out to
> -next unless there are objections?

I'm a bit wary of conflicts in entry.S, since it's likely that we're going
to have a lot going on in there for 4.19.

Could I take this via arm64 instead, please, or are there dependencies
on other parts of your tree?

Will


Re: [PATCH] arm64: Clear the stack

2018-07-02 Thread Will Deacon
Hi Kees,

On Fri, Jun 29, 2018 at 01:25:20PM -0700, Kees Cook wrote:
> On Fri, Jun 29, 2018 at 1:22 PM, Laura Abbott  wrote:
> > On 06/29/2018 01:19 PM, Kees Cook wrote:
> >>
> >> On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott  wrote:
> >>>
> >>> Implementation of stackleak based heavily on the x86 version
> >>>
> >>> Signed-off-by: Laura Abbott 
> >>> [...]
> >>> +#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE)
> >>> +#define on_thread_stack()  (on_task_stack(current,
> >>> current_stack_pointer))
> >>
> >>
> >> nit on types here. I get some warnings:
> >>
> >> kernel/stackleak.c:55:12: warning: assignment makes integer from
> >> pointer without a cast [-Wint-conversion]
> >> boundary = current_top_of_stack();
> >>  ^
> >> kernel/stackleak.c:65:24: warning: assignment makes integer from
> >> pointer without a cast [-Wint-conversion]
> >>current->lowest_stack = current_top_of_stack() - THREAD_SIZE / 64;
> >>  ^
> >>
> >> So I think this needs to be:
> >>
> >> +#define current_top_of_stack() ((unsigned long)task_stack_page(current) +
> >> \
> >> +THREAD_SIZE)
> >>
> >
> > Argh, missed that in an amend, can fix for next version if there
> > are no other objections to this approach.
> 
> No worries! I've made the change locally and will push this out to
> -next unless there are objections?

I'm a bit wary of conflicts in entry.S, since it's likely that we're going
to have a lot going on in there for 4.19.

Could I take this via arm64 instead, please, or are there dependencies
on other parts of your tree?

Will


Re: [PATCH] arm64: Clear the stack

2018-06-29 Thread Kees Cook
On Fri, Jun 29, 2018 at 1:22 PM, Laura Abbott  wrote:
> On 06/29/2018 01:19 PM, Kees Cook wrote:
>>
>> On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott  wrote:
>>>
>>> Implementation of stackleak based heavily on the x86 version
>>>
>>> Signed-off-by: Laura Abbott 
>>> [...]
>>> +#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE)
>>> +#define on_thread_stack()  (on_task_stack(current,
>>> current_stack_pointer))
>>
>>
>> nit on types here. I get some warnings:
>>
>> kernel/stackleak.c:55:12: warning: assignment makes integer from
>> pointer without a cast [-Wint-conversion]
>> boundary = current_top_of_stack();
>>  ^
>> kernel/stackleak.c:65:24: warning: assignment makes integer from
>> pointer without a cast [-Wint-conversion]
>>current->lowest_stack = current_top_of_stack() - THREAD_SIZE / 64;
>>  ^
>>
>> So I think this needs to be:
>>
>> +#define current_top_of_stack() ((unsigned long)task_stack_page(current) +
>> \
>> +THREAD_SIZE)
>>
>
> Argh, missed that in an amend, can fix for next version if there
> are no other objections to this approach.

No worries! I've made the change locally and will push this out to
-next unless there are objections?

-Kees

-- 
Kees Cook
Pixel Security


Re: [PATCH] arm64: Clear the stack

2018-06-29 Thread Kees Cook
On Fri, Jun 29, 2018 at 1:22 PM, Laura Abbott  wrote:
> On 06/29/2018 01:19 PM, Kees Cook wrote:
>>
>> On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott  wrote:
>>>
>>> Implementation of stackleak based heavily on the x86 version
>>>
>>> Signed-off-by: Laura Abbott 
>>> [...]
>>> +#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE)
>>> +#define on_thread_stack()  (on_task_stack(current,
>>> current_stack_pointer))
>>
>>
>> nit on types here. I get some warnings:
>>
>> kernel/stackleak.c:55:12: warning: assignment makes integer from
>> pointer without a cast [-Wint-conversion]
>> boundary = current_top_of_stack();
>>  ^
>> kernel/stackleak.c:65:24: warning: assignment makes integer from
>> pointer without a cast [-Wint-conversion]
>>current->lowest_stack = current_top_of_stack() - THREAD_SIZE / 64;
>>  ^
>>
>> So I think this needs to be:
>>
>> +#define current_top_of_stack() ((unsigned long)task_stack_page(current) +
>> \
>> +THREAD_SIZE)
>>
>
> Argh, missed that in an amend, can fix for next version if there
> are no other objections to this approach.

No worries! I've made the change locally and will push this out to
-next unless there are objections?

-Kees

-- 
Kees Cook
Pixel Security


Re: [PATCH] arm64: Clear the stack

2018-06-29 Thread Laura Abbott

On 06/29/2018 01:19 PM, Kees Cook wrote:

On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott  wrote:

Implementation of stackleak based heavily on the x86 version

Signed-off-by: Laura Abbott 
[...]
+#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE)
+#define on_thread_stack()  (on_task_stack(current, current_stack_pointer))


nit on types here. I get some warnings:

kernel/stackleak.c:55:12: warning: assignment makes integer from
pointer without a cast [-Wint-conversion]
boundary = current_top_of_stack();
 ^
kernel/stackleak.c:65:24: warning: assignment makes integer from
pointer without a cast [-Wint-conversion]
   current->lowest_stack = current_top_of_stack() - THREAD_SIZE / 64;
 ^

So I think this needs to be:

+#define current_top_of_stack() ((unsigned long)task_stack_page(current) + \
+THREAD_SIZE)



Argh, missed that in an amend, can fix for next version if there
are no other objections to this approach.


diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins
index a535742a1c06..972ce4ca7f6a 100644
--- a/scripts/Makefile.gcc-plugins
+++ b/scripts/Makefile.gcc-plugins
@@ -37,11 +37,14 @@ ifdef CONFIG_GCC_PLUGINS

gcc-plugin-$(CONFIG_GCC_PLUGIN_STACKLEAK)+= stackleak_plugin.so
gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK) += -DSTACKLEAK_PLUGIN 
-fplugin-arg-stackleak_plugin-track-min-size=$(CONFIG_STACKLEAK_TRACK_MIN_SIZE)
+  ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+DISABLE_STACKLEAK_PLUGIN   += -fplugin-arg-stackleak_plugin-disable
+  endif

GCC_PLUGINS_CFLAGS := $(strip $(addprefix 
-fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y)) 
$(gcc-plugin-cflags-y))

export PLUGINCC GCC_PLUGINS_CFLAGS GCC_PLUGIN GCC_PLUGIN_SUBDIR
-  export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN
+  export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN DISABLE_STACKLEAK_PLUGIN

ifneq ($(PLUGINCC),)
  # SANCOV_PLUGIN can be only in CFLAGS_KCOV because avoid duplication.


If there is a v14, I think this hunk should be taken there, since it's
part of the common code.

Otherwise, this works for me and passes the lkdtm tests.

-Kees



Thanks,
Laura


Re: [PATCH] arm64: Clear the stack

2018-06-29 Thread Laura Abbott

On 06/29/2018 01:19 PM, Kees Cook wrote:

On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott  wrote:

Implementation of stackleak based heavily on the x86 version

Signed-off-by: Laura Abbott 
[...]
+#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE)
+#define on_thread_stack()  (on_task_stack(current, current_stack_pointer))


nit on types here. I get some warnings:

kernel/stackleak.c:55:12: warning: assignment makes integer from
pointer without a cast [-Wint-conversion]
boundary = current_top_of_stack();
 ^
kernel/stackleak.c:65:24: warning: assignment makes integer from
pointer without a cast [-Wint-conversion]
   current->lowest_stack = current_top_of_stack() - THREAD_SIZE / 64;
 ^

So I think this needs to be:

+#define current_top_of_stack() ((unsigned long)task_stack_page(current) + \
+THREAD_SIZE)



Argh, missed that in an amend, can fix for next version if there
are no other objections to this approach.


diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins
index a535742a1c06..972ce4ca7f6a 100644
--- a/scripts/Makefile.gcc-plugins
+++ b/scripts/Makefile.gcc-plugins
@@ -37,11 +37,14 @@ ifdef CONFIG_GCC_PLUGINS

gcc-plugin-$(CONFIG_GCC_PLUGIN_STACKLEAK)+= stackleak_plugin.so
gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK) += -DSTACKLEAK_PLUGIN 
-fplugin-arg-stackleak_plugin-track-min-size=$(CONFIG_STACKLEAK_TRACK_MIN_SIZE)
+  ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+DISABLE_STACKLEAK_PLUGIN   += -fplugin-arg-stackleak_plugin-disable
+  endif

GCC_PLUGINS_CFLAGS := $(strip $(addprefix 
-fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y)) 
$(gcc-plugin-cflags-y))

export PLUGINCC GCC_PLUGINS_CFLAGS GCC_PLUGIN GCC_PLUGIN_SUBDIR
-  export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN
+  export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN DISABLE_STACKLEAK_PLUGIN

ifneq ($(PLUGINCC),)
  # SANCOV_PLUGIN can be only in CFLAGS_KCOV because avoid duplication.


If there is a v14, I think this hunk should be taken there, since it's
part of the common code.

Otherwise, this works for me and passes the lkdtm tests.

-Kees



Thanks,
Laura


Re: [PATCH] arm64: Clear the stack

2018-06-29 Thread Kees Cook
On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott  wrote:
> Implementation of stackleak based heavily on the x86 version
>
> Signed-off-by: Laura Abbott 
> [...]
> +#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE)
> +#define on_thread_stack()  (on_task_stack(current, 
> current_stack_pointer))

nit on types here. I get some warnings:

kernel/stackleak.c:55:12: warning: assignment makes integer from
pointer without a cast [-Wint-conversion]
   boundary = current_top_of_stack();
^
kernel/stackleak.c:65:24: warning: assignment makes integer from
pointer without a cast [-Wint-conversion]
  current->lowest_stack = current_top_of_stack() - THREAD_SIZE / 64;
^

So I think this needs to be:

+#define current_top_of_stack() ((unsigned long)task_stack_page(current) + \
+THREAD_SIZE)

> diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins
> index a535742a1c06..972ce4ca7f6a 100644
> --- a/scripts/Makefile.gcc-plugins
> +++ b/scripts/Makefile.gcc-plugins
> @@ -37,11 +37,14 @@ ifdef CONFIG_GCC_PLUGINS
>
>gcc-plugin-$(CONFIG_GCC_PLUGIN_STACKLEAK)+= stackleak_plugin.so
>gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK) += -DSTACKLEAK_PLUGIN 
> -fplugin-arg-stackleak_plugin-track-min-size=$(CONFIG_STACKLEAK_TRACK_MIN_SIZE)
> +  ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> +DISABLE_STACKLEAK_PLUGIN   += 
> -fplugin-arg-stackleak_plugin-disable
> +  endif
>
>GCC_PLUGINS_CFLAGS := $(strip $(addprefix 
> -fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y)) 
> $(gcc-plugin-cflags-y))
>
>export PLUGINCC GCC_PLUGINS_CFLAGS GCC_PLUGIN GCC_PLUGIN_SUBDIR
> -  export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN
> +  export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN DISABLE_STACKLEAK_PLUGIN
>
>ifneq ($(PLUGINCC),)
>  # SANCOV_PLUGIN can be only in CFLAGS_KCOV because avoid duplication.

If there is a v14, I think this hunk should be taken there, since it's
part of the common code.

Otherwise, this works for me and passes the lkdtm tests.

-Kees

-- 
Kees Cook
Pixel Security


Re: [PATCH] arm64: Clear the stack

2018-06-29 Thread Kees Cook
On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott  wrote:
> Implementation of stackleak based heavily on the x86 version
>
> Signed-off-by: Laura Abbott 
> [...]
> +#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE)
> +#define on_thread_stack()  (on_task_stack(current, 
> current_stack_pointer))

nit on types here. I get some warnings:

kernel/stackleak.c:55:12: warning: assignment makes integer from
pointer without a cast [-Wint-conversion]
   boundary = current_top_of_stack();
^
kernel/stackleak.c:65:24: warning: assignment makes integer from
pointer without a cast [-Wint-conversion]
  current->lowest_stack = current_top_of_stack() - THREAD_SIZE / 64;
^

So I think this needs to be:

+#define current_top_of_stack() ((unsigned long)task_stack_page(current) + \
+THREAD_SIZE)

> diff --git a/scripts/Makefile.gcc-plugins b/scripts/Makefile.gcc-plugins
> index a535742a1c06..972ce4ca7f6a 100644
> --- a/scripts/Makefile.gcc-plugins
> +++ b/scripts/Makefile.gcc-plugins
> @@ -37,11 +37,14 @@ ifdef CONFIG_GCC_PLUGINS
>
>gcc-plugin-$(CONFIG_GCC_PLUGIN_STACKLEAK)+= stackleak_plugin.so
>gcc-plugin-cflags-$(CONFIG_GCC_PLUGIN_STACKLEAK) += -DSTACKLEAK_PLUGIN 
> -fplugin-arg-stackleak_plugin-track-min-size=$(CONFIG_STACKLEAK_TRACK_MIN_SIZE)
> +  ifdef CONFIG_GCC_PLUGIN_STACKLEAK
> +DISABLE_STACKLEAK_PLUGIN   += 
> -fplugin-arg-stackleak_plugin-disable
> +  endif
>
>GCC_PLUGINS_CFLAGS := $(strip $(addprefix 
> -fplugin=$(objtree)/scripts/gcc-plugins/, $(gcc-plugin-y)) 
> $(gcc-plugin-cflags-y))
>
>export PLUGINCC GCC_PLUGINS_CFLAGS GCC_PLUGIN GCC_PLUGIN_SUBDIR
> -  export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN
> +  export SANCOV_PLUGIN DISABLE_LATENT_ENTROPY_PLUGIN DISABLE_STACKLEAK_PLUGIN
>
>ifneq ($(PLUGINCC),)
>  # SANCOV_PLUGIN can be only in CFLAGS_KCOV because avoid duplication.

If there is a v14, I think this hunk should be taken there, since it's
part of the common code.

Otherwise, this works for me and passes the lkdtm tests.

-Kees

-- 
Kees Cook
Pixel Security


Re: [PATCH] arm64: Clear the stack

2018-06-29 Thread Kees Cook
On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott  wrote:
> Implementation of stackleak based heavily on the x86 version

Awesome!

Now I just have to figure out how to unbreak cross-compilation after
the kconfig changes to gcc-plugins. Whoops. :)

-Kees

-- 
Kees Cook
Pixel Security


Re: [PATCH] arm64: Clear the stack

2018-06-29 Thread Kees Cook
On Fri, Jun 29, 2018 at 12:05 PM, Laura Abbott  wrote:
> Implementation of stackleak based heavily on the x86 version

Awesome!

Now I just have to figure out how to unbreak cross-compilation after
the kconfig changes to gcc-plugins. Whoops. :)

-Kees

-- 
Kees Cook
Pixel Security


[PATCH] arm64: Clear the stack

2018-06-29 Thread Laura Abbott
Implementation of stackleak based heavily on the x86 version

Signed-off-by: Laura Abbott 
---
Changes since last time:
- Minor name change in entry.S
- Converted to use the generic interfaces so there's minimal additions.
- Added the fast syscall path.
- Addition of on_thread_stack and current_top_of_stack
- Disable stackleak on hyp per suggestion
- Added a define for check_alloca. I'm still not sure about keeping it
  since the x86 version got reworked?

I've mostly kept this as one patch with a minimal commit text. I can
split it up and elaborate more before final merging.
---
 arch/arm64/Kconfig|  1 +
 arch/arm64/include/asm/processor.h|  9 +
 arch/arm64/kernel/entry.S |  7 +++
 arch/arm64/kernel/process.c   | 16 
 arch/arm64/kvm/hyp/Makefile   |  3 ++-
 drivers/firmware/efi/libstub/Makefile |  3 ++-
 include/linux/stackleak.h |  1 +
 scripts/Makefile.gcc-plugins  |  5 -
 8 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index eb2cf4938f6d..b0221db95dc9 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -92,6 +92,7 @@ config ARM64
select HAVE_ARCH_MMAP_RND_BITS
select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
select HAVE_ARCH_SECCOMP_FILTER
+   select HAVE_ARCH_STACKLEAK
select HAVE_ARCH_THREAD_STRUCT_WHITELIST
select HAVE_ARCH_TRACEHOOK
select HAVE_ARCH_TRANSPARENT_HUGEPAGE
diff --git a/arch/arm64/include/asm/processor.h 
b/arch/arm64/include/asm/processor.h
index 767598932549..73914fd7e142 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -248,5 +248,14 @@ void cpu_clear_disr(const struct arm64_cpu_capabilities 
*__unused);
 #define SVE_SET_VL(arg)sve_set_current_vl(arg)
 #define SVE_GET_VL()   sve_get_current_vl()
 
+/*
+ * For stackleak
+ *
+ * These need to be macros because otherwise we get stuck in a nightmare
+ * of header definitions for the use of task_stack_page.
+ */
+#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE)
+#define on_thread_stack()  (on_task_stack(current, current_stack_pointer))
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_PROCESSOR_H */
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index ec2ee720e33e..31c9da7d401e 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -401,6 +401,11 @@ tsk.reqx28 // current thread_info
 
.text
 
+   .macro  stackleak_erase
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+   bl  stackleak_erase_kstack
+#endif
+   .endm
 /*
  * Exception vectors.
  */
@@ -880,6 +885,7 @@ ret_fast_syscall:
and x2, x1, #_TIF_WORK_MASK
cbnzx2, work_pending
enable_step_tsk x1, x2
+   stackleak_erase
kernel_exit 0
 ret_fast_syscall_trace:
enable_daif
@@ -906,6 +912,7 @@ ret_to_user:
cbnzx2, work_pending
 finish_ret_to_user:
enable_step_tsk x1, x2
+   stackleak_erase
kernel_exit 0
 ENDPROC(ret_to_user)
 
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index f08a2ed9db0d..9f0f135f8b66 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -493,3 +493,19 @@ void arch_setup_new_exec(void)
 {
current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
 }
+
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+#define MIN_STACK_LEFT 256
+
+void __used stackleak_check_alloca(unsigned long size)
+{
+   unsigned long sp, stack_left;
+
+   sp = current_stack_pointer;
+
+   stack_left = sp & (THREAD_SIZE - 1);
+   BUG_ON(stack_left < MIN_STACK_LEFT ||
+   size >= stack_left - MIN_STACK_LEFT);
+}
+EXPORT_SYMBOL(stackleak_check_alloca);
+#endif
diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
index 4313f7475333..2fabc2dc1966 100644
--- a/arch/arm64/kvm/hyp/Makefile
+++ b/arch/arm64/kvm/hyp/Makefile
@@ -3,7 +3,8 @@
 # Makefile for Kernel-based Virtual Machine module, HYP part
 #
 
-ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING
+ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING \
+   $(DISABLE_STACKLEAK_PLUGIN)
 
 KVM=../../../../virt/kvm
 
diff --git a/drivers/firmware/efi/libstub/Makefile 
b/drivers/firmware/efi/libstub/Makefile
index a34e9290a699..25dd2a14560d 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -20,7 +20,8 @@ cflags-$(CONFIG_EFI_ARMSTUB)  += 
-I$(srctree)/scripts/dtc/libfdt
 KBUILD_CFLAGS  := $(cflags-y) -DDISABLE_BRANCH_PROFILING \
   -D__NO_FORTIFY \
   $(call cc-option,-ffreestanding) \
-  $(call cc-option,-fno-stack-protector)
+  $(call cc-option,-fno-stack-protector) \
+  

[PATCH] arm64: Clear the stack

2018-06-29 Thread Laura Abbott
Implementation of stackleak based heavily on the x86 version

Signed-off-by: Laura Abbott 
---
Changes since last time:
- Minor name change in entry.S
- Converted to use the generic interfaces so there's minimal additions.
- Added the fast syscall path.
- Addition of on_thread_stack and current_top_of_stack
- Disable stackleak on hyp per suggestion
- Added a define for check_alloca. I'm still not sure about keeping it
  since the x86 version got reworked?

I've mostly kept this as one patch with a minimal commit text. I can
split it up and elaborate more before final merging.
---
 arch/arm64/Kconfig|  1 +
 arch/arm64/include/asm/processor.h|  9 +
 arch/arm64/kernel/entry.S |  7 +++
 arch/arm64/kernel/process.c   | 16 
 arch/arm64/kvm/hyp/Makefile   |  3 ++-
 drivers/firmware/efi/libstub/Makefile |  3 ++-
 include/linux/stackleak.h |  1 +
 scripts/Makefile.gcc-plugins  |  5 -
 8 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index eb2cf4938f6d..b0221db95dc9 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -92,6 +92,7 @@ config ARM64
select HAVE_ARCH_MMAP_RND_BITS
select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
select HAVE_ARCH_SECCOMP_FILTER
+   select HAVE_ARCH_STACKLEAK
select HAVE_ARCH_THREAD_STRUCT_WHITELIST
select HAVE_ARCH_TRACEHOOK
select HAVE_ARCH_TRANSPARENT_HUGEPAGE
diff --git a/arch/arm64/include/asm/processor.h 
b/arch/arm64/include/asm/processor.h
index 767598932549..73914fd7e142 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -248,5 +248,14 @@ void cpu_clear_disr(const struct arm64_cpu_capabilities 
*__unused);
 #define SVE_SET_VL(arg)sve_set_current_vl(arg)
 #define SVE_GET_VL()   sve_get_current_vl()
 
+/*
+ * For stackleak
+ *
+ * These need to be macros because otherwise we get stuck in a nightmare
+ * of header definitions for the use of task_stack_page.
+ */
+#define current_top_of_stack() (task_stack_page(current) + THREAD_SIZE)
+#define on_thread_stack()  (on_task_stack(current, current_stack_pointer))
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_PROCESSOR_H */
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index ec2ee720e33e..31c9da7d401e 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -401,6 +401,11 @@ tsk.reqx28 // current thread_info
 
.text
 
+   .macro  stackleak_erase
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+   bl  stackleak_erase_kstack
+#endif
+   .endm
 /*
  * Exception vectors.
  */
@@ -880,6 +885,7 @@ ret_fast_syscall:
and x2, x1, #_TIF_WORK_MASK
cbnzx2, work_pending
enable_step_tsk x1, x2
+   stackleak_erase
kernel_exit 0
 ret_fast_syscall_trace:
enable_daif
@@ -906,6 +912,7 @@ ret_to_user:
cbnzx2, work_pending
 finish_ret_to_user:
enable_step_tsk x1, x2
+   stackleak_erase
kernel_exit 0
 ENDPROC(ret_to_user)
 
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index f08a2ed9db0d..9f0f135f8b66 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -493,3 +493,19 @@ void arch_setup_new_exec(void)
 {
current->mm->context.flags = is_compat_task() ? MMCF_AARCH32 : 0;
 }
+
+#ifdef CONFIG_GCC_PLUGIN_STACKLEAK
+#define MIN_STACK_LEFT 256
+
+void __used stackleak_check_alloca(unsigned long size)
+{
+   unsigned long sp, stack_left;
+
+   sp = current_stack_pointer;
+
+   stack_left = sp & (THREAD_SIZE - 1);
+   BUG_ON(stack_left < MIN_STACK_LEFT ||
+   size >= stack_left - MIN_STACK_LEFT);
+}
+EXPORT_SYMBOL(stackleak_check_alloca);
+#endif
diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
index 4313f7475333..2fabc2dc1966 100644
--- a/arch/arm64/kvm/hyp/Makefile
+++ b/arch/arm64/kvm/hyp/Makefile
@@ -3,7 +3,8 @@
 # Makefile for Kernel-based Virtual Machine module, HYP part
 #
 
-ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING
+ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING \
+   $(DISABLE_STACKLEAK_PLUGIN)
 
 KVM=../../../../virt/kvm
 
diff --git a/drivers/firmware/efi/libstub/Makefile 
b/drivers/firmware/efi/libstub/Makefile
index a34e9290a699..25dd2a14560d 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -20,7 +20,8 @@ cflags-$(CONFIG_EFI_ARMSTUB)  += 
-I$(srctree)/scripts/dtc/libfdt
 KBUILD_CFLAGS  := $(cflags-y) -DDISABLE_BRANCH_PROFILING \
   -D__NO_FORTIFY \
   $(call cc-option,-ffreestanding) \
-  $(call cc-option,-fno-stack-protector)
+  $(call cc-option,-fno-stack-protector) \
+