linux-kernel - Re: [PATCH v2] ARM: kprobes: Avoid fortify_panic() when copying optprobe template

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACPK8XeJzHOH-ErWB8ZZS1rMVcPJx0exjnB-va-vhuvGvwFWCQ@mail.gmail.com>
Date:   Tue, 20 Oct 2020 05:32:26 +0000
From:   Joel Stanley <joel@....id.au>
To:     Andrew Jeffery <andrew@...id.au>,
        Russell King <linux@...linux.org.uk>
Cc:     Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        mhiramat@...nel.org, labbott@...hat.com,
        Kees Cook <keescook@...omium.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Luka Oreskovic <luka.oreskovic@...tura.hr>,
        Juraj Vijtiuk <juraj.vijtiuk@...tura.hr>
Subject: Re: [PATCH v2] ARM: kprobes: Avoid fortify_panic() when copying
 optprobe template

On Fri, 9 Oct 2020 at 05:20, Joel Stanley <joel@....id.au> wrote:
>
> On Thu, 1 Oct 2020 at 04:30, Andrew Jeffery <andrew@...id.au> wrote:
> >
> > Setting both CONFIG_KPROBES=y and CONFIG_FORTIFY_SOURCE=y on ARM leads
> > to a panic in memcpy() when injecting a kprobe despite the fixes found
> > in commit e46daee53bb5 ("ARM: 8806/1: kprobes: Fix false positive with
> > FORTIFY_SOURCE") and commit 0ac569bf6a79 ("ARM: 8834/1: Fix: kprobes:
> > optimized kprobes illegal instruction").
> >
> > arch/arm/include/asm/kprobes.h effectively declares
> > the target type of the optprobe_template_entry assembly label as a u32
> > which leads memcpy()'s __builtin_object_size() call to determine that
> > the pointed-to object is of size four. However, the symbol is used as a handle
> > for the optimised probe assembly template that is at least 96 bytes in size.
> > The symbol's use despite its type blows up the memcpy() in ARM's
> > arch_prepare_optimized_kprobe() with a false-positive fortify_panic() when it
> > should instead copy the optimised probe template into place:
> >
> > ```
> > $ sudo perf probe -a aspeed_g6_pinctrl_probe
> > [  158.457252] detected buffer overflow in memcpy
> >
> > Fixes: e46daee53bb5 ("ARM: 8806/1: kprobes: Fix false positive with FORTIFY_SOURCE")
> > Fixes: 0ac569bf6a79 ("ARM: 8834/1: Fix: kprobes: optimized kprobes illegal instruction")
> > Cc: Luka Oreskovic <luka.oreskovic@...tura.hr>
> > Cc: Juraj Vijtiuk <juraj.vijtiuk@...tura.hr>
> > Suggested-by: Kees Cook <keescook@...omium.org>
> > Signed-off-by: Andrew Jeffery <andrew@...id.au>
>
> Tested-by: Joel Stanley <joel@....id.au>
> Reviewed-by: Joel Stanley <joel@....id.au>
>
> Thanks Andrew.
>
> > ---
> > v1 was sent some time back, in May:
> >
> > https://lore.kernel.org/linux-arm-kernel/20200517153959.293224-1-andrew@aj.id.au/

Russell, are you picking this fix up?

Would you prefer it to go through someone else's tree?

Cheers,

Joel

> >
> > I've taken the patch that Kees' suggested in the replies and tested it.
> > ---
> >  arch/arm/include/asm/kprobes.h    | 22 +++++++++++-----------
> >  arch/arm/probes/kprobes/opt-arm.c | 18 +++++++++---------
> >  2 files changed, 20 insertions(+), 20 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/kprobes.h b/arch/arm/include/asm/kprobes.h
> > index 213607a1f45c..e26a278d301a 100644
> > --- a/arch/arm/include/asm/kprobes.h
> > +++ b/arch/arm/include/asm/kprobes.h
> > @@ -44,20 +44,20 @@ int kprobe_exceptions_notify(struct notifier_block *self,
> >                              unsigned long val, void *data);
> >
> >  /* optinsn template addresses */
> > -extern __visible kprobe_opcode_t optprobe_template_entry;
> > -extern __visible kprobe_opcode_t optprobe_template_val;
> > -extern __visible kprobe_opcode_t optprobe_template_call;
> > -extern __visible kprobe_opcode_t optprobe_template_end;
> > -extern __visible kprobe_opcode_t optprobe_template_sub_sp;
> > -extern __visible kprobe_opcode_t optprobe_template_add_sp;
> > -extern __visible kprobe_opcode_t optprobe_template_restore_begin;
> > -extern __visible kprobe_opcode_t optprobe_template_restore_orig_insn;
> > -extern __visible kprobe_opcode_t optprobe_template_restore_end;
> > +extern __visible kprobe_opcode_t optprobe_template_entry[];
> > +extern __visible kprobe_opcode_t optprobe_template_val[];
> > +extern __visible kprobe_opcode_t optprobe_template_call[];
> > +extern __visible kprobe_opcode_t optprobe_template_end[];
> > +extern __visible kprobe_opcode_t optprobe_template_sub_sp[];
> > +extern __visible kprobe_opcode_t optprobe_template_add_sp[];
> > +extern __visible kprobe_opcode_t optprobe_template_restore_begin[];
> > +extern __visible kprobe_opcode_t optprobe_template_restore_orig_insn[];
> > +extern __visible kprobe_opcode_t optprobe_template_restore_end[];
> >
> >  #define MAX_OPTIMIZED_LENGTH   4
> >  #define MAX_OPTINSN_SIZE                               \
> > -       ((unsigned long)&optprobe_template_end -        \
> > -        (unsigned long)&optprobe_template_entry)
> > +       ((unsigned long)optprobe_template_end - \
> > +        (unsigned long)optprobe_template_entry)
> >  #define RELATIVEJUMP_SIZE      4
> >
> >  struct arch_optimized_insn {
> > diff --git a/arch/arm/probes/kprobes/opt-arm.c b/arch/arm/probes/kprobes/opt-arm.c
> > index 7a449df0b359..c78180172120 100644
> > --- a/arch/arm/probes/kprobes/opt-arm.c
> > +++ b/arch/arm/probes/kprobes/opt-arm.c
> > @@ -85,21 +85,21 @@ asm (
> >                         "optprobe_template_end:\n");
> >
> >  #define TMPL_VAL_IDX \
> > -       ((unsigned long *)&optprobe_template_val - (unsigned long *)&optprobe_template_entry)
> > +       ((unsigned long *)optprobe_template_val - (unsigned long *)optprobe_template_entry)
> >  #define TMPL_CALL_IDX \
> > -       ((unsigned long *)&optprobe_template_call - (unsigned long *)&optprobe_template_entry)
> > +       ((unsigned long *)optprobe_template_call - (unsigned long *)optprobe_template_entry)
> >  #define TMPL_END_IDX \
> > -       ((unsigned long *)&optprobe_template_end - (unsigned long *)&optprobe_template_entry)
> > +       ((unsigned long *)optprobe_template_end - (unsigned long *)optprobe_template_entry)
> >  #define TMPL_ADD_SP \
> > -       ((unsigned long *)&optprobe_template_add_sp - (unsigned long *)&optprobe_template_entry)
> > +       ((unsigned long *)optprobe_template_add_sp - (unsigned long *)optprobe_template_entry)
> >  #define TMPL_SUB_SP \
> > -       ((unsigned long *)&optprobe_template_sub_sp - (unsigned long *)&optprobe_template_entry)
> > +       ((unsigned long *)optprobe_template_sub_sp - (unsigned long *)optprobe_template_entry)
> >  #define TMPL_RESTORE_BEGIN \
> > -       ((unsigned long *)&optprobe_template_restore_begin - (unsigned long *)&optprobe_template_entry)
> > +       ((unsigned long *)optprobe_template_restore_begin - (unsigned long *)optprobe_template_entry)
> >  #define TMPL_RESTORE_ORIGN_INSN \
> > -       ((unsigned long *)&optprobe_template_restore_orig_insn - (unsigned long *)&optprobe_template_entry)
> > +       ((unsigned long *)optprobe_template_restore_orig_insn - (unsigned long *)optprobe_template_entry)
> >  #define TMPL_RESTORE_END \
> > -       ((unsigned long *)&optprobe_template_restore_end - (unsigned long *)&optprobe_template_entry)
> > +       ((unsigned long *)optprobe_template_restore_end - (unsigned long *)optprobe_template_entry)
> >
> >  /*
> >   * ARM can always optimize an instruction when using ARM ISA, except
> > @@ -234,7 +234,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *or
> >         }
> >
> >         /* Copy arch-dep-instance from template. */
> > -       memcpy(code, (unsigned long *)&optprobe_template_entry,
> > +       memcpy(code, (unsigned long *)optprobe_template_entry,
> >                         TMPL_END_IDX * sizeof(kprobe_opcode_t));
> >
> >         /* Adjust buffer according to instruction. */
> > --
> > 2.25.1
> >