Re: [RFC/PATCH RESEND -next 14/21] mm: slub: kasan: disable kasan when touching unaccessible memory

[Joonsoo Kim]

On Tue, Jul 15, 2014 at 11:37:56AM +0400, Andrey Ryabinin wrote:
> On 07/15/14 10:04, Joonsoo Kim wrote:
> > On Wed, Jul 09, 2014 at 03:30:08PM +0400, Andrey Ryabinin wrote:
> >> Some code in slub could validly touch memory marked by kasan as unaccessible.
> >> Even though slub.c doesn't instrumented, functions called in it are instrumented,
> >> so to avoid false positive reports such places are protected by
> >> kasan_disable_local()/kasan_enable_local() calls.
> >>
> >> Signed-off-by: Andrey Ryabinin <a.ryabinin@xxxxxxxxxxx>
> >> ---
> >>  mm/slub.c | 21 +++++++++++++++++++--
> >>  1 file changed, 19 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/mm/slub.c b/mm/slub.c
> >> index 6ddedf9..c8dbea7 100644
> >> --- a/mm/slub.c
> >> +++ b/mm/slub.c
> >> @@ -560,8 +560,10 @@ static void print_tracking(struct kmem_cache *s, void *object)
> >>  	if (!(s->flags & SLAB_STORE_USER))
> >>  		return;
> >>  
> >> +	kasan_disable_local();
> >>  	print_track("Allocated", get_track(s, object, TRACK_ALLOC));
> >>  	print_track("Freed", get_track(s, object, TRACK_FREE));
> >> +	kasan_enable_local();
> > 
> > I don't think that this is needed since print_track() doesn't call
> > external function with object pointer. print_track() call pr_err(), but,
> > before calling, it retrieve t->addrs[i] so memory access only occurs
> > in slub.c.
> > 
> Agree.
> 
> >>  }
> >>  
> >>  static void print_page_info(struct page *page)
> >> @@ -604,6 +606,8 @@ static void print_trailer(struct kmem_cache *s, struct page *page, u8 *p)
> >>  	unsigned int off;	/* Offset of last byte */
> >>  	u8 *addr = page_address(page);
> >>  
> >> +	kasan_disable_local();
> >> +
> >>  	print_tracking(s, p);
> >>  
> >>  	print_page_info(page);
> >> @@ -632,6 +636,8 @@ static void print_trailer(struct kmem_cache *s, struct page *page, u8 *p)
> >>  		/* Beginning of the filler is the free pointer */
> >>  		print_section("Padding ", p + off, s->size - off);
> >>  
> >> +	kasan_enable_local();
> >> +
> >>  	dump_stack();
> >>  }
> > 
> > And, I recommend that you put this hook on right place.
> > At a glance, the problematic function is print_section() which have
> > external function call, print_hex_dump(), with object pointer.
> > If you disable kasan in print_section, all the below thing won't be
> > needed, I guess.
> > 
> 
> Nope, at least memchr_inv() call in slab_pad_check will be a problem.
> 
> I think putting disable/enable only where we strictly need them might be a problem for future maintenance of slub.
> If someone is going to add a new function call somewhere, he must ensure that it this call won't be a problem
> for kasan.

I don't agree with this.

If someone is going to add a slab_pad_check() in other places in
slub.c, we should disable/enable kasan there, too. This looks same
maintenance problem to me. Putting disable/enable only where we
strictly need at least ensures that we don't need to care when using
slub internal functions.

And, if memchr_inv() is problem, I think that you also need to add hook
into validate_slab_cache().

validate_slab_cache() -> validate_slab_slab() -> validate_slab() ->
check_object() -> check_bytes_and_report() -> memchr_inv()

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/