linux-kernel - Re: [patch v4 resend 2/2] kcmp: Add KCMP_EPOLL

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 13 May 2017 00:41:30 +0200
From:   Jann Horn <jannh@...gle.com>
To:     Cyrill Gorcunov <gorcunov@...il.com>
Cc:     linux-fsdevel@...r.kernel.org,
        kernel list <linux-kernel@...r.kernel.org>,
        Linux API <linux-api@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>, akpm@...uxfoundation.org,
        avagin@...tuozzo.com, xemul@...tuozzo.com,
        Michael Kerrisk-manpages <mtk.manpages@...il.com>,
        Cyrill Gorcunov <gorcunov@...nvz.org>, avagin@...nvz.org,
        jbaron@...mai.com, Andy Lutomirski <luto@...capital.net>
Subject: Re: [patch v4 resend 2/2] kcmp: Add KCMP_EPOLL_TFD mode to compare
 epoll target files

[resending as plaintext]

On Mon, Apr 24, 2017 at 5:39 PM, Cyrill Gorcunov <gorcunov@...il.com> wrote:
> With current epoll architecture target files are addressed
> with file_struct and file descriptor number, where the last
> is not unique. Moreover files can be transferred from another
> process via unix socket, added into queue and closed then
> so we won't find this descriptor in the task fdinfo list.
>
> Thus to checkpoint and restore such processes CRIU needs to
> find out where exactly the target file is present to add it into
> epoll queue. For this sake one can use kcmp call where
> some particular target file from the queue is compared with
> arbitrary file passed as an argument.
[...]
> +#ifdef CONFIG_EPOLL
> +static int kcmp_epoll_target(struct task_struct *task1,
> +                            struct task_struct *task2,
> +                            unsigned long idx1,
> +                            struct kcmp_epoll_slot __user *uslot)
> +{
> +       struct file *filp, *filp_epoll, *filp_tgt;
> +       struct kcmp_epoll_slot slot;
> +       struct files_struct *files;
> +
> +       if (copy_from_user(&slot, uslot, sizeof(slot)))
> +               return -EFAULT;
> +
> +       filp = get_file_raw_ptr(task1, idx1);
> +       if (!filp)
> +               return -EBADF;
> +
> +       files = get_files_struct(task2);
> +       if (!files)
> +               return -EBADF;
> +
> +       spin_lock(&files->file_lock);
> +       filp_epoll = fcheck_files(files, slot.efd);
> +       if (filp_epoll)
> +               get_file(filp_epoll);
> +       else
> +               filp_tgt = ERR_PTR(-EBADF);
> +       spin_unlock(&files->file_lock);
> +       put_files_struct(files);
> +
> +       if (filp_epoll) {
> +               filp_tgt = get_epoll_tfile_raw_ptr(filp_epoll, slot.tfd, slot.toff);
> +               fput(filp_epoll);
> +       } else
> +
> +       if (IS_ERR(filp_tgt))
> +               return PTR_ERR(filp_tgt);
> +
> +       return kcmp_ptr(filp, filp_tgt, KCMP_FILE);
> +}

I realize that the existing kcmp code has the same issue, but:

Why are you not taking a reference to filp or filp_tgt? This can end up
performing a comparison between a pointer to a freed struct file and a
pointer to a struct file that was allocated afterwards, right? So it can
return a false "is equal" result when the two files aren't actually the same
if one of the target tasks is running? This looks like it unnecessarily
exposes information about whether an allocation reuses the memory of
a previously freed allocation.