Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200120141927.114373-1-elver@google.com> <CACT4Y+bnRoKinPopVqyxj4av6_xa_OUN0wwnidpO3dX3iYq_gg@mail.gmail.com>
 <CACT4Y+YuTT6kZ-AkgU0c1o09qmQdFWr4_Sds4jaDg-Va6g6jkA@mail.gmail.com>
 <CACT4Y+acrXkA-ixjQXqNf1EC=fpgTWf3Rcevxxon0DfrPdD-UQ@mail.gmail.com>
 <CANpmjNNcXUF-=Y-hmry9-xEoNpJd0WH+fOcJJM6kv2eRm5v-kg@mail.gmail.com> <CACT4Y+bD3cNxfaWOuhHz338MoVoaHpw-E8+b7v6mo_ir2KD46Q@mail.gmail.com>
In-Reply-To: <CACT4Y+bD3cNxfaWOuhHz338MoVoaHpw-E8+b7v6mo_ir2KD46Q@mail.gmail.com>
From:   Marco Elver <elver@google.com>
Date:   Mon, 20 Jan 2020 17:25:31 +0100
Message-ID: <CANpmjNN-8CLN9v7MehNUXy=iEXOfFHwpAUEPivGM573EQqmCZw@mail.gmail.com>
Subject: Re: [PATCH 1/5] include/linux: Add instrumented.h infrastructure
To:     Dmitry Vyukov <dvyukov@google.com>
Cc:     "Paul E. McKenney" <paulmck@kernel.org>,
        Andrey Konovalov <andreyknvl@google.com>,
        Alexander Potapenko <glider@google.com>,
        kasan-dev <kasan-dev@googlegroups.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Will Deacon <will@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Boqun Feng <boqun.feng@gmail.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Christophe Leroy <christophe.leroy@c-s.fr>,
        Daniel Axtens <dja@axtens.net>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Steven Rostedt <rostedt@goodmis.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Ingo Molnar <mingo@kernel.org>,
        Christian Brauner <christian.brauner@ubuntu.com>,
        Daniel Borkmann <daniel@iogearbox.net>, cyphar@cyphar.com,
        Kees Cook <keescook@chromium.org>,
        linux-arch <linux-arch@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 20 Jan 2020 at 17:06, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Mon, Jan 20, 2020 at 4:40 PM Marco Elver <elver@google.com> wrote:
> > > > > > This adds instrumented.h, which provides generic wrappers for memory
> > > > > > access instrumentation that the compiler cannot emit for various
> > > > > > sanitizers. Currently this unifies KASAN and KCSAN instrumentation. In
> > > > > > future this will also include KMSAN instrumentation.
> > > > > >
> > > > > > Note that, copy_{to,from}_user require special instrumentation,
> > > > > > providing hooks before and after the access, since we may need to know
> > > > > > the actual bytes accessed (currently this is relevant for KCSAN, and is
> > > > > > also relevant in future for KMSAN).
> > > > > >
> > > > > > Suggested-by: Arnd Bergmann <arnd@arndb.de>
> > > > > > Signed-off-by: Marco Elver <elver@google.com>
> > > > > > ---
> > > > > >  include/linux/instrumented.h | 153 +++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 153 insertions(+)
> > > > > >  create mode 100644 include/linux/instrumented.h
> > > > > >
> > > > > > diff --git a/include/linux/instrumented.h b/include/linux/instrumented.h
> > > > > > new file mode 100644
> > > > > > index 000000000000..9f83c8520223
> > > > > > --- /dev/null
> > > > > > +++ b/include/linux/instrumented.h
> > > > > > @@ -0,0 +1,153 @@
> > > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > > +
> > > > > > +/*
> > > > > > + * This header provides generic wrappers for memory access instrumentation that
> > > > > > + * the compiler cannot emit for: KASAN, KCSAN.
> > > > > > + */
> > > > > > +#ifndef _LINUX_INSTRUMENTED_H
> > > > > > +#define _LINUX_INSTRUMENTED_H
> > > > > > +
> > > > > > +#include <linux/compiler.h>
> > > > > > +#include <linux/kasan-checks.h>
> > > > > > +#include <linux/kcsan-checks.h>
> > > > > > +#include <linux/types.h>
> > > > > > +
> > > > > > +/**
> > > > > > + * instrument_read - instrument regular read access
> > > > > > + *
> > > > > > + * Instrument a regular read access. The instrumentation should be inserted
> > > > > > + * before the actual read happens.
> > > > > > + *
> > > > > > + * @ptr address of access
> > > > > > + * @size size of access
> > > > > > + */
> > > > >
> > > > > Based on offline discussion, that's what we add for KMSAN:
> > > > >
> > > > > > +static __always_inline void instrument_read(const volatile void *v, size_t size)
> > > > > > +{
> > > > > > +       kasan_check_read(v, size);
> > > > > > +       kcsan_check_read(v, size);
> > > > >
> > > > > KMSAN: nothing
> > > >
> > > > KMSAN also has instrumentation in
> > > > copy_to_user_page/copy_from_user_page. Do we need to do anything for
> > > > KASAN/KCSAN for these functions?
> >
> > copy_to_user_page/copy_from_user_page can be instrumented with
> > instrument_copy_{to,from}_user_. I prefer keeping this series with no
> > functional change intended for KASAN at least.
> >
> > > There is also copy_user_highpage.
> > >
> > > And ioread/write8/16/32_rep: do we need any instrumentation there. It
> > > seems we want both KSAN and KCSAN too. One may argue that KCSAN
> > > instrumentation there is to super critical at this point, but KASAN
> > > instrumentation is important, if anything to prevent silent memory
> > > corruptions. How do we instrument there? I don't see how it maps to
> > > any of the existing instrumentation functions.
> >
> > These should be able to use the regular instrument_{read,write}. I
> > prefer keeping this series with no functional change intended for
> > KASAN at least.
>
> instrument_{read,write} will not contain any KMSAN instrumentation,
> which means we will effectively remove KMSAN instrumentation, which is
> weird because we instrumented these functions because of KMSAN in the
> first place...
>
> > > There is also kmsan_check_skb/kmsan_handle_dma/kmsan_handle_urb that
> > > does not seem to map to any of the instrumentation functions.
> >
> > For now, I would rather that there are some one-off special
> > instrumentation, like for KMSAN. Coming up with a unified interface
> > here that, without the use-cases even settled, seems hard to justify.
> > Once instrumentation for these have settled, unifying the interface
> > would have better justification.
>
> I would assume they may also require an annotation that checks the
> memory region under all 3 tools and we don't have such annotation
> (same as the previous case and effectively copy_to_user). I would
> expect such annotation will be used in more places once we start
> looking for more opportunities.

Agreed, I'm certainly not against adding these. We may need to
introduce 'instrument_dma_' etc. However, would it be reasonable to do
this in a separate follow-up patch-series, to avoid stalling bitops
instrumentation?  Assuming that the 8 hooks in instrumented.h right
now are reasonable, and such future changes add new hooks, I think
that would be the more pragmatic approach.

Thanks,
-- Marco

>
> > This patch series is merely supposed to introduce instrumented.h and
> > replace the kasan_checks (also implicitly introducing kcsan_checks
> > there), however, with no further functional change intended.
> >
> > I propose that adding entirely new instrumentation for both KASAN and
> > KCSAN, we should send a separate patch-series.
> >
> > Thanks,
> > -- Marco