Re: [PATCH] kasan: also display registers for reports from HW exceptions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Sep 13, 2022 at 6:00 AM Peter Collingbourne <pcc@xxxxxxxxxx> wrote:
>
> Hi Andrey,
>
> The most useful case would be for tag check faults with HW tags based
> KASAN where the errant instruction would result in an immediate
> exception which gives the kernel the opportunity to save all of the
> registers to the struct pt_regs.

Right.

> For SW tags based KASAN with inline
> checks it is less useful because some registers will have been used to
> perform the check but I imagine that in some cases even that could be
> better than nothing.

Let's not print the registers for the SW_TAGS mode then. I think
sometimes-irrelevant values might confuse people.

> Peter
>
> > > We can do this easily for reports that resulted from
> > > a hardware exception by passing the struct pt_regs from the exception into
> > > the report function; do so.
> > >
> > > Signed-off-by: Peter Collingbourne <pcc@xxxxxxxxxx>
> > > ---
> > > Applies to -next.
> > >
> > >  arch/arm64/kernel/traps.c |  3 +--
> > >  arch/arm64/mm/fault.c     |  2 +-
> > >  include/linux/kasan.h     | 10 ++++++++++
> > >  mm/kasan/kasan.h          |  1 +
> > >  mm/kasan/report.c         | 27 ++++++++++++++++++++++-----
> > >  5 files changed, 35 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > > index b7fed33981f7..42f05f38c90a 100644
> > > --- a/arch/arm64/kernel/traps.c
> > > +++ b/arch/arm64/kernel/traps.c
> > > @@ -1019,9 +1019,8 @@ static int kasan_handler(struct pt_regs *regs, unsigned long esr)
> > >         bool write = esr & KASAN_ESR_WRITE;
> > >         size_t size = KASAN_ESR_SIZE(esr);
> > >         u64 addr = regs->regs[0];
> > > -       u64 pc = regs->pc;
> > >
> > > -       kasan_report(addr, size, write, pc);
> > > +       kasan_report_regs(addr, size, write, regs);
> > >
> > >         /*
> > >          * The instrumentation allows to control whether we can proceed after
> > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> > > index 5b391490e045..c4b91f5d8cc8 100644
> > > --- a/arch/arm64/mm/fault.c
> > > +++ b/arch/arm64/mm/fault.c
> > > @@ -316,7 +316,7 @@ static void report_tag_fault(unsigned long addr, unsigned long esr,
> > >          * find out access size.
> > >          */
> > >         bool is_write = !!(esr & ESR_ELx_WNR);
> > > -       kasan_report(addr, 0, is_write, regs->pc);
> > > +       kasan_report_regs(addr, 0, is_write, regs);
> > >  }
> > >  #else
> > >  /* Tag faults aren't enabled without CONFIG_KASAN_HW_TAGS. */
> > > diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> > > index d811b3d7d2a1..381aea149353 100644
> > > --- a/include/linux/kasan.h
> > > +++ b/include/linux/kasan.h
> > > @@ -353,6 +353,16 @@ static inline void *kasan_reset_tag(const void *addr)
> > >  bool kasan_report(unsigned long addr, size_t size,
> > >                 bool is_write, unsigned long ip);
> > >
> > > +/**
> > > + * kasan_report_regs - print a report about a bad memory access detected by KASAN
> > > + * @addr: address of the bad access
> > > + * @size: size of the bad access
> > > + * @is_write: whether the bad access is a write or a read
> > > + * @regs: register values at the point of the bad memory access
> > > + */
> > > +bool kasan_report_regs(unsigned long addr, size_t size, bool is_write,
> > > +                      struct pt_regs *regs);
> > > +
> > >  #else /* CONFIG_KASAN_SW_TAGS || CONFIG_KASAN_HW_TAGS */
> > >
> > >  static inline void *kasan_reset_tag(const void *addr)
> > > diff --git a/mm/kasan/kasan.h b/mm/kasan/kasan.h
> > > index abbcc1b0eec5..39772c21a8ae 100644
> > > --- a/mm/kasan/kasan.h
> > > +++ b/mm/kasan/kasan.h
> > > @@ -175,6 +175,7 @@ struct kasan_report_info {
> > >         size_t access_size;
> > >         bool is_write;
> > >         unsigned long ip;
> > > +       struct pt_regs *regs;
> > >
> > >         /* Filled in by the common reporting code. */
> > >         void *first_bad_addr;
> > > diff --git a/mm/kasan/report.c b/mm/kasan/report.c
> > > index 39e8e5a80b82..eac9cd45b4a1 100644
> > > --- a/mm/kasan/report.c
> > > +++ b/mm/kasan/report.c
> > > @@ -24,6 +24,7 @@
> > >  #include <linux/types.h>
> > >  #include <linux/kasan.h>
> > >  #include <linux/module.h>
> > > +#include <linux/sched/debug.h>
> > >  #include <linux/sched/task_stack.h>
> > >  #include <linux/uaccess.h>
> > >  #include <trace/events/error_report.h>
> > > @@ -284,7 +285,6 @@ static void print_address_description(void *addr, u8 tag,
> > >  {
> > >         struct page *page = addr_to_page(addr);
> > >
> > > -       dump_stack_lvl(KERN_ERR);
> > >         pr_err("\n");

Please pull this pr_err out of this function and put right before the
function is called.

> > >
> > >         if (info->cache && info->object) {
> > > @@ -394,11 +394,14 @@ static void print_report(struct kasan_report_info *info)
> > >                 kasan_print_tags(tag, info->first_bad_addr);
> > >         pr_err("\n");
> > >
> > > +       if (info->regs)
> > > +               show_regs(info->regs);

Looks like show_regs prints with KERN_DEFAULT. Inconsistent with
KERN_ERR used for the rest of the report, but looks like there's no
easy way to fix this. Let's leave as is.

> > > +       else
> > > +               dump_stack_lvl(KERN_ERR);
> > > +
> > >         if (addr_has_metadata(addr)) {
> > >                 print_address_description(addr, tag, info);
> > >                 print_memory_metadata(info->first_bad_addr);
> > > -       } else {
> > > -               dump_stack_lvl(KERN_ERR);
> > >         }
> > >  }
> > >
> > > @@ -458,8 +461,8 @@ void kasan_report_invalid_free(void *ptr, unsigned long ip, enum kasan_report_ty
> > >   * user_access_save/restore(): kasan_report_invalid_free() cannot be called
> > >   * from a UACCESS region, and kasan_report_async() is not used on x86.
> > >   */
> > > -bool kasan_report(unsigned long addr, size_t size, bool is_write,
> > > -                       unsigned long ip)
> > > +static bool __kasan_report(unsigned long addr, size_t size, bool is_write,
> > > +                       unsigned long ip, struct pt_regs *regs)
> > >  {
> > >         bool ret = true;
> > >         void *ptr = (void *)addr;
> > > @@ -480,6 +483,7 @@ bool kasan_report(unsigned long addr, size_t size, bool is_write,
> > >         info.access_size = size;
> > >         info.is_write = is_write;
> > >         info.ip = ip;
> > > +       info.regs = regs;
> > >
> > >         complete_report_info(&info);
> > >
> > > @@ -493,6 +497,19 @@ bool kasan_report(unsigned long addr, size_t size, bool is_write,
> > >         return ret;
> > >  }
> > >
> > > +bool kasan_report(unsigned long addr, size_t size, bool is_write,
> > > +                       unsigned long ip)
> > > +{
> > > +       return __kasan_report(addr, size, is_write, ip, NULL);
> > > +}
> > > +
> > > +bool kasan_report_regs(unsigned long addr, size_t size, bool is_write,
> > > +                      struct pt_regs *regs)
> > > +{
> > > +       return __kasan_report(addr, size, is_write, instruction_pointer(regs),
> > > +                             regs);
> > > +}
> > > +
> > >  #ifdef CONFIG_KASAN_HW_TAGS
> > >  void kasan_report_async(void)
> > >  {
> > > --
> > > 2.37.2.789.g6183377224-goog
> > >




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux