LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Todd Kjos <tkjos@google.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: gregkh@linuxfoundation.org, christian@brauner.io,
	arve@android.com, devel@driverdev.osuosl.org,
	linux-kernel@vger.kernel.org, maco@google.com,
	joel@joelfernandes.org, kernel-team@android.com
Subject: Re: [PATCH 2/3] binder: read pre-translated fds from sender buffer
Date: Wed, 24 Nov 2021 12:33:20 -0800	[thread overview]
Message-ID: <CAHRSSEzj10quNV6mK9QQezPRMyO49xNA14O4wCsx+eY_UkjQWQ@mail.gmail.com> (raw)
In-Reply-To: <20211124123719.GG6514@kadam>

On Wed, Nov 24, 2021 at 4:37 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Tue, Nov 23, 2021 at 11:17:36AM -0800, Todd Kjos wrote:
> > Since we are no longer going to copy the pre-fixup
> > data from the target buffer, we need to read
> > pre-translated FD array information from the source
> > buffer.
> >
>
> The commit message is really misleading.  From the commit message it
> sounds like the commit is changing runtime but it's not.  What I want is
> a commit message like this:
>
>   This patch is to prepare for an up coming patch where we read
>   pre-translated fds from the sender buffer and translate them before
>   copying them to the target.  It does not change run time.
>
>   The patch adds two new parameters to binder_translate_fd_array() to
>   hold the sender buffer and sender buffer parent.  These parameters let
>   us call copy_from_user() directly instead of using
>   binder_alloc_copy_from_buffer() which is a cleanup.  Also the patch
>   adds some new alignment checks.  Previously the alignment checks would
>   have been done in a different place, but this lets us print more
>   useful error messages.

Got it. Will fix in update.

>
>
> > Signed-off-by: Todd Kjos <tkjos@google.com>
> > ---
> >  drivers/android/binder.c | 40 +++++++++++++++++++++++++++++++++-------
> >  1 file changed, 33 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index 571d3c203557..2300fa8e09d5 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -2234,15 +2234,17 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
> >  }
> >
> >  static int binder_translate_fd_array(struct binder_fd_array_object *fda,
> > +                                  const void __user *u,
>
> I wish we could use sender/target terminology everywhere.  Please change
> every place that has "u" or "user" to either "sender" or "target" as
> appropriate.

Ok (all "u/user" cases are sender).

>
> >                                    struct binder_buffer_object *parent,
> > +                                  struct binder_buffer_object *uparent,
>                                                                   ^
>
> >                                    struct binder_transaction *t,
> >                                    struct binder_thread *thread,
> >                                    struct binder_transaction *in_reply_to)
> >  {
> >       binder_size_t fdi, fd_buf_size;
> >       binder_size_t fda_offset;
> > +     const void __user *ufda_base;
>                            ^
>
> >       struct binder_proc *proc = thread->proc;
> > -     struct binder_proc *target_proc = t->to_proc;
> >
> >       fd_buf_size = sizeof(u32) * fda->num_fds;
> >       if (fda->num_fds >= SIZE_MAX / sizeof(u32)) {
> > @@ -2266,7 +2268,10 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
> >        */>    fda_offset = (parent->buffer - (uintptr_t)t->buffer->user_data) +
> >               fda->parent_offset;
> > -     if (!IS_ALIGNED((unsigned long)fda_offset, sizeof(u32))) {
> > +     ufda_base = (void __user *)uparent->buffer + fda->parent_offset;
> > +
> > +     if (!IS_ALIGNED((unsigned long)fda_offset, sizeof(u32)) ||
> > +         !IS_ALIGNED((unsigned long)ufda_base, sizeof(u32))) {
> >               binder_user_error("%d:%d parent offset not aligned correctly.\n",
> >                                 proc->pid, thread->pid);
> >               return -EINVAL;
> > @@ -2275,10 +2280,9 @@ static int binder_translate_fd_array(struct binder_fd_array_object *fda,
> >               u32 fd;
> >               int ret;
> >               binder_size_t offset = fda_offset + fdi * sizeof(fd);
> > +             binder_size_t uoffset = fdi * sizeof(fd);
> >
> > -             ret = binder_alloc_copy_from_buffer(&target_proc->alloc,
> > -                                                 &fd, t->buffer,
> > -                                                 offset, sizeof(fd));
> > +             ret = copy_from_user(&fd, ufda_base + uoffset, sizeof(fd));
> >               if (!ret)
> >                       ret = binder_translate_fd(fd, offset, t, thread,
> >                                                 in_reply_to);
>
> This is something from the original code but if the copy_from_user()
> fails, then we just skip that "fd" without informing the user.

Here is the whole code fragment(the diff doesn't include the "if
(ret<0) return ret"):

  ret = copy_from_user(&fd, ufda_base + uoffset, sizeof(fd));
  if (!ret)
    ret = binder_translate_fd(fd, offset, t, thread, in_reply_to);
  if (ret < 0)
    return ret;

I agree -- if copy_from_user() for some reason doesn't copy the whole
buffer, it might return a positive integer. Then it would skip
binder_translate_fd(), but not return. That should probably be
something like:

  if (ret)
    return ret > 0 ? -EINVAL : ret;

Will fix in next version.

> It feels wrong.  Does that not lead to a bug in the target app?
>

If copy_from_user() returned a positive integer, we would translate
some random fd. Most likely it would be invalid, but it might not be.
Definitely would be a bug.

>
> > @@ -2951,6 +2955,8 @@ static void binder_transaction(struct binder_proc *proc,
> >               case BINDER_TYPE_FDA: {
> >                       struct binder_object ptr_object;
> >                       binder_size_t parent_offset;
> > +                     struct binder_object user_object;
> > +                     size_t user_parent_size;
> >                       struct binder_fd_array_object *fda =
> >                               to_binder_fd_array_object(hdr);
> >                       size_t num_valid = (buffer_offset - off_start_offset) /
> > @@ -2982,8 +2988,28 @@ static void binder_transaction(struct binder_proc *proc,
> >                               return_error_line = __LINE__;
> >                               goto err_bad_parent;
> >                       }
> > -                     ret = binder_translate_fd_array(fda, parent, t, thread,
> > -                                                     in_reply_to);
> > +
> > +                     /*
> > +                      * We need to read the user version of the parent
> > +                      * object to get the original user offset
> > +                      */
> > +                     user_parent_size =
> > +                             binder_get_object(proc, user_buffer, t->buffer,
> > +                                               parent_offset, &user_object);
> > +                     if (user_parent_size != sizeof(user_object.bbo)) {
> > +                             binder_user_error("%d:%d invalid ptr object size: %lld vs %lld\n",
>
> Apparently %lld breaks the build on my .config.  The correct format for
> size_t is %zd.
>
> > +                                               proc->pid, thread->pid,
> > +                                               user_parent_size,
> > +                                               sizeof(user_object.bbo));
> > +                             return_error = BR_FAILED_REPLY;
> > +                             return_error_param = -EINVAL;
> > +                             return_error_line = __LINE__;
> > +                             goto err_bad_parent;
> > +                     }
> > +                     ret = binder_translate_fd_array(fda, user_buffer,
> > +                                                     parent,
> > +                                                     &user_object.bbo, t,
> > +                                                     thread, in_reply_to);
>
> regards,
> dan carpenter
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

  reply	other threads:[~2021-11-24 20:33 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-23 19:17 [PATCH 0/3] binder: Prevent untranslated sender data from being copied to target Todd Kjos
2021-11-23 19:17 ` [PATCH 1/3] binder: avoid potential data leakage when copying txn Todd Kjos
2021-11-24  7:50   ` Martijn Coenen
2021-11-24 13:01   ` Dan Carpenter
2021-11-24 20:11     ` Todd Kjos
2021-11-25  2:05   ` kernel test robot
2021-11-25 12:18   ` kernel test robot
2021-11-23 19:17 ` [PATCH 2/3] binder: read pre-translated fds from sender buffer Todd Kjos
2021-11-24  7:50   ` Martijn Coenen
2021-11-24 12:37   ` Dan Carpenter
2021-11-24 20:33     ` Todd Kjos [this message]
2021-11-25  6:37       ` Dan Carpenter
2021-11-23 19:17 ` [PATCH 3/3] binder: defer copies of pre-patched txn data Todd Kjos
2021-11-24  7:50   ` Martijn Coenen
2021-11-24 11:09   ` Dan Carpenter
2021-11-24 20:39     ` Todd Kjos
2021-11-24 12:43   ` Dan Carpenter
2021-11-24 20:37     ` Todd Kjos
2021-11-24  8:08 ` [PATCH 0/3] binder: Prevent untranslated sender data from being copied to target Greg KH
2021-11-24 15:54   ` Todd Kjos

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=CAHRSSEzj10quNV6mK9QQezPRMyO49xNA14O4wCsx+eY_UkjQWQ@mail.gmail.com \
    --to=tkjos@google.com \
    --cc=arve@android.com \
    --cc=christian@brauner.io \
    --cc=dan.carpenter@oracle.com \
    --cc=devel@driverdev.osuosl.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=joel@joelfernandes.org \
    --cc=kernel-team@android.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maco@google.com \
    --subject='Re: [PATCH 2/3] binder: read pre-translated fds from sender buffer' \
    /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

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).