linux-sparse.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v13 08/25] kasan: initialize shadow to 0xff for tag-based mode
       [not found]   ` <CAP=VYLo-o8vpGrpM_+0jdvxLC9uxw+F7_OtsSfRyq24HR1dDwA@mail.gmail.com>
@ 2018-12-10 12:12     ` Andrey Konovalov
  0 siblings, 0 replies; 12+ messages in thread
From: Andrey Konovalov @ 2018-12-10 12:12 UTC (permalink / raw)
  To: paul.gortmaker
  Cc: Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov,
	Catalin Marinas, Will Deacon, Christoph Lameter, Andrew Morton,
	Mark Rutland, Nick Desaulniers, Marc Zyngier, Dave Martin,
	Ard Biesheuvel, Eric W . Biederman, Ingo Molnar, Paul Lawrence,
	Geert Uytterhoeven, Arnd Bergmann, Kirill A. Shutemov,
	Greg Kroah-Hartman

On Mon, Dec 10, 2018 at 2:35 AM Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
>
> On Thu, Dec 6, 2018 at 7:25 AM Andrey Konovalov <andreyknvl@google.com> wrote:
>>
>> A tag-based KASAN shadow memory cell contains a memory tag, that
>> corresponds to the tag in the top byte of the pointer, that points to that
>> memory. The native top byte value of kernel pointers is 0xff, so with
>> tag-based KASAN we need to initialize shadow memory to 0xff.
>>
>> Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
>> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>> ---
>>  arch/arm64/mm/kasan_init.c | 15 +++++++++++++--
>>  include/linux/kasan.h      |  8 ++++++++
>
>
> The version of this in  linux-next breaks arm64 allmodconfig for me:
>
> mm/kasan/common.c: In function ‘kasan_module_alloc’:
> mm/kasan/common.c:481:17: error: ‘KASAN_SHADOW_INIT’ undeclared (first use in this function)
>    __memset(ret, KASAN_SHADOW_INIT, shadow_size);
>                  ^
> mm/kasan/common.c:481:17: note: each undeclared identifier is reported only once for each function it appears in
> make[3]: *** [mm/kasan/common.o] Error 1
> make[3]: *** Waiting for unfinished jobs....
> make[2]: *** [mm/kasan] Error 2
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [mm/] Error 2
> make: *** [sub-make] Error 2

Hi Paul,

This is my bad, this should be fixed in v13 of this patchset, which is
in mm right now but not in linux-next yet as it seems.

Thanks!

>
> An automated git bisect-run points at this:
>
> 5c36287813721999e79ac76f637f1ba7e5054402 is the first bad commit
> commit 5c36287813721999e79ac76f637f1ba7e5054402
> Author: Andrey Konovalov <andreyknvl@google.com>
> Date:   Wed Dec 5 11:13:21 2018 +1100
>
>     kasan: initialize shadow to 0xff for tag-based mode
>
> A quick look at the commit makes me think that the case where the
> "# CONFIG_KASAN_GENERIC is not set" has not been handled.
>
> I'm using an older gcc 4.8.3 - only used for build testing.
>
> Paul.
> --
>
>>  mm/kasan/common.c          |  3 ++-
>>  3 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/mm/kasan_init.c b/arch/arm64/mm/kasan_init.c
>> index 4ebc19422931..7a4a0904cac8 100644
>> --- a/arch/arm64/mm/kasan_init.c
>> +++ b/arch/arm64/mm/kasan_init.c
>> @@ -43,6 +43,15 @@ static phys_addr_t __init kasan_alloc_zeroed_page(int node)
>>         return __pa(p);
>>  }
>>
>> +static phys_addr_t __init kasan_alloc_raw_page(int node)
>> +{
>> +       void *p = memblock_alloc_try_nid_raw(PAGE_SIZE, PAGE_SIZE,
>> +                                               __pa(MAX_DMA_ADDRESS),
>> +                                               MEMBLOCK_ALLOC_ACCESSIBLE,
>> +                                               node);
>> +       return __pa(p);
>> +}
>> +
>>  static pte_t *__init kasan_pte_offset(pmd_t *pmdp, unsigned long addr, int node,
>>                                       bool early)
>>  {
>> @@ -92,7 +101,9 @@ static void __init kasan_pte_populate(pmd_t *pmdp, unsigned long addr,
>>         do {
>>                 phys_addr_t page_phys = early ?
>>                                 __pa_symbol(kasan_early_shadow_page)
>> -                                       : kasan_alloc_zeroed_page(node);
>> +                                       : kasan_alloc_raw_page(node);
>> +               if (!early)
>> +                       memset(__va(page_phys), KASAN_SHADOW_INIT, PAGE_SIZE);
>>                 next = addr + PAGE_SIZE;
>>                 set_pte(ptep, pfn_pte(__phys_to_pfn(page_phys), PAGE_KERNEL));
>>         } while (ptep++, addr = next, addr != end && pte_none(READ_ONCE(*ptep)));
>> @@ -239,7 +250,7 @@ void __init kasan_init(void)
>>                         pfn_pte(sym_to_pfn(kasan_early_shadow_page),
>>                                 PAGE_KERNEL_RO));
>>
>> -       memset(kasan_early_shadow_page, 0, PAGE_SIZE);
>> +       memset(kasan_early_shadow_page, KASAN_SHADOW_INIT, PAGE_SIZE);
>>         cpu_replace_ttbr1(lm_alias(swapper_pg_dir));
>>
>>         /* At this point kasan is fully initialized. Enable error messages */
>> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
>> index ec22d548d0d7..c56af24bd3e7 100644
>> --- a/include/linux/kasan.h
>> +++ b/include/linux/kasan.h
>> @@ -153,6 +153,8 @@ static inline size_t kasan_metadata_size(struct kmem_cache *cache) { return 0; }
>>
>>  #ifdef CONFIG_KASAN_GENERIC
>>
>> +#define KASAN_SHADOW_INIT 0
>> +
>>  void kasan_cache_shrink(struct kmem_cache *cache);
>>  void kasan_cache_shutdown(struct kmem_cache *cache);
>>
>> @@ -163,4 +165,10 @@ static inline void kasan_cache_shutdown(struct kmem_cache *cache) {}
>>
>>  #endif /* CONFIG_KASAN_GENERIC */
>>
>> +#ifdef CONFIG_KASAN_SW_TAGS
>> +
>> +#define KASAN_SHADOW_INIT 0xFF
>> +
>> +#endif /* CONFIG_KASAN_SW_TAGS */
>> +
>>  #endif /* LINUX_KASAN_H */
>> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
>> index 5f68c93734ba..7134e75447ff 100644
>> --- a/mm/kasan/common.c
>> +++ b/mm/kasan/common.c
>> @@ -473,11 +473,12 @@ int kasan_module_alloc(void *addr, size_t size)
>>
>>         ret = __vmalloc_node_range(shadow_size, 1, shadow_start,
>>                         shadow_start + shadow_size,
>> -                       GFP_KERNEL | __GFP_ZERO,
>> +                       GFP_KERNEL,
>>                         PAGE_KERNEL, VM_NO_GUARD, NUMA_NO_NODE,
>>                         __builtin_return_address(0));
>>
>>         if (ret) {
>> +               __memset(ret, KASAN_SHADOW_INIT, shadow_size);
>>                 find_vm_area(addr)->flags |= VM_KASAN;
>>                 kmemleak_ignore(ret);
>>                 return 0;
>> --
>> 2.20.0.rc1.387.gf8505762e3-goog
>>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v13 00/25] kasan: add software tag-based mode for arm64
       [not found] <cover.1544099024.git.andreyknvl@google.com>
       [not found] ` <5cc1b789aad7c99cf4f3ec5b328b147ad53edb40.1544099024.git.andreyknvl@google.com>
@ 2018-12-11 15:18 ` Will Deacon
  2018-12-11 15:57   ` Andrey Konovalov
       [not found] ` <b2550106eb8a68b10fefbabce820910b115aa853.1544099024.git.andreyknvl@google.com>
       [not found] ` <bda78069e3b8422039794050ddcb2d53d053ed41.1544099024.git.andreyknvl@google.com>
  3 siblings, 1 reply; 12+ messages in thread
From: Will Deacon @ 2018-12-11 15:18 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov,
	Catalin Marinas, Christoph Lameter, Andrew Morton, Mark Rutland,
	Nick Desaulniers, Marc Zyngier, Dave Martin, Ard Biesheuvel,
	Eric W . Biederman, Ingo Molnar, Paul Lawrence,
	Geert Uytterhoeven, Arnd Bergmann, Kirill A . Shutemov,
	Greg Kroah-Hartman, Kate Stewart <ks>

Hi Andrey,

On Thu, Dec 06, 2018 at 01:24:18PM +0100, Andrey Konovalov wrote:
> This patchset adds a new software tag-based mode to KASAN [1].
> (Initially this mode was called KHWASAN, but it got renamed,
>  see the naming rationale at the end of this section).
> 
> The plan is to implement HWASan [2] for the kernel with the incentive,
> that it's going to have comparable to KASAN performance, but in the same
> time consume much less memory, trading that off for somewhat imprecise
> bug detection and being supported only for arm64.

For the arm64 parts:

Acked-by: Will Deacon <will.deacon@arm.com>

I assume that you plan to replace the current patches in -mm with this
series?

Cheers,

Will

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v13 05/25] kasan: add CONFIG_KASAN_GENERIC and CONFIG_KASAN_SW_TAGS
       [not found] ` <b2550106eb8a68b10fefbabce820910b115aa853.1544099024.git.andreyknvl@google.com>
@ 2018-12-11 15:28   ` Luc Van Oostenryck
  2018-12-11 16:02     ` Andrey Konovalov
  0 siblings, 1 reply; 12+ messages in thread
From: Luc Van Oostenryck @ 2018-12-11 15:28 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov,
	Catalin Marinas, Will Deacon, Christoph Lameter, Andrew Morton,
	Mark Rutland, Nick Desaulniers, Marc Zyngier, Dave Martin,
	Ard Biesheuvel, Eric W . Biederman, Ingo Molnar, Paul Lawrence,
	Geert Uytterhoeven, Arnd Bergmann, Kirill A . Shutemov,
	Greg Kroah-Hartman

On Thu, Dec 06, 2018 at 01:24:23PM +0100, Andrey Konovalov wrote:
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index 3e7dafb3ea80..39f668d5066b 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -16,9 +16,13 @@
>  /* all clang versions usable with the kernel support KASAN ABI version 5 */
>  #define KASAN_ABI_VERSION 5
>  
> +#if __has_feature(address_sanitizer) || __has_feature(hwaddress_sanitizer)
>  /* emulate gcc's __SANITIZE_ADDRESS__ flag */
> -#if __has_feature(address_sanitizer)
>  #define __SANITIZE_ADDRESS__
> +#define __no_sanitize_address \
> +		__attribute__((no_sanitize("address", "hwaddress")))
> +#else
> +#define __no_sanitize_address
>  #endif
>  
>  /*
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index 2010493e1040..5776da43da97 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -143,6 +143,12 @@
>  #define KASAN_ABI_VERSION 3
>  #endif
>  
> +#if __has_attribute(__no_sanitize_address__)
> +#define __no_sanitize_address __attribute__((no_sanitize_address))
> +#else
> +#define __no_sanitize_address
> +#endif

Not really important but it's the name with leading and trailing
underscores that is tested with __has_attribute() but then it's
the naked 'no_sanitize_address' that is used in the attribute.

-- Luc

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v13 00/25] kasan: add software tag-based mode for arm64
  2018-12-11 15:18 ` [PATCH v13 00/25] kasan: add software tag-based mode for arm64 Will Deacon
@ 2018-12-11 15:57   ` Andrey Konovalov
  2018-12-11 16:00     ` Will Deacon
  0 siblings, 1 reply; 12+ messages in thread
From: Andrey Konovalov @ 2018-12-11 15:57 UTC (permalink / raw)
  To: Will Deacon
  Cc: Mark Rutland, Kate Stewart, open list:DOCUMENTATION,
	Catalin Marinas, Paul Lawrence, Linux Memory Management List,
	Alexander Potapenko, Chintan Pandya, Christoph Lameter,
	Ingo Molnar, Jacob Bramley, Ruben Ayrapetyan, Mark Brand,
	kasan-dev, linux-sparse, Geert Uytterhoeven, Linux ARM,
	Andrey Ryabinin, Dave Martin, Evgenii Stepanov, Vishwath Mohan,
	Arnd Bergmann

On Tue, Dec 11, 2018 at 4:18 PM Will Deacon <will.deacon@arm.com> wrote:
>
> Hi Andrey,
>
> On Thu, Dec 06, 2018 at 01:24:18PM +0100, Andrey Konovalov wrote:
> > This patchset adds a new software tag-based mode to KASAN [1].
> > (Initially this mode was called KHWASAN, but it got renamed,
> >  see the naming rationale at the end of this section).
> >
> > The plan is to implement HWASan [2] for the kernel with the incentive,
> > that it's going to have comparable to KASAN performance, but in the same
> > time consume much less memory, trading that off for somewhat imprecise
> > bug detection and being supported only for arm64.
>
> For the arm64 parts:
>
> Acked-by: Will Deacon <will.deacon@arm.com>
>
> I assume that you plan to replace the current patches in -mm with this
> series?
>
> Cheers,
>
> Will

Hi Will,

Yes, that was the intention of sending v13. Should have I sent a
separate patch with v12->v13 fixes instead? I don't know what's the
usual way to make changes to the patchset once it's in the mm tree.

Thanks!

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v13 00/25] kasan: add software tag-based mode for arm64
  2018-12-11 15:57   ` Andrey Konovalov
@ 2018-12-11 16:00     ` Will Deacon
  2018-12-11 21:44       ` Andrew Morton
  0 siblings, 1 reply; 12+ messages in thread
From: Will Deacon @ 2018-12-11 16:00 UTC (permalink / raw)
  To: Andrey Konovalov, akpm
  Cc: Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov,
	Catalin Marinas, Christoph Lameter, Mark Rutland,
	Nick Desaulniers, Marc Zyngier, Dave Martin, Ard Biesheuvel,
	Eric W . Biederman, Ingo Molnar, Paul Lawrence,
	Geert Uytterhoeven, Arnd Bergmann, Kirill A. Shutemov,
	Greg Kroah-Hartman, Kate Stewart

[moving akpm to To:]

On Tue, Dec 11, 2018 at 04:57:27PM +0100, Andrey Konovalov wrote:
> On Tue, Dec 11, 2018 at 4:18 PM Will Deacon <will.deacon@arm.com> wrote:
> > On Thu, Dec 06, 2018 at 01:24:18PM +0100, Andrey Konovalov wrote:
> > > This patchset adds a new software tag-based mode to KASAN [1].
> > > (Initially this mode was called KHWASAN, but it got renamed,
> > >  see the naming rationale at the end of this section).
> > >
> > > The plan is to implement HWASan [2] for the kernel with the incentive,
> > > that it's going to have comparable to KASAN performance, but in the same
> > > time consume much less memory, trading that off for somewhat imprecise
> > > bug detection and being supported only for arm64.
> >
> > For the arm64 parts:
> >
> > Acked-by: Will Deacon <will.deacon@arm.com>
> >
> > I assume that you plan to replace the current patches in -mm with this
> > series?
> >
> > Cheers,
> >
> > Will
> 
> Hi Will,
> 
> Yes, that was the intention of sending v13. Should have I sent a
> separate patch with v12->v13 fixes instead? I don't know what's the
> usual way to make changes to the patchset once it's in the mm tree.

No, I was just checking that the intention was for akpm to pick this up in
preference to the old stuff! That works for me, and the minor conflict with
arm64 in next was already resolved by Stephen.

Will

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v13 05/25] kasan: add CONFIG_KASAN_GENERIC and CONFIG_KASAN_SW_TAGS
  2018-12-11 15:28   ` [PATCH v13 05/25] kasan: add CONFIG_KASAN_GENERIC and CONFIG_KASAN_SW_TAGS Luc Van Oostenryck
@ 2018-12-11 16:02     ` Andrey Konovalov
  0 siblings, 0 replies; 12+ messages in thread
From: Andrey Konovalov @ 2018-12-11 16:02 UTC (permalink / raw)
  To: Luc Van Oostenryck
  Cc: Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov,
	Catalin Marinas, Will Deacon, Christoph Lameter, Andrew Morton,
	Mark Rutland, Nick Desaulniers, Marc Zyngier, Dave Martin,
	Ard Biesheuvel, Eric W . Biederman, Ingo Molnar, Paul Lawrence,
	Geert Uytterhoeven, Arnd Bergmann, Kirill A. Shutemov,
	Greg Kroah-Hartman

On Tue, Dec 11, 2018 at 4:28 PM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> On Thu, Dec 06, 2018 at 01:24:23PM +0100, Andrey Konovalov wrote:
> > diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> > index 3e7dafb3ea80..39f668d5066b 100644
> > --- a/include/linux/compiler-clang.h
> > +++ b/include/linux/compiler-clang.h
> > @@ -16,9 +16,13 @@
> >  /* all clang versions usable with the kernel support KASAN ABI version 5 */
> >  #define KASAN_ABI_VERSION 5
> >
> > +#if __has_feature(address_sanitizer) || __has_feature(hwaddress_sanitizer)
> >  /* emulate gcc's __SANITIZE_ADDRESS__ flag */
> > -#if __has_feature(address_sanitizer)
> >  #define __SANITIZE_ADDRESS__
> > +#define __no_sanitize_address \
> > +             __attribute__((no_sanitize("address", "hwaddress")))
> > +#else
> > +#define __no_sanitize_address
> >  #endif
> >
> >  /*
> > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> > index 2010493e1040..5776da43da97 100644
> > --- a/include/linux/compiler-gcc.h
> > +++ b/include/linux/compiler-gcc.h
> > @@ -143,6 +143,12 @@
> >  #define KASAN_ABI_VERSION 3
> >  #endif
> >
> > +#if __has_attribute(__no_sanitize_address__)
> > +#define __no_sanitize_address __attribute__((no_sanitize_address))
> > +#else
> > +#define __no_sanitize_address
> > +#endif
>
> Not really important but it's the name with leading and trailing
> underscores that is tested with __has_attribute() but then it's
> the naked 'no_sanitize_address' that is used in the attribute.

Hi Luc,

You're right. This shouldn't be important though, since "__" in
attribute names are optional AFAIK. It might make sense to fix it as a
separate patch when this series is merged.

Thanks!

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v13 00/25] kasan: add software tag-based mode for arm64
  2018-12-11 16:00     ` Will Deacon
@ 2018-12-11 21:44       ` Andrew Morton
  0 siblings, 0 replies; 12+ messages in thread
From: Andrew Morton @ 2018-12-11 21:44 UTC (permalink / raw)
  To: Will Deacon
  Cc: Andrey Konovalov, Andrey Ryabinin, Alexander Potapenko,
	Dmitry Vyukov, Catalin Marinas, Christoph Lameter, Mark Rutland,
	Nick Desaulniers, Marc Zyngier, Dave Martin, Ard Biesheuvel,
	Eric W . Biederman, Ingo Molnar, Paul Lawrence,
	Geert Uytterhoeven, Arnd Bergmann, Kirill A. Shutemov,
	Greg Kroah-Hartman, Kate Stewart <kste>

On Tue, 11 Dec 2018 16:00:19 +0000 Will Deacon <will.deacon@arm.com> wrote:

> > Yes, that was the intention of sending v13. Should have I sent a
> > separate patch with v12->v13 fixes instead? I don't know what's the
> > usual way to make changes to the patchset once it's in the mm tree.

I usually convert replacement patches into deltas so people can see
what changed.  In this case it got messy so I dropped v12 and remerged
v13 wholesale.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v13 19/25] kasan: add hooks implementation for tag-based mode
       [not found]   ` <2bf7415e-2724-b3c3-9571-20c8b6d43b92@arm.com>
@ 2018-12-12 15:04     ` Andrey Konovalov
  2018-12-14 12:35       ` Vincenzo Frascino
  0 siblings, 1 reply; 12+ messages in thread
From: Andrey Konovalov @ 2018-12-12 15:04 UTC (permalink / raw)
  To: Vincenzo Frascino
  Cc: Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov,
	Catalin Marinas, Will Deacon, Christoph Lameter, Andrew Morton,
	Mark Rutland, Nick Desaulniers, Marc Zyngier, Dave Martin,
	Ard Biesheuvel, Eric W . Biederman, Ingo Molnar, Paul Lawrence,
	Geert Uytterhoeven, Arnd Bergmann, Kirill A. Shutemov,
	Greg Kroah-Hartman

On Tue, Dec 11, 2018 at 5:22 PM Vincenzo Frascino
<vincenzo.frascino@arm.com> wrote:
>
> Hi Andrey,
>
> On 06/12/2018 12:24, Andrey Konovalov wrote:
> > This commit adds tag-based KASAN specific hooks implementation and
> > adjusts common generic and tag-based KASAN ones.
> >
> > 1. When a new slab cache is created, tag-based KASAN rounds up the size of
> >    the objects in this cache to KASAN_SHADOW_SCALE_SIZE (== 16).
> >
> > 2. On each kmalloc tag-based KASAN generates a random tag, sets the shadow
> >    memory, that corresponds to this object to this tag, and embeds this
> >    tag value into the top byte of the returned pointer.
> >
> > 3. On each kfree tag-based KASAN poisons the shadow memory with a random
> >    tag to allow detection of use-after-free bugs.
> >
> > The rest of the logic of the hook implementation is very much similar to
> > the one provided by generic KASAN. Tag-based KASAN saves allocation and
> > free stack metadata to the slab object the same way generic KASAN does.
> >
> > Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> > Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> > ---
> >  mm/kasan/common.c | 116 ++++++++++++++++++++++++++++++++++++++--------
> >  mm/kasan/kasan.h  |   8 ++++
> >  mm/kasan/tags.c   |  48 +++++++++++++++++++
> >  3 files changed, 153 insertions(+), 19 deletions(-)
> >
>
>
> [...]
>
> > @@ -265,6 +290,8 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
> >               return;
> >       }
> >
> > +     cache->align = round_up(cache->align, KASAN_SHADOW_SCALE_SIZE);
> > +
>
> Did you consider to set ARCH_SLAB_MINALIGN instead of this round up?

I didn't know about this macro. Looks like we can use it to do the
same thing. Do you think it's a better solution to redefine
ARCH_SLAB_MINALIGN to KASAN_SHADOW_SCALE_SIZE for arm64 when tag-based
KASAN is enabled instead of adjusting cache->align in
kasan_cache_create?

>
> --
> Regards,
> Vincenzo
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To post to this group, send email to kasan-dev@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/2bf7415e-2724-b3c3-9571-20c8b6d43b92%40arm.com.
> For more options, visit https://groups.google.com/d/optout.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v13 19/25] kasan: add hooks implementation for tag-based mode
  2018-12-12 15:04     ` [PATCH v13 19/25] kasan: add hooks implementation for tag-based mode Andrey Konovalov
@ 2018-12-14 12:35       ` Vincenzo Frascino
  2018-12-17 19:33         ` Andrey Konovalov
  0 siblings, 1 reply; 12+ messages in thread
From: Vincenzo Frascino @ 2018-12-14 12:35 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov,
	Catalin Marinas, Will Deacon, Christoph Lameter, Andrew Morton,
	Mark Rutland, Nick Desaulniers, Marc Zyngier, Dave Martin,
	Ard Biesheuvel, Eric W . Biederman, Ingo Molnar, Paul Lawrence,
	Geert Uytterhoeven, Arnd Bergmann, Kirill A. Shutemov,
	Greg Kroah-Hartman

On 12/12/18 3:04 PM, Andrey Konovalov wrote:
> On Tue, Dec 11, 2018 at 5:22 PM Vincenzo Frascino
> <vincenzo.frascino@arm.com> wrote:
>>
>> Hi Andrey,
>>
>> On 06/12/2018 12:24, Andrey Konovalov wrote:
>>> This commit adds tag-based KASAN specific hooks implementation and
>>> adjusts common generic and tag-based KASAN ones.
>>>
>>> 1. When a new slab cache is created, tag-based KASAN rounds up the size of
>>>    the objects in this cache to KASAN_SHADOW_SCALE_SIZE (== 16).
>>>
>>> 2. On each kmalloc tag-based KASAN generates a random tag, sets the shadow
>>>    memory, that corresponds to this object to this tag, and embeds this
>>>    tag value into the top byte of the returned pointer.
>>>
>>> 3. On each kfree tag-based KASAN poisons the shadow memory with a random
>>>    tag to allow detection of use-after-free bugs.
>>>
>>> The rest of the logic of the hook implementation is very much similar to
>>> the one provided by generic KASAN. Tag-based KASAN saves allocation and
>>> free stack metadata to the slab object the same way generic KASAN does.
>>>
>>> Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
>>> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
>>> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>>> ---
>>>  mm/kasan/common.c | 116 ++++++++++++++++++++++++++++++++++++++--------
>>>  mm/kasan/kasan.h  |   8 ++++
>>>  mm/kasan/tags.c   |  48 +++++++++++++++++++
>>>  3 files changed, 153 insertions(+), 19 deletions(-)
>>>
>>
>>
>> [...]
>>
>>> @@ -265,6 +290,8 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
>>>               return;
>>>       }
>>>
>>> +     cache->align = round_up(cache->align, KASAN_SHADOW_SCALE_SIZE);
>>> +
>>
>> Did you consider to set ARCH_SLAB_MINALIGN instead of this round up?
> 
> I didn't know about this macro. Looks like we can use it to do the
> same thing. Do you think it's a better solution to redefine
> ARCH_SLAB_MINALIGN to KASAN_SHADOW_SCALE_SIZE for arm64 when tag-based
> KASAN is enabled instead of adjusting cache->align in
> kasan_cache_create?
>

Yes, I think it is better because in this way we do not need to add extra code
to do the rounding.

Curiosity, did you try your patches with SLUB red zoning enabled?
Since the area used for the Redzone is just after the payload, aligning the
object_size independently from the allocator could have side effects, at least
if I understand well how the mechanism works.

Setting ARCH_SLAB_MINALIGN should avoid this as well.

What do you think?

>>
>> --
>> Regards,
>> Vincenzo
>>
>> --
>> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
>> To post to this group, send email to kasan-dev@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/2bf7415e-2724-b3c3-9571-20c8b6d43b92%40arm.com.
>> For more options, visit https://groups.google.com/d/optout.

-- 
Regards,
Vincenzo

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v13 19/25] kasan: add hooks implementation for tag-based mode
  2018-12-14 12:35       ` Vincenzo Frascino
@ 2018-12-17 19:33         ` Andrey Konovalov
  2018-12-17 20:38           ` Andrew Morton
  0 siblings, 1 reply; 12+ messages in thread
From: Andrey Konovalov @ 2018-12-17 19:33 UTC (permalink / raw)
  To: Vincenzo Frascino, Andrew Morton
  Cc: Andrey Ryabinin, Alexander Potapenko, Dmitry Vyukov,
	Catalin Marinas, Will Deacon, Christoph Lameter, Mark Rutland,
	Nick Desaulniers, Marc Zyngier, Dave Martin, Ard Biesheuvel,
	Eric W . Biederman, Ingo Molnar, Paul Lawrence,
	Geert Uytterhoeven, Arnd Bergmann, Kirill A. Shutemov,
	Greg Kroah-Hartman, Kate Stewart

On Fri, Dec 14, 2018 at 1:34 PM Vincenzo Frascino
<vincenzo.frascino@arm.com> wrote:
>
> On 12/12/18 3:04 PM, Andrey Konovalov wrote:
> > On Tue, Dec 11, 2018 at 5:22 PM Vincenzo Frascino
> > <vincenzo.frascino@arm.com> wrote:
> >>
> >> Hi Andrey,
> >>
> >> On 06/12/2018 12:24, Andrey Konovalov wrote:
> >>> This commit adds tag-based KASAN specific hooks implementation and
> >>> adjusts common generic and tag-based KASAN ones.
> >>>
> >>> 1. When a new slab cache is created, tag-based KASAN rounds up the size of
> >>>    the objects in this cache to KASAN_SHADOW_SCALE_SIZE (== 16).
> >>>
> >>> 2. On each kmalloc tag-based KASAN generates a random tag, sets the shadow
> >>>    memory, that corresponds to this object to this tag, and embeds this
> >>>    tag value into the top byte of the returned pointer.
> >>>
> >>> 3. On each kfree tag-based KASAN poisons the shadow memory with a random
> >>>    tag to allow detection of use-after-free bugs.
> >>>
> >>> The rest of the logic of the hook implementation is very much similar to
> >>> the one provided by generic KASAN. Tag-based KASAN saves allocation and
> >>> free stack metadata to the slab object the same way generic KASAN does.
> >>>
> >>> Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
> >>> Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
> >>> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> >>> ---
> >>>  mm/kasan/common.c | 116 ++++++++++++++++++++++++++++++++++++++--------
> >>>  mm/kasan/kasan.h  |   8 ++++
> >>>  mm/kasan/tags.c   |  48 +++++++++++++++++++
> >>>  3 files changed, 153 insertions(+), 19 deletions(-)
> >>>
> >>
> >>
> >> [...]
> >>
> >>> @@ -265,6 +290,8 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
> >>>               return;
> >>>       }
> >>>
> >>> +     cache->align = round_up(cache->align, KASAN_SHADOW_SCALE_SIZE);
> >>> +
> >>
> >> Did you consider to set ARCH_SLAB_MINALIGN instead of this round up?
> >
> > I didn't know about this macro. Looks like we can use it to do the
> > same thing. Do you think it's a better solution to redefine
> > ARCH_SLAB_MINALIGN to KASAN_SHADOW_SCALE_SIZE for arm64 when tag-based
> > KASAN is enabled instead of adjusting cache->align in
> > kasan_cache_create?
> >
>
> Yes, I think it is better because in this way we do not need to add extra code
> to do the rounding.
>
> Curiosity, did you try your patches with SLUB red zoning enabled?
> Since the area used for the Redzone is just after the payload, aligning the
> object_size independently from the allocator could have side effects, at least
> if I understand well how the mechanism works.
>
> Setting ARCH_SLAB_MINALIGN should avoid this as well.
>
> What do you think?

Sounds good to me.

Andrew, how should proceed with this? Send another fixup patch or
resend the whole series?

>
> >>
> >> --
> >> Regards,
> >> Vincenzo
> >>
> >> --
> >> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> >> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> >> To post to this group, send email to kasan-dev@googlegroups.com.
> >> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/2bf7415e-2724-b3c3-9571-20c8b6d43b92%40arm.com.
> >> For more options, visit https://groups.google.com/d/optout.
>
> --
> Regards,
> Vincenzo
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To post to this group, send email to kasan-dev@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/b99b331d-22ca-b9db-8677-4896c427ef10%40arm.com.
> For more options, visit https://groups.google.com/d/optout.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v13 19/25] kasan: add hooks implementation for tag-based mode
  2018-12-17 19:33         ` Andrey Konovalov
@ 2018-12-17 20:38           ` Andrew Morton
  2018-12-18 13:31             ` Andrey Konovalov
  0 siblings, 1 reply; 12+ messages in thread
From: Andrew Morton @ 2018-12-17 20:38 UTC (permalink / raw)
  To: Andrey Konovalov
  Cc: Vincenzo Frascino, Andrey Ryabinin, Alexander Potapenko,
	Dmitry Vyukov, Catalin Marinas, Will Deacon, Christoph Lameter,
	Mark Rutland, Nick Desaulniers, Marc Zyngier, Dave Martin,
	Ard Biesheuvel, Eric W . Biederman, Ingo Molnar, Paul Lawrence,
	Geert Uytterhoeven, Arnd Bergmann, Kirill A. Shutemov,
	Greg Kroah-Hartman <gregkh@

On Mon, 17 Dec 2018 20:33:42 +0100 Andrey Konovalov <andreyknvl@google.com> wrote:

> > Curiosity, did you try your patches with SLUB red zoning enabled?
> > Since the area used for the Redzone is just after the payload, aligning the
> > object_size independently from the allocator could have side effects, at least
> > if I understand well how the mechanism works.
> >
> > Setting ARCH_SLAB_MINALIGN should avoid this as well.
> >
> > What do you think?
> 
> Sounds good to me.
> 
> Andrew, how should proceed with this? Send another fixup patch or
> resend the whole series?

It depends on how extensive the changes are.  I prefer a fixup, but at
some point it's time to drop it all and start again.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v13 19/25] kasan: add hooks implementation for tag-based mode
  2018-12-17 20:38           ` Andrew Morton
@ 2018-12-18 13:31             ` Andrey Konovalov
  0 siblings, 0 replies; 12+ messages in thread
From: Andrey Konovalov @ 2018-12-18 13:31 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Vincenzo Frascino, Andrey Ryabinin, Alexander Potapenko,
	Dmitry Vyukov, Catalin Marinas, Will Deacon, Christoph Lameter,
	Mark Rutland, Nick Desaulniers, Marc Zyngier, Dave Martin,
	Ard Biesheuvel, Eric W . Biederman, Ingo Molnar, Paul Lawrence,
	Geert Uytterhoeven, Arnd Bergmann, Kirill A. Shutemov,
	Greg Kroah-Hartman <gregkh@

On Mon, Dec 17, 2018 at 9:38 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Mon, 17 Dec 2018 20:33:42 +0100 Andrey Konovalov <andreyknvl@google.com> wrote:
>
> > > Curiosity, did you try your patches with SLUB red zoning enabled?
> > > Since the area used for the Redzone is just after the payload, aligning the
> > > object_size independently from the allocator could have side effects, at least
> > > if I understand well how the mechanism works.
> > >
> > > Setting ARCH_SLAB_MINALIGN should avoid this as well.
> > >
> > > What do you think?
> >
> > Sounds good to me.
> >
> > Andrew, how should proceed with this? Send another fixup patch or
> > resend the whole series?
>
> It depends on how extensive the changes are.  I prefer a fixup, but at
> some point it's time to drop it all and start again.

The fixup in only a few lines, so I just sent it as a separate patch
named "[PATCH mm] kasan, arm64: use ARCH_SLAB_MINALIGN instead of
manual aligning".

Thanks!

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-12-18 13:31 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <cover.1544099024.git.andreyknvl@google.com>
     [not found] ` <5cc1b789aad7c99cf4f3ec5b328b147ad53edb40.1544099024.git.andreyknvl@google.com>
     [not found]   ` <CAP=VYLo-o8vpGrpM_+0jdvxLC9uxw+F7_OtsSfRyq24HR1dDwA@mail.gmail.com>
2018-12-10 12:12     ` [PATCH v13 08/25] kasan: initialize shadow to 0xff for tag-based mode Andrey Konovalov
2018-12-11 15:18 ` [PATCH v13 00/25] kasan: add software tag-based mode for arm64 Will Deacon
2018-12-11 15:57   ` Andrey Konovalov
2018-12-11 16:00     ` Will Deacon
2018-12-11 21:44       ` Andrew Morton
     [not found] ` <b2550106eb8a68b10fefbabce820910b115aa853.1544099024.git.andreyknvl@google.com>
2018-12-11 15:28   ` [PATCH v13 05/25] kasan: add CONFIG_KASAN_GENERIC and CONFIG_KASAN_SW_TAGS Luc Van Oostenryck
2018-12-11 16:02     ` Andrey Konovalov
     [not found] ` <bda78069e3b8422039794050ddcb2d53d053ed41.1544099024.git.andreyknvl@google.com>
     [not found]   ` <2bf7415e-2724-b3c3-9571-20c8b6d43b92@arm.com>
2018-12-12 15:04     ` [PATCH v13 19/25] kasan: add hooks implementation for tag-based mode Andrey Konovalov
2018-12-14 12:35       ` Vincenzo Frascino
2018-12-17 19:33         ` Andrey Konovalov
2018-12-17 20:38           ` Andrew Morton
2018-12-18 13:31             ` Andrey Konovalov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).