linux-kernel - Re: [PATCH] ipc/shm: fix use-after-free of shm file via remap_file

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: <20180409203635.GD203367@gmail.com>
Date:   Mon, 9 Apr 2018 13:36:35 -0700
From:   Eric Biggers <ebiggers3@...il.com>
To:     Davidlohr Bueso <dave@...olabs.net>
Cc:     linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Manfred Spraul <manfred@...orfullife.com>,
        "Eric W . Biederman" <ebiederm@...ssion.com>,
        syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] ipc/shm: fix use-after-free of shm file via
 remap_file_pages()

On Mon, Apr 09, 2018 at 01:12:32PM -0700, Davidlohr Bueso wrote:
> On Mon, 09 Apr 2018, Eric Biggers wrote:
> 
> > It's necessary because if we don't hold a reference to sfd->file, then it can be
> > a stale pointer when we compare it in __shm_open().  In particular, if the new
> > struct file happened to be allocated at the same address as the old one, then
> > 'sfd->file == shp->shm_file' so the mmap would be allowed.  But, it will be a
> > different shm segment than was intended.  The caller may not even have
> > permissions to map it normally, yet it would be done anyway.
> > 
> > In the end it's just broken to have a pointer to something that can be freed out
> > from under you...
> 
> So this is actually handled by shm_nattch, serialized by the ipc perm->lock.
> shm_destroy() is called when 0, which in turn does the fput(shm_file). Note
> that shm_file is given a count of 1 when a new segment is created (deep in
> get_empty_filp()). So I don't think the pointer is going anywhere, or am I missing
> something?
> 
> Thanks,
> Davidlohr

In the remap_file_pages() case, a reference is taken to the ->vm_file, then the
segment is unmapped.  If that brings ->shm_nattch to 0, then the underlying shm
segment and ID can be removed, which (currently) causes the real shm file to be
freed.  But, the outer file still exists and will have ->mmap() called on it.
That's why the outer file needs to hold a reference to the real shm file.

Eric