Re: [PATCH v2] binder: fix use-after-free due to ksys_close() during fdget()

[Al Viro]

On Fri, Dec 14, 2018 at 12:38:15PM -0800, Todd Kjos wrote:
> 44d8047f1d8 ("binder: use standard functions to allocate fds")
> exposed a pre-existing issue in the binder driver.
> 
> fdget() is used in ksys_ioctl() as a performance optimization.
> One of the rules associated with fdget() is that ksys_close() must
> not be called between the fdget() and the fdput(). There is a case
> where this requirement is not met in the binder driver which results
> in the reference count dropping to 0 when the device is still in
> use. This can result in use-after-free or other issues.
> 
> If userpace has passed a file-descriptor for the binder driver using
> a BINDER_TYPE_FDA object, then kys_close() is called on it when
> handling a binder_ioctl(BC_FREE_BUFFER) command. This violates
> the assumptions for using fdget().
> 
> The problem is fixed by deferring the fd close using task_work_add()
> so ksys_close() is called after returning from binder_ioctl().
> 
> Fixes: 44d8047f1d87a ("binder: use standard functions to allocate fds")
> Suggested-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Todd Kjos <tkjos@xxxxxxxxxx>

Umm...  IMO you are making it more brittle than needed.  Descriptors
(as in, integers serving as indices in file descriptor tables) are
sensitive to a lot of things and generally you don't want to pass
them around, at least not without a lot more context than you do.

References to struct file are much more robust.  And frankly, ksys_close()
is best not touched - what you want is basically a version of __close_fd()
that would grab a reference to struct file before filp_close() and
passed that reference to you.  Then your delayed ksys_close() would turn
into (equally delayed) fput().

Something like
int rip_fd(int fd, struct file **res)
{
	struct files_struct *files = current->files;
        struct file *file;
        struct fdtable *fdt;

        spin_lock(&files->file_lock);
        fdt = files_fdtable(files);
        if (fd >= fdt->max_fds)
                goto out_unlock;
        file = fdt->fd[fd];
        if (!file)
                goto out_unlock;
        rcu_assign_pointer(fdt->fd[fd], NULL);
        __put_unused_fd(files, fd);
        spin_unlock(&files->file_lock);
	get_file(file);
	*res = file;
        return filp_close(file, files);

out_unlock:
        spin_unlock(&files->file_lock);
	*res = NULL;
	return -ENOENT;
}

used as
	error = rip_fd(fd, &file);
	/* we are committed to arranging fput(file) now, error or not */

Frankly, the main objection to exporting that would be to avoid encouraging
the "we'll just pass the number around" kind of braindamage.  In case of
binder you are locked into that braindamage by an atrocious userland API
design and warnings along the lines of "use that and you *will* have
to explain yourself; yes, we will be watching" might suffice to prevent
additional uses...