lkml.org 
[lkml]   [2018]   [Apr]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] ipc/shm: fix use-after-free of shm file via remap_file_pages()
    On Mon, Apr 09, 2018 at 04:30:39AM +0000, Eric Biggers wrote:
    > From: Eric Biggers <ebiggers@google.com>
    >
    > 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.
    >
    > 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.

    Right. Thanks for catching this.

    > 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);
    > srand(getpid());
    > 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
    > entry_SYSCALL_64_after_hwframe+0x42/0xb7
    >
    > Reported-by: syzbot+d11f321e7f1923157eac80aa990b446596f46439@syzkaller.appspotmail.com
    > Fixes: c8d78c1823f4 ("mm: replace remap_file_pages() syscall with emulation")
    > Cc: stable@vger.kernel.org
    > Signed-off-by: Eric Biggers <ebiggers@google.com>
    > ---
    > ipc/shm.c | 14 +++++++++++---
    > 1 file changed, 11 insertions(+), 3 deletions(-)
    >
    > diff --git a/ipc/shm.c b/ipc/shm.c
    > index acefe44fefefa..c80c5691a9970 100644
    > --- a/ipc/shm.c
    > +++ b/ipc/shm.c
    > @@ -225,6 +225,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 = ktime_get_real_seconds();
    > ipc_update_pid(&shp->shm_lprid, task_tgid(current));
    > shp->shm_nattch++;
    > @@ -455,8 +461,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)
    > @@ -480,6 +487,7 @@ static int shm_release(struct inode *ino, struct file *file)
    > struct shm_file_data *sfd = shm_file_data(file);
    >
    > put_ipc_ns(sfd->ns);
    > + fput(sfd->file);
    > shm_file_data(file) = NULL;
    > kfree(sfd);
    > return 0;
    > @@ -1432,7 +1440,7 @@ 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;
    > + sfd->file = get_file(shp->shm_file);
    > sfd->vm_ops = NULL;
    >
    > err = security_mmap_file(file, prot, flags);

    Hm. Why do we need sfd->file refcounting now? It's not obvious to me.

    Looks like it's either a separate bug or an unneeded change.

    --
    Kirill A. Shutemov

    \
     
     \ /
      Last update: 2018-04-09 11:48    [W:3.988 / U:0.188 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site