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-kbuild" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
