linux-kernel - Re: Files leak from nfsd in 4.7.1-rc1 (and more?)

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]

Message-Id: <1D1008D4-9A28-48B2-9BA7-4069C2998D17@linuxhacker.ru>
Date:	Tue, 7 Jun 2016 23:55:41 -0400
From:	Oleg Drokin <green@...uxhacker.ru>
To:	Jeff Layton <jlayton@...chiereds.net>
Cc:	"J. Bruce Fields" <bfields@...ldses.org>,
	linux-nfs@...r.kernel.org,
	"<linux-kernel@...r.kernel.org> Mailing List" 
	<linux-kernel@...r.kernel.org>
Subject: Re: Files leak from nfsd in 4.7.1-rc1 (and more?)


On Jun 7, 2016, at 10:22 PM, Oleg Drokin wrote:

> 
> On Jun 7, 2016, at 8:03 PM, Jeff Layton wrote:
> 
>>>> That said, this code is quite subtle. I'd need to look over it in more
>>>> detail before I offer up any fixes. I'd also appreciate it if anyone
>>>> else wants to sanity check my analysis there.
>>>> 
>> Yeah, I think you're right. It's fine since r/w opens have a distinct
>> slot, even though the refcounting just tracks the number of read and
>> write references. So yeah, the leak probably is in an error path
>> someplace, or maybe a race someplace.
> 
> So I noticed that set_access is always called locked, but clear_access is not,
> this does not sound right.
> 
> So I placed this strategic WARN_ON:
> @@ -3991,6 +4030,7 @@ static __be32 nfs4_get_vfs_file(struct svc_rqst *rqstp, struct nfs4_file *fp,
>                        goto out_put_access;
>                spin_lock(&fp->fi_lock);
>                if (!fp->fi_fds[oflag]) {
> +WARN_ON(!test_access(open->op_share_access, stp));
>                        fp->fi_fds[oflag] = filp;
>                        filp = NULL;
> 
> This is right in the place where nfsd set the access flag already, discovered
> that the file is not opened and went on to open it, yet some parallel thread
> came in and cleared the flag by the time we got the file opened.
> It did trigger (but there are 30 minutes left till test finish, so I don't
> know yet if this will correspond to the problem at hand yet, so below is speculation).

Duh, I looked for a warning, but did not cross reference, and it was not this one that
hit yet.

Though apparently I am hitting some of the "impossible" warnings, so you might want to
look into that anyway.

        status = nfsd4_process_open2(rqstp, resfh, open);
        WARN(status && open->op_created,
             "nfsd4_process_open2 failed to open newly-created file! status=%u\n",
             be32_to_cpu(status));

and

        filp = find_readable_file(fp);
        if (!filp) {
                /* We should always have a readable file here */
                WARN_ON_ONCE(1);
                locks_free_lock(fl);
                return -EBADF;
        }