[RHEL7,COMMIT] ms/ipc/shm: fix use-after-free of shm file via remap_file_pages()

Submitted by Konstantin Khorenko on Oct. 28, 2018, 7:40 a.m.


Message ID 201810280740.w9S7eQG6029754@finist-ce7.sw.ru
State New
Series "ms/ipc/shm: fix use-after-free of shm file via remap_file_pages()"
Headers show

Commit Message

Konstantin Khorenko Oct. 28, 2018, 7:40 a.m.
The commit is pushed to "branch-rh7-3.10.0-862.14.4.vz7.72.x-ovz" and will appear at https://src.openvz.org/scm/ovz/vzkernel.git
after rh7-3.10.0-862.14.4.vz7.72.14
commit 85f55043567282fd60f946571a9dd2f8af248bd2
Author: Eric Biggers <ebiggers@google.com>
Date:   Fri Apr 13 15:35:30 2018 -0700

    ms/ipc/shm: fix use-after-free of shm file via remap_file_pages()
    syzbot reported a use-after-free of shm_file_data(file)->file->f_op in
    shm_get_unmapped_area(), called via sys_remap_file_pages().
    Unfortunately it couldn't generate a reproducer, but I found a bug which
    I think caused it.  When remap_file_pages() is passed a full System V
    shared memory segment, the memory is first unmapped, then a new map is
    created using the ->vm_file.  Between these steps, the shm ID can be
    removed and reused for a new shm segment.  But, shm_mmap() only checks
    whether the ID is currently valid before calling the underlying file's
    ->mmap(); it doesn't check whether it was reused.  Thus it can use the
    wrong underlying file, one that was already freed.
    Fix this by making the "outer" shm file (the one that gets put in
    ->vm_file) hold a reference to the real shm file, and by making
    __shm_open() require that the file associated with the shm ID matches
    the one associated with the "outer" file.
    Taking the reference to the real shm file is needed to fully solve the
    problem, since otherwise sfd->file could point to a freed file, which
    then could be reallocated for the reused shm ID, causing the wrong shm
    segment to be mapped (and without the required permission checks).
    Commit 1ac0b6dec656 ("ipc/shm: handle removed segments gracefully in
    shm_mmap()") almost fixed this bug, but it didn't go far enough because
    it didn't consider the case where the shm ID is reused.
    The following program usually reproduces this bug:
            #include <stdlib.h>
            #include <sys/shm.h>
            #include <sys/syscall.h>
            #include <unistd.h>
            int main()
                    int is_parent = (fork() != 0);
                    for (;;) {
                            int id = shmget(0xF00F, 4096, IPC_CREAT|0700);
                            if (is_parent) {
                                    void *addr = shmat(id, NULL, 0);
                                    usleep(rand() % 50);
                                    while (!syscall(__NR_remap_file_pages, addr, 4096, 0, 0, 0));
                            } else {
                                    usleep(rand() % 50);
                                    shmctl(id, IPC_RMID, NULL);
    It causes the following NULL pointer dereference due to a 'struct file'
    being used while it's being freed.  (I couldn't actually get a KASAN
    use-after-free splat like in the syzbot report.  But I think it's
    possible with this bug; it would just take a more extraordinary race...)
            BUG: unable to handle kernel NULL pointer dereference at 0000000000000058
            PGD 0 P4D 0
            Oops: 0000 [#1] SMP NOPTI
            CPU: 9 PID: 258 Comm: syz_ipc Not tainted 4.16.0-05140-gf8cf2f16a7c95 #189
            Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-20171110_100015-anatol 04/01/2014
            RIP: 0010:d_inode include/linux/dcache.h:519 [inline]
            RIP: 0010:touch_atime+0x25/0xd0 fs/inode.c:1724
            Call Trace:
             file_accessed include/linux/fs.h:2063 [inline]
             shmem_mmap+0x25/0x40 mm/shmem.c:2149
             call_mmap include/linux/fs.h:1789 [inline]
             shm_mmap+0x34/0x80 ipc/shm.c:465
             call_mmap include/linux/fs.h:1789 [inline]
             mmap_region+0x309/0x5b0 mm/mmap.c:1712
             do_mmap+0x294/0x4a0 mm/mmap.c:1483
             do_mmap_pgoff include/linux/mm.h:2235 [inline]
             SYSC_remap_file_pages mm/mmap.c:2853 [inline]
             SyS_remap_file_pages+0x232/0x310 mm/mmap.c:2769
             do_syscall_64+0x64/0x1a0 arch/x86/entry/common.c:287
    [ebiggers@google.com: add comment]
      Link: http://lkml.kernel.org/r/20180410192850.235835-1-ebiggers3@gmail.com
    Link: http://lkml.kernel.org/r/20180409043039.28915-1-ebiggers3@gmail.com
    Reported-by: syzbot+d11f321e7f1923157eac80aa990b446596f46439@syzkaller.appspotmail.com
    Fixes: c8d78c1823f4 ("mm: replace remap_file_pages() syscall with emulation")
    Signed-off-by: Eric Biggers <ebiggers@google.com>
    Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
    Acked-by: Davidlohr Bueso <dbueso@suse.de>
    Cc: Manfred Spraul <manfred@colorfullife.com>
    Cc: "Eric W . Biederman" <ebiederm@xmission.com>
    Cc: <stable@vger.kernel.org>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
    (cherry picked from commit 3f05317d9889ab75c7190dcd39491d2a97921984)
    Signed-off-by: Konstantin Khorenko <khorenko@virtuozzo.com>
 ipc/shm.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/ipc/shm.c b/ipc/shm.c
index 6b7263cf2441..b4a8cef36817 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -196,6 +196,12 @@  static int __shm_open(struct vm_area_struct *vma)
 	if (IS_ERR(shp))
 		return PTR_ERR(shp);
+	if (shp->shm_file != sfd->file) {
+		/* ID was reused */
+		shm_unlock(shp);
+		return -EINVAL;
+	}
 	shp->shm_atim = get_seconds();
 	shp->shm_lprid = task_tgid_vnr(current);
@@ -421,8 +427,9 @@  static int shm_mmap(struct file * file, struct vm_area_struct * vma)
 	int ret;
-	 * In case of remap_file_pages() emulation, the file can represent
-	 * removed IPC ID: propogate shm_lock() error to caller.
+	 * In case of remap_file_pages() emulation, the file can represent an
+	 * IPC ID that was removed, and possibly even reused by another shm
+	 * segment already.  Propagate this case as an error to caller.
 	ret =__shm_open(vma);
 	if (ret)
@@ -446,6 +453,7 @@  static int shm_release(struct inode *ino, struct file *file)
 	struct shm_file_data *sfd = shm_file_data(file);
+	fput(sfd->file);
 	shm_file_data(file) = NULL;
 	return 0;
@@ -1201,7 +1209,16 @@  long do_shmat(int shmid, char __user *shmaddr, int shmflg,
 	file->f_mapping = shp->shm_file->f_mapping;
 	sfd->id = shp->shm_perm.id;
 	sfd->ns = get_ipc_ns(ns);
-	sfd->file = shp->shm_file;
+	/*
+	 * We need to take a reference to the real shm file to prevent the
+	 * pointer from becoming stale in cases where the lifetime of the outer
+	 * file extends beyond that of the shm segment.  It's not usually
+	 * possible, but it can happen during remap_file_pages() emulation as
+	 * that unmaps the memory, then does ->mmap() via file reference only.
+	 * We'll deny the ->mmap() if the shm segment was since removed, but to
+	 * detect shm ID reuse we need to compare the file pointers.
+	 */
+	sfd->file = get_file(shp->shm_file);
 	sfd->vm_ops = NULL;
 	err = security_mmap_file(file, prot, flags);