Re: [PATCH 32/35] kasan, arm64: print report from tag fault handler

2020-08-27 Thread Catalin Marinas
On Thu, Aug 27, 2020 at 02:34:31PM +0200, Andrey Konovalov wrote:
> On Thu, Aug 27, 2020 at 12:48 PM 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 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  #include 
> > >  #include 
> > >  #include 
> > > @@ -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).

-- 
Catalin


Re: [PATCH 32/35] kasan, arm64: print report from tag fault handler

2020-08-27 Thread Andrey Konovalov
On Thu, Aug 27, 2020 at 12:48 PM 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 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -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?


Re: [PATCH 32/35] kasan, arm64: print report from tag fault handler

2020-08-27 Thread Vincenzo Frascino



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 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>  #include 
>> @@ -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.

-- 
Regards,
Vincenzo


Re: [PATCH 32/35] kasan, arm64: print report from tag fault handler

2020-08-27 Thread Catalin Marinas
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 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -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.

-- 
Catalin


[PATCH 32/35] kasan, arm64: print report from tag fault handler

2020-08-14 Thread Andrey Konovalov
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 
---
 arch/arm64/mm/fault.c |  9 +
 mm/kasan/report.c | 11 ---
 2 files changed, 17 insertions(+), 3 deletions(-)

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 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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);
-- 
2.28.0.220.ged08abb693-goog