Message-ID: <CAG48ez014nW8pie91cnrn_7N1zyziAN+9xrT9xN0iLecCoRwfA@mail.gmail.com>
Date:   Thu, 5 Mar 2020 13:45:44 +0100
From:   Jann Horn <jannh@...gle.com>
To:     Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:     Alexander Potapenko <glider@...gle.com>,
        Joe Perches <joe@...ches.com>, Todd Kjos <tkjos@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arve Hjønnevåg <arve@...roid.com>,
        Ingo Molnar <mingo@...hat.com>,
        Dmitriy Vyukov <dvyukov@...gle.com>,
        "open list:ANDROID DRIVERS" <devel@...verdev.osuosl.org>,
        Peter Zijlstra <peterz@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/3] binder: do not initialize locals passed to copy_from_user()

On Thu, Mar 5, 2020 at 10:03 AM Rasmus Villemoes
<linux@...musvillemoes.dk> wrote:
> On 02/03/2020 19.31, Jann Horn wrote:
> > On Mon, Mar 2, 2020 at 7:17 PM Alexander Potapenko <glider@...gle.com> wrote:
> >> On Mon, Mar 2, 2020 at 3:00 PM Joe Perches <joe@...ches.com> wrote:
> >>>
> >>> So?  CONFIG_INIT_STACK_ALL by design slows down code.
> >> Correct.
> >>
> >>> This marking would likely need to be done for nearly all
> >>> 3000+ copy_from_user entries.
> >> Unfortunately, yes. I was just hoping to do so for a handful of hot
> >> cases that we encounter, but in the long-term a compiler solution must
> >> supersede them.
> >>
> >>> Why not try to get something done on the compiler side
> >>> to mark the function itself rather than the uses?
> >> This is being worked on in the meantime as well (see
> >> http://lists.llvm.org/pipermail/cfe-dev/2020-February/064633.html)
> >> Do you have any particular requisitions about how this should look on
> >> the source level?
> >
> > Just thinking out loud: Should this be a function attribute, or should
> > it be a builtin - something like __builtin_assume_initialized(ptr,
> > len)? That would make it also work for macros,
>
> But with macros (and static inlines), the compiler sees all the
> initialization being done, no?

Depends on how the macro writes to the buffer, whether it's a normal
write or happens through another function call or whatever.

> and it might simplify
> > the handling of inlining in the compiler. And you wouldn't need such a
> > complicated attribute that refers to function arguments by index and
> > such.
>
> Does copy_from_user guarantee to zero-initialize the remaining buffer if
> copying fails partway through?

Basically yes. From include/linux/uaccess.h:

static __always_inline unsigned long __must_check
copy_from_user(void *to, const void __user *from, unsigned long n)
{
  if (likely(check_copy_size(to, n, false)))
    n = _copy_from_user(to, from, n);
  return n;
}

check_copy_size() should be optimized out entirely for straightforward
use of stack objects; it will only return false if the specified
address range crosses beyond an allocation boundary.
_copy_from_user() is defined as follows (there are two possible
definitions, both of them have the same method body, but they differ
in whether the function is inline - which one is used depends on the
architecture):

static inline __must_check unsigned long
_copy_from_user(void *to, const void __user *from, unsigned long n)
{
  unsigned long res = n;
  might_fault();
  if (likely(access_ok(from, n))) {
    kasan_check_write(to, n);
    res = raw_copy_from_user(to, from, n);
  }
  if (unlikely(res))
    memset(to + (n - res), 0, res);
  return res;
}

So annotating _copy_from_user(), or calling a magic
fake-initialization builtin directly before calling _copy_from_user(),
should be safe. As long as the compiler can eliminate the call to
check_copy_size(), that should then make that propagate up to the
caller of copy_from_user(). (You could also try to annotate
copy_from_user() directly, but I'm not sure whether doing it before
the bounds check might confuse the compiler somehow.)

Powered by blists - more mailing lists