| Message ID | 4691d6019ef00c11007787f5190841b47ba576c4.1597425745.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kasan: add hardware tag-based mode for arm64 | expand |
On Fri, Aug 14, 2020 at 07:27:14PM +0200, Andrey Konovalov wrote: > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > index c62c8ba85c0e..cf00b3942564 100644 > --- a/arch/arm64/mm/fault.c > +++ b/arch/arm64/mm/fault.c > @@ -14,6 +14,7 @@ > #include <linux/mm.h> > #include <linux/hardirq.h> > #include <linux/init.h> > +#include <linux/kasan.h> > #include <linux/kprobes.h> > #include <linux/uaccess.h> > #include <linux/page-flags.h> > @@ -314,11 +315,19 @@ static void report_tag_fault(unsigned long addr, unsigned int esr, > { > bool is_write = ((esr & ESR_ELx_WNR) >> ESR_ELx_WNR_SHIFT) != 0; > > +#ifdef CONFIG_KASAN_HW_TAGS > + /* > + * SAS bits aren't set for all faults reported in EL1, so we can't > + * find out access size. > + */ > + kasan_report(addr, 0, is_write, regs->pc); > +#else > pr_alert("Memory Tagging Extension Fault in %pS\n", (void *)regs->pc); > pr_alert(" %s at address %lx\n", is_write ? "Write" : "Read", addr); > pr_alert(" Pointer tag: [%02x], memory tag: [%02x]\n", > mte_get_ptr_tag(addr), > mte_get_mem_tag((void *)addr)); > +#endif > } More dead code. So what's the point of keeping the pr_alert() introduced earlier? CONFIG_KASAN_HW_TAGS is always on for in-kernel MTE. If MTE is disabled, this function isn't called anyway.
On 8/27/20 11:48 AM, Catalin Marinas wrote: > On Fri, Aug 14, 2020 at 07:27:14PM +0200, Andrey Konovalov wrote: >> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c >> index c62c8ba85c0e..cf00b3942564 100644 >> --- a/arch/arm64/mm/fault.c >> +++ b/arch/arm64/mm/fault.c >> @@ -14,6 +14,7 @@ >> #include <linux/mm.h> >> #include <linux/hardirq.h> >> #include <linux/init.h> >> +#include <linux/kasan.h> >> #include <linux/kprobes.h> >> #include <linux/uaccess.h> >> #include <linux/page-flags.h> >> @@ -314,11 +315,19 @@ static void report_tag_fault(unsigned long addr, unsigned int esr, >> { >> bool is_write = ((esr & ESR_ELx_WNR) >> ESR_ELx_WNR_SHIFT) != 0; >> >> +#ifdef CONFIG_KASAN_HW_TAGS >> + /* >> + * SAS bits aren't set for all faults reported in EL1, so we can't >> + * find out access size. >> + */ >> + kasan_report(addr, 0, is_write, regs->pc); >> +#else >> pr_alert("Memory Tagging Extension Fault in %pS\n", (void *)regs->pc); >> pr_alert(" %s at address %lx\n", is_write ? "Write" : "Read", addr); >> pr_alert(" Pointer tag: [%02x], memory tag: [%02x]\n", >> mte_get_ptr_tag(addr), >> mte_get_mem_tag((void *)addr)); >> +#endif >> } > > More dead code. So what's the point of keeping the pr_alert() introduced > earlier? CONFIG_KASAN_HW_TAGS is always on for in-kernel MTE. If MTE is > disabled, this function isn't called anyway. > I agree we should remove them (togheter with '#ifdef CONFIG_KASAN_HW_TAGS') or integrate them with the kasan code if still meaningful.
On Thu, Aug 27, 2020 at 12:48 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Fri, Aug 14, 2020 at 07:27:14PM +0200, Andrey Konovalov wrote: > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > > index c62c8ba85c0e..cf00b3942564 100644 > > --- a/arch/arm64/mm/fault.c > > +++ b/arch/arm64/mm/fault.c > > @@ -14,6 +14,7 @@ > > #include <linux/mm.h> > > #include <linux/hardirq.h> > > #include <linux/init.h> > > +#include <linux/kasan.h> > > #include <linux/kprobes.h> > > #include <linux/uaccess.h> > > #include <linux/page-flags.h> > > @@ -314,11 +315,19 @@ static void report_tag_fault(unsigned long addr, unsigned int esr, > > { > > bool is_write = ((esr & ESR_ELx_WNR) >> ESR_ELx_WNR_SHIFT) != 0; > > > > +#ifdef CONFIG_KASAN_HW_TAGS > > + /* > > + * SAS bits aren't set for all faults reported in EL1, so we can't > > + * find out access size. > > + */ > > + kasan_report(addr, 0, is_write, regs->pc); > > +#else > > pr_alert("Memory Tagging Extension Fault in %pS\n", (void *)regs->pc); > > pr_alert(" %s at address %lx\n", is_write ? "Write" : "Read", addr); > > pr_alert(" Pointer tag: [%02x], memory tag: [%02x]\n", > > mte_get_ptr_tag(addr), > > mte_get_mem_tag((void *)addr)); > > +#endif > > } > > More dead code. So what's the point of keeping the pr_alert() introduced > earlier? CONFIG_KASAN_HW_TAGS is always on for in-kernel MTE. If MTE is > disabled, this function isn't called anyway. I was considering that we can enable in-kernel MTE without enabling CONFIG_KASAN_HW_TAGS, but perhaps this isn't what we want. I'll drop this part in v2, but then we also need to make sure that in-kernel MTE is only enabled when CONFIG_KASAN_HW_TAGS is enabled. Do we need more ifdefs in arm64 patches when we write to MTE-related registers, or does this work as is?
On Thu, Aug 27, 2020 at 02:34:31PM +0200, Andrey Konovalov wrote: > On Thu, Aug 27, 2020 at 12:48 PM Catalin Marinas > <catalin.marinas@arm.com> wrote: > > On Fri, Aug 14, 2020 at 07:27:14PM +0200, Andrey Konovalov wrote: > > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > > > index c62c8ba85c0e..cf00b3942564 100644 > > > --- a/arch/arm64/mm/fault.c > > > +++ b/arch/arm64/mm/fault.c > > > @@ -14,6 +14,7 @@ > > > #include <linux/mm.h> > > > #include <linux/hardirq.h> > > > #include <linux/init.h> > > > +#include <linux/kasan.h> > > > #include <linux/kprobes.h> > > > #include <linux/uaccess.h> > > > #include <linux/page-flags.h> > > > @@ -314,11 +315,19 @@ static void report_tag_fault(unsigned long addr, unsigned int esr, > > > { > > > bool is_write = ((esr & ESR_ELx_WNR) >> ESR_ELx_WNR_SHIFT) != 0; > > > > > > +#ifdef CONFIG_KASAN_HW_TAGS > > > + /* > > > + * SAS bits aren't set for all faults reported in EL1, so we can't > > > + * find out access size. > > > + */ > > > + kasan_report(addr, 0, is_write, regs->pc); > > > +#else > > > pr_alert("Memory Tagging Extension Fault in %pS\n", (void *)regs->pc); > > > pr_alert(" %s at address %lx\n", is_write ? "Write" : "Read", addr); > > > pr_alert(" Pointer tag: [%02x], memory tag: [%02x]\n", > > > mte_get_ptr_tag(addr), > > > mte_get_mem_tag((void *)addr)); > > > +#endif > > > } > > > > More dead code. So what's the point of keeping the pr_alert() introduced > > earlier? CONFIG_KASAN_HW_TAGS is always on for in-kernel MTE. If MTE is > > disabled, this function isn't called anyway. > > I was considering that we can enable in-kernel MTE without enabling > CONFIG_KASAN_HW_TAGS, but perhaps this isn't what we want. I'll drop > this part in v2, but then we also need to make sure that in-kernel MTE > is only enabled when CONFIG_KASAN_HW_TAGS is enabled. Do we need more > ifdefs in arm64 patches when we write to MTE-related registers, or > does this work as is? I think the in-kernel MTE for the time being should only mean CONFIG_KASAN_HW_TAGS, with a dependency on CONFIG_MTE. KASAN carries some additional debugging features but if we can trim it down, we may not need a separate in-kernel MTE option for production systems (maybe a CONFIG_KASAN_HW_TAGS_LITE).
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c index c62c8ba85c0e..cf00b3942564 100644 --- a/arch/arm64/mm/fault.c +++ b/arch/arm64/mm/fault.c @@ -14,6 +14,7 @@ #include <linux/mm.h> #include <linux/hardirq.h> #include <linux/init.h> +#include <linux/kasan.h> #include <linux/kprobes.h> #include <linux/uaccess.h> #include <linux/page-flags.h> @@ -314,11 +315,19 @@ static void report_tag_fault(unsigned long addr, unsigned int esr, { bool is_write = ((esr & ESR_ELx_WNR) >> ESR_ELx_WNR_SHIFT) != 0; +#ifdef CONFIG_KASAN_HW_TAGS + /* + * SAS bits aren't set for all faults reported in EL1, so we can't + * find out access size. + */ + kasan_report(addr, 0, is_write, regs->pc); +#else pr_alert("Memory Tagging Extension Fault in %pS\n", (void *)regs->pc); pr_alert(" %s at address %lx\n", is_write ? "Write" : "Read", addr); pr_alert(" Pointer tag: [%02x], memory tag: [%02x]\n", mte_get_ptr_tag(addr), mte_get_mem_tag((void *)addr)); +#endif } static void __do_kernel_fault(unsigned long addr, unsigned int esr, diff --git a/mm/kasan/report.c b/mm/kasan/report.c index c904edab33b8..34ef81736d73 100644 --- a/mm/kasan/report.c +++ b/mm/kasan/report.c @@ -65,9 +65,14 @@ static void print_error_description(struct kasan_access_info *info) { pr_err("BUG: KASAN: %s in %pS\n", get_bug_type(info), (void *)info->ip); - pr_err("%s of size %zu at addr %px by task %s/%d\n", - info->is_write ? "Write" : "Read", info->access_size, - info->access_addr, current->comm, task_pid_nr(current)); + if (info->access_size) + pr_err("%s of size %zu at addr %px by task %s/%d\n", + info->is_write ? "Write" : "Read", info->access_size, + info->access_addr, current->comm, task_pid_nr(current)); + else + pr_err("%s at addr %px by task %s/%d\n", + info->is_write ? "Write" : "Read", + info->access_addr, current->comm, task_pid_nr(current)); } static DEFINE_SPINLOCK(report_lock);
Add error reporting for hardware tag-based KASAN. When CONFIG_KASAN_HW_TAGS is enabled, print KASAN report from the arm64 tag fault handler. SAS bits aren't set in ESR for all faults reported in EL1, so it's impossible to find out the size of the access the caused the fault. Adapt KASAN reporting code to handle this case. Signed-off-by: Andrey Konovalov <andreyknvl@google.com> --- arch/arm64/mm/fault.c | 9 +++++++++ mm/kasan/report.c | 11 ++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-)