Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211001024105.3217339-1-willy@infradead.org> <CA+fCnZfSUxToYKUfHwQT0r3bC9NYZNc2iC3PXv+GciuW0Fm79A@mail.gmail.com>
 <YVcVtNLnyJModOhn@casper.infradead.org>
In-Reply-To: <YVcVtNLnyJModOhn@casper.infradead.org>
From:   Andrey Konovalov <andreyknvl@gmail.com>
Date:   Sun, 3 Oct 2021 18:27:09 +0200
Message-ID: <CA+fCnZd7dGOz2T3eVwbJzAEmXMB7YezB5FoZt5a0D92mPPv74g@mail.gmail.com>
Subject: Re: [PATCH] kasan: Fix tag for large allocations when using CONFIG_SLAB
To:     Matthew Wilcox <willy@infradead.org>
Cc:     Andrey Ryabinin <ryabinin.a.a@gmail.com>,
        Alexander Potapenko <glider@google.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        kasan-dev <kasan-dev@googlegroups.com>,
        Linux Memory Management List <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Oct 1, 2021 at 4:06 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Fri, Oct 01, 2021 at 03:29:29PM +0200, Andrey Konovalov wrote:
> > On Fri, Oct 1, 2021 at 4:42 AM Matthew Wilcox (Oracle)
> > <willy@infradead.org> wrote:
> > >
> > > If an object is allocated on a tail page of a multi-page slab, kasan
> > > will get the wrong tagbecause page->s_mem is NULL for tail pages.
> >
> > Interesting. Is this a known property of tail pages? Why does this
> > happen? I failed to find this exception in the code.
>
> Yes, it's a known property of tail pages.  kmem_getpages() calls
> __alloc_pages_node() which returns a pointer to the head page.
> All the tail pages are initialised to point to the head page.
> Then in alloc_slabmgmt(), we set ->s_mem of the head page, but
> we never set ->s_mem of the tail pages.  Instead, we rely on
> people always passing in the head page.  I have a patch in the works
> to change the type from struct page to struct slab so you can't
> make this mistake.  That was how I noticed this problem.

Ah, so it's not "the tail page", it's "a tail page". Meaning any page
but the head page. Got it.

> > The tag value won't really be "wrong", just unexpected. But if s_mem
> > is indeed NULL for tail pages, your fix makes sense.
> >
> > > I'm not quite sure what the user-visible effect of this might be.
> >
> > Everything should work, as long as tag values are assigned
> > consistently based on the object address.
>
> OK, maybe this doesn't need to be backported then?  Actually, why
> subtract s_mem in the first place?  Can we just avoid that for all
> tag calculations?

We could avoid it. To me, it seems cleaner to assign tags based on the
object index rather than on the absolute address. But either way
should work.

There's no security nor stability impact from this issue, so probably
not so much incentive to backport. But the patch makes sense.

Thanks!