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: <cover.1609871239.git.andreyknvl@google.com> <a83aa371e2ef96e79cbdefceebaa960a34957a79.1609871239.git.andreyknvl@google.com>
 <X/2zBibnd/zCBFa/@elver.google.com>
In-Reply-To: <X/2zBibnd/zCBFa/@elver.google.com>
From:   Andrey Konovalov <andreyknvl@google.com>
Date:   Tue, 12 Jan 2021 22:16:38 +0100
Message-ID: <CAAeHK+y0nmeDEWG8ZMX9KmE3-MhWCtrssDJi5oHG2PFNtrDK_g@mail.gmail.com>
Subject: Re: [PATCH 10/11] kasan: fix bug detection via ksize for HW_TAGS mode
To:     Marco Elver <elver@google.com>
Cc:     Catalin Marinas <catalin.marinas@arm.com>,
        Vincenzo Frascino <vincenzo.frascino@arm.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        Alexander Potapenko <glider@google.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Will Deacon <will.deacon@arm.com>,
        Andrey Ryabinin <aryabinin@virtuozzo.com>,
        Evgenii Stepanov <eugenis@google.com>,
        Branislav Rankov <Branislav.Rankov@arm.com>,
        Kevin Brodsky <kevin.brodsky@arm.com>,
        kasan-dev <kasan-dev@googlegroups.com>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        Linux Memory Management List <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Jan 12, 2021 at 3:32 PM Marco Elver <elver@google.com> wrote:
>
> > +/*
> > + * Unlike kasan_check_read/write(), kasan_check_byte() is performed even for
> > + * the hardware tag-based mode that doesn't rely on compiler instrumentation.
> > + */
>
> We have too many check-functions, and the name needs to be more precise.
> Intuitively, I would have thought this should have access-type, i.e.
> read or write, effectively mirroring a normal access.
>
> Would kasan_check_byte_read() be better (and just not have a 'write'
> variant because we do not need it)? This would restore ksize() closest
> to what it was before (assuming reporting behaviour is fixed, too).

> >  void kasan_poison(const void *address, size_t size, u8 value);
> >  void kasan_unpoison(const void *address, size_t size);
> > -bool kasan_check_invalid_free(void *addr);
> > +bool kasan_check(const void *addr);
>
> Definitely prefer shorted names, but we're in the unfortunate situation
> of having numerous kasan_check-functions, so we probably need to be more
> precise.
>
> kasan_check() makes me think this also does reporting, but it does not
> (it seems to only check the metadata for validity).
>
> The internal function could therefore be kasan_check_allocated() (it's
> now the inverse of kasan_check_invalid_free()).

Re: kasan_check_byte():

I think the _read suffix is only making the name longer. ksize() isn't
checking that the memory is readable (or writable), it's checking that
it's addressable. At least that's the intention of the annotation, so
it makes sense to name it correspondingly despite the implementation.

Having all kasan_check_*() functions both checking and reporting makes
sense, so let's keep the kasan_check_ prefix.

What isn't obvious from the name is that this function is present for
every kasan mode. Maybe kasan_check_byte_always()? Although it also
seems too long.

But I'm OK with keeping kasan_check_byte().

Re kasan_check():

Here we can use Andrew's suggestion about the name being related to
what the function returns. And also drop the kasan_check_ prefix as
this function only does the checking.

Let's use kasan_byte_accessible() instead of kasan_check().

> > +bool __kasan_check_byte(const void *address, unsigned long ip)
> > +{
> > +     if (!kasan_check(address)) {
> > +             kasan_report_invalid_free((void *)address, ip);
>
> This is strange: why does it report an invalid free? Should this be a
> use-after-free? I think this could just call kasan_report(....) for 1
> byte, and we'd get the right report.

Will fix in v2.

Thanks!