From: Alexander Potapenko <glider@google.com>
To: Dmitry Vyukov <dvyukov@google.com>
Cc: Peter Collingbourne <pcc@google.com>,
Catalin Marinas <catalin.marinas@arm.com>,
Will Deacon <will@kernel.org>, Ingo Molnar <mingo@redhat.com>,
Peter Zijlstra <peterz@infradead.org>,
Juri Lelli <juri.lelli@redhat.com>,
Vincent Guittot <vincent.guittot@linaro.org>,
Dietmar Eggemann <dietmar.eggemann@arm.com>,
Steven Rostedt <rostedt@goodmis.org>,
Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
Daniel Bristot de Oliveira <bristot@redhat.com>,
Thomas Gleixner <tglx@linutronix.de>,
Andy Lutomirski <luto@kernel.org>,
Kees Cook <keescook@chromium.org>,
Andrew Morton <akpm@linux-foundation.org>,
Masahiro Yamada <masahiroy@kernel.org>,
Sami Tolvanen <samitolvanen@google.com>,
YiFei Zhu <yifeifz2@illinois.edu>,
Colin Ian King <colin.king@canonical.com>,
Mark Rutland <mark.rutland@arm.com>,
Frederic Weisbecker <frederic@kernel.org>,
Viresh Kumar <viresh.kumar@linaro.org>,
Andrey Konovalov <andreyknvl@gmail.com>,
Gabriel Krisman Bertazi <krisman@collabora.com>,
Chris Hyser <chris.hyser@oracle.com>,
Daniel Vetter <daniel.vetter@ffwll.ch>,
Chris Wilson <chris@chris-wilson.co.uk>,
Arnd Bergmann <arnd@arndb.de>,
Christian Brauner <christian.brauner@ubuntu.com>,
"Eric W. Biederman" <ebiederm@xmission.com>,
Alexey Gladkov <legion@kernel.org>,
Ran Xiaokai <ran.xiaokai@zte.com.cn>,
David Hildenbrand <david@redhat.com>,
Xiaofeng Cao <caoxiaofeng@yulong.com>,
Cyrill Gorcunov <gorcunov@gmail.com>,
Thomas Cedeno <thomascedeno@google.com>,
Marco Elver <elver@google.com>,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
Evgenii Stepanov <eugenis@google.com>
Subject: Re: [PATCH v2 1/5] fs: use raw_copy_from_user() to copy mount() data
Date: Tue, 23 Nov 2021 11:09:05 +0100 [thread overview]
Message-ID: <CAG_fn=XnLANd=FBYuW_MvhCqZBGczfNqk0cBmQdXgra81LT+nA@mail.gmail.com> (raw)
In-Reply-To: <CACT4Y+bNA7NocP_wG+FKVPKEygWWbq4E0Zrw5bn47KvCwgQXAg@mail.gmail.com>
On Tue, Nov 23, 2021 at 8:51 AM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Tue, 23 Nov 2021 at 06:17, Peter Collingbourne <pcc@google.com> wrote:
> >
> > With uaccess logging the contract is that the kernel must not report
> > accessing more data than necessary, as this can lead to false positive
> > reports in downstream consumers. This generally works out of the box
> > when instrumenting copy_{from,to}_user(), but with the data argument
> > to mount() we use copy_from_user() to copy PAGE_SIZE bytes (or as
> > much as we can, if the PAGE_SIZE sized access failed) and figure out
> > later how much we actually need.
> >
> > To prevent this from leading to a false positive report, use
> > raw_copy_from_user(), which will prevent the access from being logged.
> > Recall that it is valid for the kernel to report accessing less
> > data than it actually accessed, as uaccess logging is a best-effort
> > mechanism for reporting uaccesses.
> >
> > Link: https://linux-review.googlesource.com/id/I5629b92a725c817acd9a861288338dd605cafee6
> > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > ---
> > fs/namespace.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/namespace.c b/fs/namespace.c
> > index 659a8f39c61a..695b30e391f0 100644
> > --- a/fs/namespace.c
> > +++ b/fs/namespace.c
> > @@ -3197,7 +3197,12 @@ static void *copy_mount_options(const void __user * data)
> > if (!copy)
> > return ERR_PTR(-ENOMEM);
> >
> > - left = copy_from_user(copy, data, PAGE_SIZE);
> > + /*
> > + * Use raw_copy_from_user to avoid reporting overly large accesses in
> > + * the uaccess buffer, as this can lead to false positive reports in
> > + * downstream consumers.
> > + */
> > + left = raw_copy_from_user(copy, data, PAGE_SIZE);
I don't really like the idea of using raw_copy_from_user() anywhere.
Right now users of instrumented.h can more or less assume they see all
usercopy events, and removing the copy_from_user() call from here
looks like a regression.
Cannot the usercopy logger decide whether it wants to log the access
based on the size (e.g. skip accesses >= PAGE_SIZE)?
Will it help if we can instrument both sides of copy_from_user() (see
the code here: https://linux-review.googlesource.com/c/linux/kernel/git/torvalds/linux/+/14103/4)?
If not, maybe we can disable/enable uaccess logging for the current
task around these accesses?
> This will skip KASAN/etc checks as well, right? I guess it is fine b/c
> this affects just this place and the code looks safe (famous last
> words :)) and we can refine it in future.
> But I wonder about false positives under KMSAN. However, we probably
> can add an explicit KMSAN annotation to mark it as initialised.
> Alex?
>
> > /*
> > * Not all architectures have an exact copy_from_user(). Resort to
> > --
> > 2.34.0.rc2.393.gf8c9666880-goog
> >
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
next prev parent reply other threads:[~2021-11-23 10:09 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-23 5:16 [PATCH v2 0/5] kernel: introduce uaccess logging Peter Collingbourne
2021-11-23 5:16 ` [PATCH v2 1/5] fs: use raw_copy_from_user() to copy mount() data Peter Collingbourne
2021-11-23 7:50 ` Dmitry Vyukov
2021-11-23 10:09 ` Alexander Potapenko [this message]
2021-12-08 3:53 ` Peter Collingbourne
2021-11-23 5:16 ` [PATCH v2 2/5] uaccess-buffer: add core code Peter Collingbourne
2021-11-23 9:56 ` Dmitry Vyukov
2021-11-23 10:08 ` Dmitry Vyukov
2021-11-23 10:19 ` Alexander Potapenko
2021-11-23 14:53 ` David Laight
2021-12-08 3:52 ` Peter Collingbourne
2021-11-23 5:16 ` [PATCH v2 3/5] uaccess-buffer: add CONFIG_GENERIC_ENTRY support Peter Collingbourne
2021-11-25 13:40 ` [uaccess] 7cd6f10220: BUG:unable_to_handle_page_fault_for_address kernel test robot
2021-11-23 5:16 ` [PATCH v2 4/5] arm64: add support for uaccess logging Peter Collingbourne
2021-11-23 5:16 ` [PATCH v2 5/5] Documentation: document " Peter Collingbourne
2021-11-23 7:46 ` Dmitry Vyukov
2021-12-10 21:29 ` Peter Collingbourne
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAG_fn=XnLANd=FBYuW_MvhCqZBGczfNqk0cBmQdXgra81LT+nA@mail.gmail.com' \
--to=glider@google.com \
--cc=akpm@linux-foundation.org \
--cc=andreyknvl@gmail.com \
--cc=arnd@arndb.de \
--cc=bristot@redhat.com \
--cc=bsegall@google.com \
--cc=caoxiaofeng@yulong.com \
--cc=catalin.marinas@arm.com \
--cc=chris.hyser@oracle.com \
--cc=chris@chris-wilson.co.uk \
--cc=christian.brauner@ubuntu.com \
--cc=colin.king@canonical.com \
--cc=daniel.vetter@ffwll.ch \
--cc=david@redhat.com \
--cc=dietmar.eggemann@arm.com \
--cc=dvyukov@google.com \
--cc=ebiederm@xmission.com \
--cc=elver@google.com \
--cc=eugenis@google.com \
--cc=frederic@kernel.org \
--cc=gorcunov@gmail.com \
--cc=juri.lelli@redhat.com \
--cc=keescook@chromium.org \
--cc=krisman@collabora.com \
--cc=legion@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mark.rutland@arm.com \
--cc=masahiroy@kernel.org \
--cc=mgorman@suse.de \
--cc=mingo@redhat.com \
--cc=pcc@google.com \
--cc=peterz@infradead.org \
--cc=ran.xiaokai@zte.com.cn \
--cc=rostedt@goodmis.org \
--cc=samitolvanen@google.com \
--cc=tglx@linutronix.de \
--cc=thomascedeno@google.com \
--cc=vincent.guittot@linaro.org \
--cc=viresh.kumar@linaro.org \
--cc=will@kernel.org \
--cc=yifeifz2@illinois.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).