Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process

[Christian Brauner <christian.brauner@ubuntu.com>]

On Fri, Apr 16, 2021 at 03:35:59PM +0000, Al Viro wrote:
> On Fri, Apr 16, 2021 at 05:13:10PM +0200, Christian Brauner wrote:
> 
> > My point here was more that the _file_ has already been opened _before_
> > that call to io_uring_add_task_file(). But any potential non-trivial
> > side-effects of opening that file that you correctly pointed out in an
> > earlier mail has already happened by that time.
> 
> The file comes from io_uring_get_file(), the entire thing is within the
> io_ring_ctx constructor and the only side effect there is ->ring_sock
> creation.  And that stays until the io_ring_ctx is freed.  I'm _not_
> saying I like io_uring style in general, BTW - in particular,
> ->ring_sock->file handling is a kludge (as is too much of interation
> with AF_UNIX machinery there).  But from side effects POV we are fine
> there.
> 
> > Granted there are more
> > obvious examples, e.g. the binder stuff.
> > 
> > 		int fd = get_unused_fd_flags(O_CLOEXEC);
> > 
> > 		if (fd < 0) {
> > 			binder_debug(BINDER_DEBUG_TRANSACTION,
> > 				     "failed fd fixup txn %d fd %d\n",
> > 				     t->debug_id, fd);
> > 			ret = -ENOMEM;
> > 			break;
> > 		}
> > 		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;
> > 		if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
> > 						fixup->offset, &fd,
> > 						sizeof(u32))) {
> > 			ret = -EINVAL;
> > 			break;
> > 		}
> 
> ... and it's actually broken, since this
>         /* All copies must be 32-bit aligned and 32-bit size */
> 	if (!check_buffer(alloc, buffer, buffer_offset, bytes))
> 		return -EINVAL;
> in binder_alloc_copy_to_buffer() should've been done *before*
> fd_install().  If anything, it's an example of the situation when
> fd_receive() would be wrong...

They could probably refactor this but I'm not sure why they'd bother. If
they fail processing any of those files they end up aborting the
whole transaction.
(And the original code didn't check the error code btw.)