From: Mark Rutland <mark.rutland@arm.com>
To: Laura Abbott <labbott@redhat.com>
Cc: Alexander Popov <alex.popov@linux.com>,
Kees Cook <keescook@chromium.org>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
kernel-hardening@lists.openwall.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Will Deacon <will.deacon@arm.com>,
Catalin Marinas <catalin.marinas@arm.com>,
james.morse@arm.com
Subject: Re: [PATCH 1/2] arm64: Introduce current_stack_type
Date: Thu, 19 Jul 2018 12:07:58 +0100 [thread overview]
Message-ID: <20180719110757.jh4cnitfrz6pasho@lakrids.cambridge.arm.com> (raw)
In-Reply-To: <20180718211013.14512-2-labbott@redhat.com>
Hi Laura,
On Wed, Jul 18, 2018 at 02:10:12PM -0700, Laura Abbott wrote:
>
> In preparation for enabling the stackleak plugin on arm64,
> we need a way to get the bounds of the current stack.
> Introduce a new primitive current_stack_type which is similar
> to x86's get_stack_info. Utilize that to rework
> on_accessible_stack slightly as well.
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> So this did end up looking quite a bit like get_stack_info but I didn't
> really see the need to integrate this more than this. I do think
> actually enumerating the types makes things a bit cleaner.
> ---
> arch/arm64/include/asm/sdei.h | 8 ++-
> arch/arm64/include/asm/stacktrace.h | 94 ++++++++++++++++++++++++-----
> arch/arm64/kernel/ptrace.c | 2 +-
> arch/arm64/kernel/sdei.c | 21 ++++++-
> 4 files changed, 103 insertions(+), 22 deletions(-)
>
> diff --git a/arch/arm64/include/asm/sdei.h b/arch/arm64/include/asm/sdei.h
> index e073e6886685..34f7b203845b 100644
> --- a/arch/arm64/include/asm/sdei.h
> +++ b/arch/arm64/include/asm/sdei.h
> @@ -40,15 +40,17 @@ asmlinkage unsigned long __sdei_handler(struct pt_regs *regs,
> unsigned long sdei_arch_get_entry_point(int conduit);
> #define sdei_arch_get_entry_point(x) sdei_arch_get_entry_point(x)
>
> -bool _on_sdei_stack(unsigned long sp);
> -static inline bool on_sdei_stack(unsigned long sp)
> +bool _on_sdei_stack(unsigned long sp, unsigned long *, unsigned long *);
> +static inline bool on_sdei_stack(unsigned long sp,
> + unsigned long *stack_low,
> + unsigned long *stack_high)
> {
> if (!IS_ENABLED(CONFIG_VMAP_STACK))
> return false;
> if (!IS_ENABLED(CONFIG_ARM_SDE_INTERFACE))
> return false;
> if (in_nmi())
> - return _on_sdei_stack(sp);
> + return _on_sdei_stack(sp, stack_low, stack_high);
>
> return false;
> }
> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
> index 902f9edacbea..9855a0425e64 100644
> --- a/arch/arm64/include/asm/stacktrace.h
> +++ b/arch/arm64/include/asm/stacktrace.h
> @@ -39,7 +39,9 @@ extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk);
>
> DECLARE_PER_CPU(unsigned long *, irq_stack_ptr);
>
> -static inline bool on_irq_stack(unsigned long sp)
> +static inline bool on_irq_stack(unsigned long sp,
> + unsigned long *stack_low,
> + unsigned long *stack_high)
> {
> unsigned long low = (unsigned long)raw_cpu_read(irq_stack_ptr);
> unsigned long high = low + IRQ_STACK_SIZE;
> @@ -47,47 +49,109 @@ static inline bool on_irq_stack(unsigned long sp)
> if (!low)
> return false;
>
> - return (low <= sp && sp < high);
> + if (sp < low || sp >= high)
> + return false;
> +
> + if (stack_low && stack_high) {
> + *stack_low = low;
> + *stack_high = high;
> + }
> +
> + return true;
> }
Rather than having to pass separete pointers to low/high, could we
please wrap them up as a struct, e.g.
struct stack_info {
unsigned long low, high;
stack_type type;
}
... and pass a single pointer to that? e.g.
static inline bool on_irq_stack(unsigned long sp, struct stack_info *info)
{
unsigned long low = (unsigned long)raw_cpu_read(irq_stack_ptr);
unsigned long high = low + IRQ_STACK_SIZE;
if (!low)
return false;
if (sp < low || sp >= high)
return false;
if (info) {
info->low = low;
info->high = high;
info->type = STACK_TYPE_IRQ;
}
return true;
}
That simplified the prototypes, and will allow us to distinguish the two
SDEI stacks (which we'll need for making stack unwiding robust to
cross-stack loops).
> -static inline bool on_task_stack(struct task_struct *tsk, unsigned long sp)
> +static inline bool on_task_stack(struct task_struct *tsk, unsigned long sp,
> + unsigned long *stack_low,
> + unsigned long *stack_high)
> {
> unsigned long low = (unsigned long)task_stack_page(tsk);
> unsigned long high = low + THREAD_SIZE;
>
> - return (low <= sp && sp < high);
> + if (sp < low || sp >= high)
> + return false;
> +
> + if (stack_low && stack_high) {
> + *stack_low = low;
> + *stack_high = high;
> + }
> +
> + return true;
> }
>
> #ifdef CONFIG_VMAP_STACK
> DECLARE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)], overflow_stack);
>
> -static inline bool on_overflow_stack(unsigned long sp)
> +static inline bool on_overflow_stack(unsigned long sp,
> + unsigned long *stack_low,
> + unsigned long *stack_high)
> {
> unsigned long low = (unsigned long)raw_cpu_ptr(overflow_stack);
> unsigned long high = low + OVERFLOW_STACK_SIZE;
>
> - return (low <= sp && sp < high);
> + if (sp < low || sp >= high)
> + return false;
> +
> + if (stack_low && stack_high) {
> + *stack_low = low;
> + *stack_high = high;
> + }
> +
> + return true;
> }
> #else
> -static inline bool on_overflow_stack(unsigned long sp) { return false; }
> +static inline bool on_overflow_stack(unsigned long sp,
> + unsigned long *stack_low,
> + unsigned long *stack_high) { return false; }
> #endif
>
> +enum stack_type {
> + STACK_TYPE_UNKNOWN,
> + STACK_TYPE_TASK,
> + STACK_TYPE_IRQ,
> + STACK_TYPE_OVERFLOW,
> + STACK_TYPE_SDEI,
> +};
For SDEI we'll need STACK_TYPE_SDEI_NORMAL and STACK_TYPE_SDEI_CRITICAL,
at least for stack unwinding.
> +
> +static inline enum stack_type current_stack_type(struct task_struct *tsk,
> + unsigned long sp,
> + unsigned long *stack_low,
> + unsigned long *stack_high)
> +{
> + if (on_task_stack(tsk, sp, stack_low, stack_high))
> + return STACK_TYPE_TASK;
> + if (on_irq_stack(sp, stack_low, stack_high))
> + return STACK_TYPE_IRQ;
> + if (on_overflow_stack(sp, stack_low, stack_high))
> + return STACK_TYPE_OVERFLOW;
> + if (on_sdei_stack(sp, stack_low, stack_high))
> + return STACK_TYPE_SDEI;
> + return STACK_TYPE_UNKNOWN;
> +}
> +
> /*
> * We can only safely access per-cpu stacks from current in a non-preemptible
> * context.
> */
> static inline bool on_accessible_stack(struct task_struct *tsk, unsigned long sp)
> {
> - if (on_task_stack(tsk, sp))
> + enum stack_type type;
> + unsigned long low, high;
> +
> + type = current_stack_type(tsk, sp, &low, &high);
> +
> + switch (type) {
> + case STACK_TYPE_TASK:
> return true;
> - if (tsk != current || preemptible())
> + case STACK_TYPE_IRQ:
> + case STACK_TYPE_OVERFLOW:
> + case STACK_TYPE_SDEI:
> + if (tsk != current || preemptible())
> + return false;
> + else
> + return true;
> + case STACK_TYPE_UNKNOWN:
> return false;
> - if (on_irq_stack(sp))
> - return true;
> - if (on_overflow_stack(sp))
> - return true;
> - if (on_sdei_stack(sp))
> - return true;
> + }
>
> return false;
> }
With the stacut stack_info, I think we can leave the logic of
on_accessible_stack() as-is, modulo a new info parameter that we pass on
into each on_<foo>_stack(), and then we don't neeed a separate
current_stack_type() function.
> diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> index 5c338ce5a7fa..a6b3a2be7561 100644
> --- a/arch/arm64/kernel/ptrace.c
> +++ b/arch/arm64/kernel/ptrace.c
> @@ -132,7 +132,7 @@ static bool regs_within_kernel_stack(struct pt_regs *regs, unsigned long addr)
> {
> return ((addr & ~(THREAD_SIZE - 1)) ==
> (kernel_stack_pointer(regs) & ~(THREAD_SIZE - 1))) ||
> - on_irq_stack(addr);
> + on_irq_stack(addr, NULL, NULL);
> }
>
> /**
> diff --git a/arch/arm64/kernel/sdei.c b/arch/arm64/kernel/sdei.c
> index 6b8d90d5ceae..8e18913a53fd 100644
> --- a/arch/arm64/kernel/sdei.c
> +++ b/arch/arm64/kernel/sdei.c
> @@ -88,7 +88,9 @@ static int init_sdei_stacks(void)
> return err;
> }
>
> -bool _on_sdei_stack(unsigned long sp)
> +bool _on_sdei_stack(unsigned long sp,
> + unsigned long *stack_low,
> + unsigned long *stack_high)
> {
> unsigned long low, high;
>
> @@ -98,13 +100,26 @@ bool _on_sdei_stack(unsigned long sp)
> low = (unsigned long)raw_cpu_read(sdei_stack_critical_ptr);
> high = low + SDEI_STACK_SIZE;
>
> - if (low <= sp && sp < high)
> + if (low <= sp && sp < high) {
> + if (stack_low && stack_high) {
> + *stack_low = low;
> + *stack_high = high;
> + }
> return true;
> + }
>
> low = (unsigned long)raw_cpu_read(sdei_stack_normal_ptr);
> high = low + SDEI_STACK_SIZE;
>
> - return (low <= sp && sp < high);
> + if (low <= sp && sp < high) {
> + if (stack_low && stack_high) {
> + *stack_low = low;
> + *stack_high = high;
> + }
> + return true;
> + }
> +
> + return false;
> }
We should probably split this into separate on_sdei_normal_stack() and
on_sdei_critical_stack() functions.
Then we can do:
bool on_sdei_<foo>_stack(...)
{
if (<out of bounds>)
return false;
if (info) {
<assign info fields>;
}
return true;
}
bool _on_sdei_stack(unsigned long sp, struct stack_info *info)
{
if (on_sedi_critical_stack(sp, info))
return true;
if (on_sdei_normal_stack(sp, info))
return true;
return false;
}
... which is a little nicer for legibility.
Otherwise, this looks good to me.
Thanks,
Mark.
next prev parent reply other threads:[~2018-07-19 11:08 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1531341400-12077-1-git-send-email-alex.popov@linux.com>
2018-07-18 21:10 ` [PATCH 0/2] Stackleak for arm64 Laura Abbott
2018-07-18 21:10 ` [PATCH 1/2] arm64: Introduce current_stack_type Laura Abbott
2018-07-19 11:07 ` Mark Rutland [this message]
2018-07-18 21:10 ` [PATCH 2/2] arm64: Clear the stack Laura Abbott
2018-07-19 2:20 ` Kees Cook
2018-07-19 10:41 ` Alexander Popov
2018-07-19 11:41 ` Mark Rutland
2018-07-19 23:28 ` [PATCHv2 0/2] Stackleak for arm64 Laura Abbott
2018-07-19 23:28 ` [PATCHv2 1/2] arm64: Add stack information to on_accessible_stack Laura Abbott
2018-07-20 6:38 ` Mark Rutland
2018-07-19 23:28 ` [PATCHv2 2/2] arm64: Clear the stack Laura Abbott
2018-07-20 4:33 ` Kees Cook
2018-07-20 6:39 ` Mark Rutland
2018-07-20 21:41 ` [PATCHv3 0/2] Stackleak for arm64 Laura Abbott
2018-07-20 21:41 ` [PATCHv3 1/2] arm64: Add stack information to on_accessible_stack Laura Abbott
2018-07-20 21:41 ` [PATCHv3 2/2] arm64: Add support for STACKLEAK gcc plugin Laura Abbott
2018-07-24 12:44 ` Alexander Popov
2018-07-24 16:35 ` Kees Cook
2018-07-24 16:38 ` [PATCHv3 0/2] Stackleak for arm64 Will Deacon
2018-07-25 11:49 ` Will Deacon
2018-07-25 22:05 ` Laura Abbott
2018-07-26 9:55 ` Will Deacon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180719110757.jh4cnitfrz6pasho@lakrids.cambridge.arm.com \
--to=mark.rutland@arm.com \
--cc=alex.popov@linux.com \
--cc=ard.biesheuvel@linaro.org \
--cc=catalin.marinas@arm.com \
--cc=james.morse@arm.com \
--cc=keescook@chromium.org \
--cc=kernel-hardening@lists.openwall.com \
--cc=labbott@redhat.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=will.deacon@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).