| 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
| ||
|
Date: Tue, 19 Apr 2022 11:24:13 -0700
From: Todd Kjos <tkjos@...gle.com>
To: Carlos Llamas <cmllamas@...gle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Arve Hjønnevåg <arve@...roid.com>,
Todd Kjos <tkjos@...roid.com>,
Martijn Coenen <maco@...roid.com>,
Joel Fernandes <joel@...lfernandes.org>,
Christian Brauner <brauner@...nel.org>,
Hridya Valsaraju <hridya@...gle.com>,
Suren Baghdasaryan <surenb@...gle.com>,
kernel-team@...roid.com, linux-kernel@...r.kernel.org,
Christian Brauner <christian.brauner@...ntu.com>,
Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH] binder: hold fd_install until allocating fds first
On Fri, Mar 25, 2022 at 4:25 PM Carlos Llamas <cmllamas@...gle.com> wrote:
>
> Al noted in [1] that fd_install can't be undone, so it must come last in
> the fd translation sequence, only after we've successfully reserved all
> descriptors and copied them into the transaction buffer.
>
> This patch takes Al's proposed fix in [2] and makes a few tweaks to fold
> the traversal of t->fd_fixups during release.
>
> [1] https://lore.kernel.org/driverdev-devel/YHnJwRvUhaK3IM0l@zeniv-ca.linux.org.uk
> [2] https://lore.kernel.org/driverdev-devel/YHo6Ln9VI1T7RmLK@zeniv-ca.linux.org.uk
>
> Cc: Christian Brauner <christian.brauner@...ntu.com>
> Suggested-by: Al Viro <viro@...iv.linux.org.uk>
> Signed-off-by: Carlos Llamas <cmllamas@...gle.com>
Acked-by: Todd Kjos <tkjos@...gle.com>
> ---
> drivers/android/binder.c | 34 ++++++++++++-------------------
> drivers/android/binder_internal.h | 2 ++
> 2 files changed, 15 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 8351c5638880..bfadc0c4a442 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -1481,6 +1481,8 @@ static void binder_free_txn_fixups(struct binder_transaction *t)
>
> list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) {
> fput(fixup->file);
> + if (fixup->target_fd >= 0)
> + put_unused_fd(fixup->target_fd);
> list_del(&fixup->fixup_entry);
> kfree(fixup);
> }
> @@ -2220,6 +2222,7 @@ static int binder_translate_fd(u32 fd, binder_size_t fd_offset,
> }
> fixup->file = file;
> fixup->offset = fd_offset;
> + fixup->target_fd = -1;
> trace_binder_transaction_fd_send(t, fd, fixup->offset);
> list_add_tail(&fixup->fixup_entry, &t->fd_fixups);
>
> @@ -4067,10 +4070,9 @@ static int binder_wait_for_work(struct binder_thread *thread,
> * Now that we are in the context of the transaction target
> * process, we can allocate and install fds. Process the
> * list of fds to translate and fixup the buffer with the
> - * new fds.
> + * new fds first and only then install the files.
> *
> - * If we fail to allocate an fd, then free the resources by
> - * fput'ing files that have not been processed and ksys_close'ing
> + * If we fail to allocate an fd, skip the install and release
> * any fds that have already been allocated.
> */
> static int binder_apply_fd_fixups(struct binder_proc *proc,
> @@ -4087,41 +4089,31 @@ static int binder_apply_fd_fixups(struct binder_proc *proc,
> "failed fd fixup txn %d fd %d\n",
> t->debug_id, fd);
> ret = -ENOMEM;
> - break;
> + goto err;
> }
> binder_debug(BINDER_DEBUG_TRANSACTION,
> "fd fixup txn %d fd %d\n",
> t->debug_id, fd);
> trace_binder_transaction_fd_recv(t, fd, fixup->offset);
> - fd_install(fd, fixup->file);
> - fixup->file = NULL;
> + fixup->target_fd = fd;
> if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
> fixup->offset, &fd,
> sizeof(u32))) {
> ret = -EINVAL;
> - break;
> + goto err;
> }
> }
> list_for_each_entry_safe(fixup, tmp, &t->fd_fixups, fixup_entry) {
> - if (fixup->file) {
> - fput(fixup->file);
> - } else if (ret) {
> - u32 fd;
> - int err;
> -
> - err = binder_alloc_copy_from_buffer(&proc->alloc, &fd,
> - t->buffer,
> - fixup->offset,
> - sizeof(fd));
> - WARN_ON(err);
> - if (!err)
> - binder_deferred_fd_close(fd);
> - }
> + fd_install(fixup->target_fd, fixup->file);
> list_del(&fixup->fixup_entry);
> kfree(fixup);
> }
>
> return ret;
> +
> +err:
> + binder_free_txn_fixups(t);
> + return ret;
> }
>
> static int binder_thread_read(struct binder_proc *proc,
> diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> index d6b6b8cb7346..cf70a104594d 100644
> --- a/drivers/android/binder_internal.h
> +++ b/drivers/android/binder_internal.h
> @@ -515,6 +515,7 @@ struct binder_thread {
> * @fixup_entry: list entry
> * @file: struct file to be associated with new fd
> * @offset: offset in buffer data to this fixup
> + * @target_fd: fd to use by the target to install @file
> *
> * List element for fd fixups in a transaction. Since file
> * descriptors need to be allocated in the context of the
> @@ -525,6 +526,7 @@ struct binder_txn_fd_fixup {
> struct list_head fixup_entry;
> struct file *file;
> size_t offset;
> + int target_fd;
> };
>
> struct binder_transaction {
> --
> 2.35.1.1021.g381101b075-goog
>
Powered by blists - more mailing lists