| Message ID | 20220910052426.943376-1-pcc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kasan: also display registers for reports from HW exceptions | expand |
Hi Peter,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on next-20220909]
[cannot apply to arm64/for-next/core linus/master v6.0-rc4 v6.0-rc3 v6.0-rc2 v6.0-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Peter-Collingbourne/kasan-also-display-registers-for-reports-from-HW-exceptions/20220910-132721
base: 9a82ccda91ed2b40619cb3c10d446ae1f97bab6e
config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20220910/202209101733.SvgcwWsA-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/2140392d32582f62b922eaf4d1824e5a7838b420
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Peter-Collingbourne/kasan-also-display-registers-for-reports-from-HW-exceptions/20220910-132721
git checkout 2140392d32582f62b922eaf4d1824e5a7838b420
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> mm/kasan/report.c:506:6: warning: no previous prototype for 'kasan_report_regs' [-Wmissing-prototypes]
506 | bool kasan_report_regs(unsigned long addr, size_t size, bool is_write,
| ^~~~~~~~~~~~~~~~~
vim +/kasan_report_regs +506 mm/kasan/report.c
505
> 506 bool kasan_report_regs(unsigned long addr, size_t size, bool is_write,
507 struct pt_regs *regs)
508 {
509 return __kasan_report(addr, size, is_write, instruction_pointer(regs),
510 regs);
511 }
512
On Sat, Sep 10, 2022 at 7:24 AM Peter Collingbourne <pcc@google.com> wrote: > > It is sometimes useful to know the values of the registers when a KASAN > report is generated. Hi Peter, What are the cases when the register values are useful? They are "corrupted" by KASAN runtime anyway and thus are not relevant to the place in code where the bad access happened. Thanks! > 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@google.com> > --- > 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"); > > 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); > + 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 >
On Sat, Sep 10, 2022 at 2:40 PM Andrey Konovalov <andreyknvl@gmail.com> wrote: > > On Sat, Sep 10, 2022 at 7:24 AM Peter Collingbourne <pcc@google.com> wrote: > > > > It is sometimes useful to know the values of the registers when a KASAN > > report is generated. > > Hi Peter, > > What are the cases when the register values are useful? They are > "corrupted" by KASAN runtime anyway and thus are not relevant to the > place in code where the bad access happened. > > Thanks! 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. 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. 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@google.com> > > --- > > 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"); > > > > 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); > > + 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 > >
On Tue, Sep 13, 2022 at 6:00 AM Peter Collingbourne <pcc@google.com> 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@google.com> > > > --- > > > 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 > > >
On Sat, Sep 24, 2022 at 11:23 AM Andrey Konovalov <andreyknvl@gmail.com> wrote: > > On Tue, Sep 13, 2022 at 6:00 AM Peter Collingbourne <pcc@google.com> 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. Done in v2. > > 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@google.com> > > > > --- > > > > 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. Done in v2. > > > > > > > > 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. Ack. Peter
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"); 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); + 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) {
It is sometimes useful to know the values of the registers when a KASAN report is generated. 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@google.com> --- 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(-)