Received-SPF: pass (google.com: domain of linux-kernel+bounces-108739-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240319163656.2100766-1-glider@google.com> <20240319163656.2100766-3-glider@google.com>
 <CAHk-=wiUf3Eqqz3PttTCBLyDKqwW2sdpeqjL+PuKtip15vDauA@mail.gmail.com>
In-Reply-To: <CAHk-=wiUf3Eqqz3PttTCBLyDKqwW2sdpeqjL+PuKtip15vDauA@mail.gmail.com>
From: Alexander Potapenko <glider@google.com>
Date: Wed, 20 Mar 2024 11:12:23 +0100
Message-ID: <CAG_fn=WRz22XEV_Em+M2FJsNjuBr3mZFT7aA5G8YdT4OTf1p1g@mail.gmail.com>
Subject: Re: [PATCH v1 3/3] x86: call instrumentation hooks from copy_mc.c
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: akpm@linux-foundation.org, linux-kernel@vger.kernel.org, 
	linux-mm@kvack.org, kasan-dev@googlegroups.com, tglx@linutronix.de, 
	x86@kernel.org, Dmitry Vyukov <dvyukov@google.com>, Marco Elver <elver@google.com>, 
	Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Mar 19, 2024 at 6:58=E2=80=AFPM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Tue, 19 Mar 2024 at 09:37, Alexander Potapenko <glider@google.com> wro=
te:
> >
> >         if (copy_mc_fragile_enabled) {
> >                 __uaccess_begin();
> > +               instrument_copy_to_user(dst, src, len);
> >                 ret =3D copy_mc_fragile((__force void *)dst, src, len);
> >                 __uaccess_end();
>
> I'd actually prefer that instrument_copy_to_user() to be *outside* the
> __uaccess_begin.

Good point, this is doable.

>
> In fact, I'm a bit surprised that objtool didn't complain about it in tha=
t form.

This is because a bunch of KMSAN functions is ignored by objtool:
https://elixir.bootlin.com/linux/latest/source/tools/objtool/check.c#L1200

> __uaccess_begin() causes the CPU to accept kernel accesses to user
> mode, and I don't think instrument_copy_to_user() has any business
> actually touching user mode memory.

Ack.

> In fact it might be better to rename the function and change the prototyp=
e to
>
>    instrument_src(src, len);
>
> because you really can't sanely instrument the destination of a user
> copy, but "instrument_src()" might be useful in other situations than
> just user copies.

Right now at least for KMSAN it is important to distinguish between a
usercopy and e.g. a URB submission: both are checked using the same
function, but depending on what is happening the report title is
different.

The destination parameter is also used by KMSAN to print fancier error repo=
rts.
For an infoleak we show the target userspace address together with
other information, e.g.:

  BUG: KMSAN: kernel-infoleak in instrument_copy_to_user
include/linux/instrumented.h:114 [inline]
  ...
  Bytes 34-35 of 36 are uninitialized
  Memory access of size 36 starts at ffff8881152e5680
  Data copied to user address 00007ffc9a4a12a0

It comes in handy when debugging reproducers locally.

Future debugging tools may also need more insight into the semantics
of the instrumented accesses.