| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHRSSEzj10quNV6mK9QQezPRMyO49xNA14O4wCsx+eY_UkjQWQ@mail.gmail.com>
Date: Wed, 24 Nov 2021 12:33:20 -0800
From: Todd Kjos <tkjos@...gle.com>
To: Dan Carpenter <dan.carpenter@...cle.com>
Cc: gregkh@...uxfoundation.org, christian@...uner.io, arve@...roid.com,
devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
maco@...gle.com, joel@...lfernandes.org, kernel-team@...roid.com
Subject: Re: [PATCH 2/3] binder: read pre-translated fds from sender buffer
On Wed, Nov 24, 2021 at 4:37 AM Dan Carpenter <dan.carpenter@...cle.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@...gle.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@...roid.com.
>
Powered by blists - more mailing lists