linux-kernel - Re: [PATCH] fsi:fsi-sbefifo: Fix possible concurrency use-after-free bugs in sbefifo_user

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:   Thu, 03 Jan 2019 14:27:56 +1100
From:   Benjamin Herrenschmidt <benh@...nel.crashing.org>
To:     David Howells <dhowells@...hat.com>,
        Jia-Ju Bai <baijiaju1990@...il.com>
Cc:     joel@....id.au, eajames@...ux.vnet.ibm.com, andrew@...id.au,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fsi:fsi-sbefifo: Fix possible concurrency
 use-after-free bugs in sbefifo_user_release

On Wed, 2019-01-02 at 09:34 +0000, David Howells wrote:
> Jia-Ju Bai <baijiaju1990@...il.com> wrote:
> 
> > +	mutex_lock(&user->file_lock);
> >  	sbefifo_release_command(user);
> >  	free_page((unsigned long)user->cmd_page);
> > +	mutex_unlock(&user->file_lock);
> 
> It shouldn't be necessary to do the free_page() call inside the locked
> section.

True. However, I didn't realize read/write could be concurrent with
release so we have another problem.

I assume when release is called, no new read/write can be issued, I am
correct ? So all we have to protect against is a read/write that has
started prior to release being called, right ?

In that case, what can happen is that release() wins the race on the
mutex, frees everything, then read or write starts using feed stuff.

This is nasty to fix because the mutex is in the user structure,
so even looking at the mutex is racy if release is called.

The right fix, would be, I think, for "user" (pointed to by file-
>private_data) to be protected by a kref. That doesn't close it
completely as the free in release() can still lead to the structure
becoming re-used before read/write tries to get the kref but after it
has NULL checked the private data.

So to make that solid, I would also RCU-defer the actual freeing and
use RCU around dereferencing file->private_data

Now, I yet have to see other chardevs do any of the above, do that mean
they are all hopelessly racy ?

Cheers,
Ben.