Re: [PATCH 3/3] asm-generic, x86: Add bitops instrumentation for KASAN

[Marco Elver]

On Wed, 29 May 2019 at 10:53, Dmitry Vyukov <dvyukov@xxxxxxxxxx> wrote:
>
> On Tue, May 28, 2019 at 6:50 PM Mark Rutland <mark.rutland@xxxxxxx> wrote:
> >
> > On Tue, May 28, 2019 at 06:32:58PM +0200, Marco Elver wrote:
> > > This adds a new header to asm-generic to allow optionally instrumenting
> > > architecture-specific asm implementations of bitops.
> > >
> > > This change includes the required change for x86 as reference and
> > > changes the kernel API doc to point to bitops-instrumented.h instead.
> > > Rationale: the functions in x86's bitops.h are no longer the kernel API
> > > functions, but instead the arch_ prefixed functions, which are then
> > > instrumented via bitops-instrumented.h.
> > >
> > > Other architectures can similarly add support for asm implementations of
> > > bitops.
> > >
> > > The documentation text has been copied/moved, and *no* changes to it
> > > have been made in this patch.
> > >
> > > Tested: using lib/test_kasan with bitops tests (pre-requisite patch).
> > >
> > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198439
> > > Signed-off-by: Marco Elver <elver@xxxxxxxxxx>
> > > ---
> > >  Documentation/core-api/kernel-api.rst     |   2 +-
> > >  arch/x86/include/asm/bitops.h             | 210 ++++----------
> > >  include/asm-generic/bitops-instrumented.h | 327 ++++++++++++++++++++++
> > >  3 files changed, 380 insertions(+), 159 deletions(-)
> > >  create mode 100644 include/asm-generic/bitops-instrumented.h
> >
> > [...]
> >
> > > +#if !defined(BITOPS_INSTRUMENT_RANGE)
> > > +/*
> > > + * This may be defined by an arch's bitops.h, in case bitops do not operate on
> > > + * single bytes only. The default version here is conservative and assumes that
> > > + * bitops operate only on the byte with the target bit.
> > > + */
> > > +#define BITOPS_INSTRUMENT_RANGE(addr, nr)                                  \
> > > +     (const volatile char *)(addr) + ((nr) / BITS_PER_BYTE), 1
> > > +#endif
> >
> > I was under the impression that logically, all the bitops operated on
> > the entire long the bit happend to be contained in, so checking the
> > entire long would make more sense to me.
> >
> > FWIW, arm64's atomic bit ops are all implemented atop of atomic_long_*
> > functions, which are instrumented, and always checks at the granularity
> > of a long. I haven't seen splats from that when fuzzing with Syzkaller.
> >
> > Are you seeing bugs without this?
>
> bitops are not instrumented on x86 at all at the moment, so we have
> not seen any splats. What we've seen are assorted crashes caused by
> previous silent memory corruptions by incorrect bitops :)
>
> Good point. If arm already does this, I guess we also need to check
> whole long's.

For the default, we decided to err on the conservative side for now,
since it seems that e.g. x86 operates only on the byte the bit is on.
Other architectures that need bitops-instrumented.h may redefine
BITOPS_INSTRUMENT_RANGE.

Let me know what you prefer.

Thanks,
-- Marco