Re: [PATCH 0/3] binder: Prevent untranslated sender data from being copied to target
On Tue, Nov 23, 2021 at 11:17:34AM -0800, Todd Kjos wrote: > Binder copies transactions directly from the sender buffer > to the target buffer and then fixes up BINDER_TYPE_PTR and > BINDER_TYPE_FDA objects. This means there is a brief time > when sender pointers and fds are visible to the target > process. > > This series reworks the the sender to target copy to > avoid leaking any untranslated sender data from being > visible in the target. > > Todd Kjos (3): > binder: defer copies of pre-patched txn data > binder: read pre-translated fds from sender buffer > binder: avoid potential data leakage when copying txn > > drivers/android/binder.c | 442 > + > 1 file changed, 387 insertions(+), 55 deletions(-) Are these changes needed now in 5.16-final and also in stable kernels? Or can they wait until 5.17-rc1? thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: fix test regression due to sender_euid change
On Fri, Nov 19, 2021 at 03:39:59PM -0800, Todd Kjos wrote:
> On Fri, Nov 19, 2021 at 3:00 PM Paul Moore wrote:
> >
> > On Fri, Nov 12, 2021 at 1:07 PM Todd Kjos wrote:
> > >
> > > This is a partial revert of commit
> > > 29bc22ac5e5b ("binder: use euid from cred instead of using task").
> > > Setting sender_euid using proc->cred caused some Android system test
> > > regressions that need further investigation. It is a partial
> > > reversion because subsequent patches rely on proc->cred.
> > >
> > > Cc: sta...@vger.kernel.org # 4.4+
> > > Fixes: 29bc22ac5e5b ("binder: use euid from cred instead of using task")
> > > Signed-off-by: Todd Kjos
> > > Change-Id: I9b1769a3510fed250bb21859ef8beebabe034c66
>
> Greg, I neglected to remove the "Change-Id" from my Android pre-submit
> testing. Can you remove that, or would you like me to resubmit without
> it?
Sorry, I missed it too. It's already in my tree, no need to worry about
it.
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/3] binder: defer copies of pre-patched txn data
On Tue, Nov 23, 2021 at 11:17:37AM -0800, Todd Kjos wrote:
> +/**
> + * binder_do_deferred_txn_copies() - copy and fixup scatter-gather data
> + * @alloc: binder_alloc associated with @buffer
> + * @buffer: binder buffer in target process
> + * @sgc_head:list_head of scatter-gather copy list
> + * @pf_head: list_head of pointer fixup list
> + *
> + * Processes all elements of @sgc_head, applying fixups from @pf_head
> + * and copying the scatter-gather data from the source process' user
> + * buffer to the target's buffer. It is expected that the list creation
> + * and processing all occurs during binder_transaction() so these lists
> + * are only accessed in local context.
> + *
> + * Return: 0=success, else -errno
^^^
This function is supposed to return negatives on error.
> + */
> +static int binder_do_deferred_txn_copies(struct binder_alloc *alloc,
> + struct binder_buffer *buffer,
> + struct list_head *sgc_head,
> + struct list_head *pf_head)
> +{
> + int ret = 0;
> + struct list_head *entry, *tmp;
> + struct binder_ptr_fixup *pf =
> + list_first_entry_or_null(pf_head, struct binder_ptr_fixup,
> + node);
> +
> + list_for_each_safe(entry, tmp, sgc_head) {
> + size_t bytes_copied = 0;
> + struct binder_sg_copy *sgc =
> + container_of(entry, struct binder_sg_copy, node);
> +
> + while (bytes_copied < sgc->length) {
> + size_t copy_size;
> + size_t bytes_left = sgc->length - bytes_copied;
> + size_t offset = sgc->offset + bytes_copied;
> +
> + /*
> + * We copy up to the fixup (pointed to by pf)
> + */
> + copy_size = pf ? min(bytes_left, (size_t)pf->offset -
> offset)
> +: bytes_left;
> + if (!ret && copy_size)
> + ret = binder_alloc_copy_user_to_buffer(
^^
"ret" is the number of bytes remaining to be copied.
> + alloc, buffer,
> + offset,
> + sgc->uaddr + bytes_copied,
> + copy_size);
> + bytes_copied += copy_size;
> + if (copy_size != bytes_left) {
> + BUG_ON(!pf);
> + /* we stopped at a fixup offset */
> + if (pf->skip_size) {
> + /*
> + * we are just skipping. This is for
> + * BINDER_TYPE_FDA where the translated
> + * fds will be fixed up when we get
> + * to target context.
> + */
> + bytes_copied += pf->skip_size;
> + } else {
> + /* apply the fixup indicated by pf */
> + if (!ret)
> + ret =
> binder_alloc_copy_to_buffer(
> + alloc, buffer,
> + pf->offset,
> + &pf->fixup_data,
> + sizeof(pf->fixup_data));
> + bytes_copied += sizeof(pf->fixup_data);
> + }
> + list_del(&pf->node);
> + kfree(pf);
> + pf = list_first_entry_or_null(pf_head,
> + struct binder_ptr_fixup, node);
> + }
> + }
> + list_del(&sgc->node);
> + kfree(sgc);
> + }
> + BUG_ON(!list_empty(pf_head));
> + BUG_ON(!list_empty(sgc_head));
> +
> + return ret;
> +}
> +
> +/**
> + * binder_cleanup_deferred_txn_lists() - free specified lists
> + * @sgc_head:list_head of scatter-gather copy list
> + * @pf_head: list_head of pointer fixup list
> + *
> + * Called to clean up @sgc_head and @pf_head if there is an
> + * error.
> + */
> +static void binder_cleanup_deferred_txn_lists(struct list_head *sgc_head,
> + struct list_head *pf_head)
> +{
> + struct list_head *entry, *tmp;
> +
> + list_for_each_safe(entry, tmp, sgc_head) {
> + struct binder_sg_copy *sgc =
>
Re: [PATCH 2/3] binder: read pre-translated fds from sender buffer
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.
> Signed-off-by: Todd Kjos
> ---
> 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.
>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. It feels
wrong. Does that not lead to a bug in the target app?
> @@ -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, thr
Re: [PATCH 3/3] binder: defer copies of pre-patched txn data
On Tue, Nov 23, 2021 at 11:17:37AM -0800, Todd Kjos wrote:
> +static int binder_do_deferred_txn_copies(struct binder_alloc *alloc,
> + struct binder_buffer *buffer,
> + struct list_head *sgc_head,
> + struct list_head *pf_head)
> +{
> + int ret = 0;
> + struct list_head *entry, *tmp;
> + struct binder_ptr_fixup *pf =
> + list_first_entry_or_null(pf_head, struct binder_ptr_fixup,
> + node);
> +
> + list_for_each_safe(entry, tmp, sgc_head) {
^^^
All the list_for_each() loops can be changed to list_for_each_entry().
list_for_each_entry_safe(sgc, tmp, sgc_head, node) {
regards,
dan carpenter
> + size_t bytes_copied = 0;
> + struct binder_sg_copy *sgc =
> + container_of(entry, struct binder_sg_copy, node);
> +
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/3] binder: avoid potential data leakage when copying txn
On Tue, Nov 23, 2021 at 11:17:35AM -0800, Todd Kjos wrote:
> Transactions are copied from the sender to the target
> first and objects like BINDER_TYPE_PTR and BINDER_TYPE_FDA
> are then fixed up. This means there is a short period where
> the sender's version of these objects are visible to the
> target prior to the fixups.
>
> Instead of copying all of the data first, copy data only
> after any needed fixups have been applied.
>
This patch needs a fixes tag.
So this patch basically fixes the simple part of the info leak and
patch 3 fixes the complicated part. Have I understood that correctly?
> @@ -2956,7 +2984,11 @@ static void binder_transaction(struct binder_proc
> *proc,
> }
> ret = binder_translate_fd_array(fda, parent, t, thread,
> in_reply_to);
> - if (ret < 0) {
> + if (ret < 0 ||
> + binder_alloc_copy_to_buffer(&target_proc->alloc,
> + t->buffer,
> + object_offset,
> + fda, sizeof(*fda))) {
> return_error = BR_FAILED_REPLY;
> return_error_param = ret;
"ret" is not a negative error code if binder_translate_fd_array()
succeeds but binder_alloc_copy_to_buffer() fails.
> return_error_line = __LINE__;
> @@ -3028,6 +3060,19 @@ static void binder_transaction(struct binder_proc
> *proc,
> goto err_bad_object_type;
> }
> }
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/3] binder: avoid potential data leakage when copying txn
Hi Todd, I love your patch! Perhaps something to improve: [auto build test WARNING on staging/staging-testing] [also build test WARNING on v5.16-rc2 next-20211124] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Todd-Kjos/binder-Prevent-untranslated-sender-data-from-being-copied-to-target/20211124-031908 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 1189d2fb15a4b09b2e8dd01d60a0817d985d933d config: nds32-buildonly-randconfig-r004-20211123 (https://download.01.org/0day-ci/archive/20211125/202111251005.zqfhg0pb-...@intel.com/config) compiler: nds32le-linux-gcc (GCC) 11.2.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/d51c5e7a3791e9e748200416f85456c826d3030e git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Todd-Kjos/binder-Prevent-untranslated-sender-data-from-being-copied-to-target/20211124-031908 git checkout d51c5e7a3791e9e748200416f85456c826d3030e # save the config file to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=nds32 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All warnings (new ones prefixed by >>): In file included from include/asm-generic/bug.h:22, from ./arch/nds32/include/generated/asm/bug.h:1, from include/linux/bug.h:5, from include/linux/thread_info.h:13, from include/asm-generic/preempt.h:5, from ./arch/nds32/include/generated/asm/preempt.h:1, from include/linux/preempt.h:78, from include/linux/spinlock.h:55, from include/linux/fdtable.h:11, from drivers/android/binder.c:45: drivers/android/binder.c: In function 'binder_transaction': >> drivers/android/binder.c:2711:30: warning: cast from pointer to integer of >> different size [-Wpointer-to-int-cast] 2711 | (u64)user_buffer, | ^ include/linux/printk.h:418:33: note: in definition of macro 'printk_index_wrap' 418 | _p_func(_fmt, ##__VA_ARGS__); \ | ^~~ include/linux/printk.h:640:17: note: in expansion of macro 'printk' 640 | printk(fmt, ##__VA_ARGS__); \ | ^~ include/linux/printk.h:660:9: note: in expansion of macro 'printk_ratelimited' 660 | printk_ratelimited(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__) | ^~ drivers/android/binder.c:139:25: note: in expansion of macro 'pr_info_ratelimited' 139 | pr_info_ratelimited(x); \ | ^~~ drivers/android/binder.c:2707:17: note: in expansion of macro 'binder_debug' 2707 | binder_debug(BINDER_DEBUG_TRANSACTION, | ^~~~ drivers/android/binder.c:2720:30: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] 2720 | (u64)user_buffer, | ^ include/linux/printk.h:418:33: note: in definition of macro 'printk_index_wrap' 418 | _p_func(_fmt, ##__VA_ARGS__); \ | ^~~ include/linux/printk.h:640:17: note: in expansion of macro 'printk' 640 | printk(fmt, ##__VA_ARGS__); \ | ^~ include/linux/printk.h:660:9: note: in expansion of macro 'printk_ratelimited' 660 | printk_ratelimited(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__) | ^~ drivers/android/binder.c:139:25: note: in expansion of macro 'pr_info_ratelimited' 139 | pr_info_ratelimited(x); \ | ^~~ drivers/android/binder.c:2716:17: note: in expansion of macro 'binder_debug' 2716 | binder_debug(BINDER_DEBUG_TRANSACTION, | ^~~~ vim +2711 drivers/android/binder.c 2457 2458 static void binder_transaction(struct binder_proc *proc, 2459
[driver-core:driver-core-testing] BUILD SUCCESS 393c3714081a53795bbff0e985d24146def6f57f
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git driver-core-testing branch HEAD: 393c3714081a53795bbff0e985d24146def6f57f kernfs: switch global kernfs_rwsem lock to per-fs lock elapsed time: 781m configs tested: 151 configs skipped: 3 The following configs have been built successfully. More configs may be tested in the coming days. gcc tested configs: arm defconfig arm64allyesconfig arm64 defconfig arm allyesconfig arm allmodconfig i386 randconfig-c001-20211124 i386 randconfig-c001-20211125 m68k hp300_defconfig mips pic32mzda_defconfig arm cns3420vb_defconfig m68k sun3x_defconfig arm simpad_defconfig sparc defconfig armqcom_defconfig m68k m5275evb_defconfig powerpc currituck_defconfig ia64 alldefconfig sh se7721_defconfig armspear3xx_defconfig s390 zfcpdump_defconfig arm colibri_pxa300_defconfig mips bigsur_defconfig sh espt_defconfig um defconfig sh lboxre2_defconfig powerpc tqm8541_defconfig powerpcsocrates_defconfig armrealview_defconfig powerpc chrp32_defconfig mips gcw0_defconfig xtensa virt_defconfig powerpc mpc8315_rdb_defconfig mips cavium_octeon_defconfig mips bmips_stb_defconfig armmmp2_defconfig powerpc mgcoge_defconfig powerpc ebony_defconfig mips malta_kvm_defconfig mipsqi_lb60_defconfig armmagician_defconfig armmulti_v7_defconfig powerpc mpc834x_itxgp_defconfig nios2allyesconfig sh sdk7786_defconfig mips tb0226_defconfig mips ip22_defconfig sh rsk7264_defconfig powerpc64 defconfig shshmin_defconfig arm lpd270_defconfig arm collie_defconfig x86_64 allyesconfig sh se7724_defconfig mips loongson3_defconfig armmps2_defconfig arm jornada720_defconfig arm omap1_defconfig mipsmaltaup_xpa_defconfig shdreamcast_defconfig arm socfpga_defconfig mips maltasmvp_eva_defconfig sparc64 defconfig powerpc canyonlands_defconfig sh kfr2r09-romimage_defconfig m68kmac_defconfig powerpc mpc83xx_defconfig nios2 defconfig m68km5307c3_defconfig mips loongson1b_defconfig powerpc microwatt_defconfig m68kstmark2_defconfig arm randconfig-c002-20211124 arm randconfig-c002-20211125 ia64 allmodconfig ia64defconfig ia64 allyesconfig m68k allmodconfig m68kdefconfig m68k allyesconfig arc allyesconfig nds32 allnoconfig nds32 defconfig cskydefconfig alpha defconfig alphaallyesconfig xtensa allyesconfig h8300allyesconfig arc defconfig sh allmodconfig parisc defconfig s390 allyesconfig s390 allmodconfig parisc allyesconfig s390defconfig i386 allyesconfig sparcallyesconfig i386defconfig i386 debian-10.3 mips allyesconfig mips allmodconfig
Re: [PATCH 2/3] binder: read pre-translated fds from sender buffer
On Wed, Nov 24, 2021 at 12:33:20PM -0800, Todd Kjos wrote: > 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 should really be a separate patch at the start of the series because it's from the original code and unrelated. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
