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 10:15:10 +0300
From:   Cyrill Gorcunov <gorcunov@...il.com>
To:     Andrei Vagin <avagin@...tuozzo.com>, Jann Horn <jannh@...gle.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,
        xemul@...tuozzo.com,
        Michael Kerrisk-manpages <mtk.manpages@...il.com>,
        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

On Sat, May 13, 2017 at 09:55:14AM +0300, Cyrill Gorcunov wrote:
> On Fri, May 12, 2017 at 06:45:09PM -0700, Andrei Vagin wrote:
> > On Sat, May 13, 2017 at 01:53:40AM +0300, Cyrill Gorcunov wrote:
> > > On Sat, May 13, 2017 at 12:41:30AM +0200, Jann Horn wrote:
> > > > [resending as plaintext]
> > > > 
> > > > 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.
> > > 
> > > It work with unlocked data on purpose for speed sake. Moreover even
> > > if we grap a reference it is valid _only_ during comparision operation,
> > > next we drop ref and it can be easily freed by os. Thus it's up to
> > > a caller to keep references to files/task and other resources used.
> > 
> > Looks like we can take rcu_read_lock() to guarantee that these objects
> > will not be freed, and rcu_read_lock() should not affect perfomance too much.
> 
> Rather they should be get_file_rcu/fput. Still I'm not convinced we need it,
> but fine will update both: plain KCMP_FILE and KCMP_EPOLL_TFD since it won't
> hurt performance.

>From manpage we wrote:

       Note the kcmp() is not protected against false positives which may occur
       if tasks are running.  One should stop tasks by sending SIGSTOP (see  sig‐
       nal(7)) prior to inspection with this system call to obtain meaningful results.

So no, not going to uglify source code and add get/put files there.

	Cyrill

Powered by blists - more mailing lists