|
Michal Hocko |
a10fae |
From 3f05317d9889ab75c7190dcd39491d2a97921984 Mon Sep 17 00:00:00 2001
|
|
Michal Hocko |
a10fae |
From: Eric Biggers <ebiggers@google.com>
|
|
Michal Hocko |
a10fae |
Date: Fri, 13 Apr 2018 15:35:30 -0700
|
|
Michal Hocko |
a10fae |
Subject: [PATCH] ipc/shm: fix use-after-free of shm file via
|
|
Michal Hocko |
a10fae |
remap_file_pages()
|
|
Michal Hocko |
a10fae |
Git-commit: 3f05317d9889ab75c7190dcd39491d2a97921984
|
|
Benjamin Poirier |
3525f6 |
Patch-mainline: v4.17-rc1
|
|
Michal Hocko |
a10fae |
References: bnc#1102512
|
|
Michal Hocko |
a10fae |
|
|
Michal Hocko |
a10fae |
syzbot reported a use-after-free of shm_file_data(file)->file->f_op in
|
|
Michal Hocko |
a10fae |
shm_get_unmapped_area(), called via sys_remap_file_pages().
|
|
Michal Hocko |
a10fae |
|
|
Michal Hocko |
a10fae |
Unfortunately it couldn't generate a reproducer, but I found a bug which
|
|
Michal Hocko |
a10fae |
I think caused it. When remap_file_pages() is passed a full System V
|
|
Michal Hocko |
a10fae |
shared memory segment, the memory is first unmapped, then a new map is
|
|
Michal Hocko |
a10fae |
created using the ->vm_file. Between these steps, the shm ID can be
|
|
Michal Hocko |
a10fae |
removed and reused for a new shm segment. But, shm_mmap() only checks
|
|
Michal Hocko |
a10fae |
whether the ID is currently valid before calling the underlying file's
|
|
Michal Hocko |
a10fae |
->mmap(); it doesn't check whether it was reused. Thus it can use the
|
|
Michal Hocko |
a10fae |
wrong underlying file, one that was already freed.
|
|
Michal Hocko |
a10fae |
|
|
Michal Hocko |
a10fae |
Fix this by making the "outer" shm file (the one that gets put in
|
|
Michal Hocko |
a10fae |
->vm_file) hold a reference to the real shm file, and by making
|
|
Michal Hocko |
a10fae |
__shm_open() require that the file associated with the shm ID matches
|
|
Michal Hocko |
a10fae |
the one associated with the "outer" file.
|
|
Michal Hocko |
a10fae |
|
|
Michal Hocko |
a10fae |
Taking the reference to the real shm file is needed to fully solve the
|
|
Michal Hocko |
a10fae |
problem, since otherwise sfd->file could point to a freed file, which
|
|
Michal Hocko |
a10fae |
then could be reallocated for the reused shm ID, causing the wrong shm
|
|
Michal Hocko |
a10fae |
segment to be mapped (and without the required permission checks).
|
|
Michal Hocko |
a10fae |
|
|
Michal Hocko |
a10fae |
Commit 1ac0b6dec656 ("ipc/shm: handle removed segments gracefully in
|
|
Michal Hocko |
a10fae |
shm_mmap()") almost fixed this bug, but it didn't go far enough because
|
|
Michal Hocko |
a10fae |
it didn't consider the case where the shm ID is reused.
|
|
Michal Hocko |
a10fae |
|
|
Michal Hocko |
a10fae |
The following program usually reproduces this bug:
|
|
Michal Hocko |
a10fae |
|
|
Michal Hocko |
a10fae |
#include <stdlib.h>
|
|
Michal Hocko |
a10fae |
#include <sys/shm.h>
|
|
Michal Hocko |
a10fae |
#include <sys/syscall.h>
|
|
Michal Hocko |
a10fae |
#include <unistd.h>
|
|
Michal Hocko |
a10fae |
|
|
Michal Hocko |
a10fae |
int main()
|
|
Michal Hocko |
a10fae |
{
|
|
Michal Hocko |
a10fae |
int is_parent = (fork() != 0);
|
|
Michal Hocko |
a10fae |
srand(getpid());
|
|
Michal Hocko |
a10fae |
for (;;) {
|
|
Michal Hocko |
a10fae |
int id = shmget(0xF00F, 4096, IPC_CREAT|0700);
|
|
Michal Hocko |
a10fae |
if (is_parent) {
|
|
Michal Hocko |
a10fae |
void *addr = shmat(id, NULL, 0);
|
|
Michal Hocko |
a10fae |
usleep(rand() % 50);
|
|
Michal Hocko |
a10fae |
while (!syscall(__NR_remap_file_pages, addr, 4096, 0, 0, 0));
|
|
Michal Hocko |
a10fae |
} else {
|
|
Michal Hocko |
a10fae |
usleep(rand() % 50);
|
|
Michal Hocko |
a10fae |
shmctl(id, IPC_RMID, NULL);
|
|
Michal Hocko |
a10fae |
}
|
|
Michal Hocko |
a10fae |
}
|
|
Michal Hocko |
a10fae |
}
|
|
Michal Hocko |
a10fae |
|
|
Michal Hocko |
a10fae |
It causes the following NULL pointer dereference due to a 'struct file'
|
|
Michal Hocko |
a10fae |
being used while it's being freed. (I couldn't actually get a KASAN
|
|
Michal Hocko |
a10fae |
use-after-free splat like in the syzbot report. But I think it's
|
|
Michal Hocko |
a10fae |
possible with this bug; it would just take a more extraordinary race...)
|
|
Michal Hocko |
a10fae |
|
|
Michal Hocko |
a10fae |
BUG: unable to handle kernel NULL pointer dereference at 0000000000000058
|
|
Michal Hocko |
a10fae |
PGD 0 P4D 0
|
|
Michal Hocko |
a10fae |
Oops: 0000 [#1] SMP NOPTI
|
|
Michal Hocko |
a10fae |
CPU: 9 PID: 258 Comm: syz_ipc Not tainted 4.16.0-05140-gf8cf2f16a7c95 #189
|
|
Michal Hocko |
a10fae |
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-20171110_100015-anatol 04/01/2014
|
|
Michal Hocko |
a10fae |
RIP: 0010:d_inode include/linux/dcache.h:519 [inline]
|
|
Michal Hocko |
a10fae |
RIP: 0010:touch_atime+0x25/0xd0 fs/inode.c:1724
|
|
Michal Hocko |
a10fae |
[...]
|
|
Michal Hocko |
a10fae |
Call Trace:
|
|
Michal Hocko |
a10fae |
file_accessed include/linux/fs.h:2063 [inline]
|
|
Michal Hocko |
a10fae |
shmem_mmap+0x25/0x40 mm/shmem.c:2149
|
|
Michal Hocko |
a10fae |
call_mmap include/linux/fs.h:1789 [inline]
|
|
Michal Hocko |
a10fae |
shm_mmap+0x34/0x80 ipc/shm.c:465
|
|
Michal Hocko |
a10fae |
call_mmap include/linux/fs.h:1789 [inline]
|
|
Michal Hocko |
a10fae |
mmap_region+0x309/0x5b0 mm/mmap.c:1712
|
|
Michal Hocko |
a10fae |
do_mmap+0x294/0x4a0 mm/mmap.c:1483
|
|
Michal Hocko |
a10fae |
do_mmap_pgoff include/linux/mm.h:2235 [inline]
|
|
Michal Hocko |
a10fae |
SYSC_remap_file_pages mm/mmap.c:2853 [inline]
|
|
Michal Hocko |
a10fae |
SyS_remap_file_pages+0x232/0x310 mm/mmap.c:2769
|
|
Michal Hocko |
a10fae |
do_syscall_64+0x64/0x1a0 arch/x86/entry/common.c:287
|
|
Michal Hocko |
a10fae |
entry_SYSCALL_64_after_hwframe+0x42/0xb7
|
|
Michal Hocko |
a10fae |
|
|
Michal Hocko |
a10fae |
[ebiggers@google.com: add comment]
|
|
Michal Hocko |
a10fae |
Link: http://lkml.kernel.org/r/20180410192850.235835-1-ebiggers3@gmail.com
|
|
Michal Hocko |
a10fae |
Link: http://lkml.kernel.org/r/20180409043039.28915-1-ebiggers3@gmail.com
|
|
Michal Hocko |
a10fae |
Reported-by: syzbot+d11f321e7f1923157eac80aa990b446596f46439@syzkaller.appspotmail.com
|
|
Michal Hocko |
a10fae |
Fixes: c8d78c1823f4 ("mm: replace remap_file_pages() syscall with emulation")
|
|
Michal Hocko |
a10fae |
Signed-off-by: Eric Biggers <ebiggers@google.com>
|
|
Michal Hocko |
a10fae |
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
|
|
Michal Hocko |
a10fae |
Acked-by: Davidlohr Bueso <dbueso@suse.de>
|
|
Michal Hocko |
a10fae |
Cc: Manfred Spraul <manfred@colorfullife.com>
|
|
Michal Hocko |
a10fae |
Cc: "Eric W . Biederman" <ebiederm@xmission.com>
|
|
Michal Hocko |
a10fae |
Cc: <stable@vger.kernel.org>
|
|
Michal Hocko |
a10fae |
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
|
|
Michal Hocko |
a10fae |
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
|
|
Michal Hocko |
a10fae |
Signed-off-by: Michal Hocko <mhocko@suse.cz>
|
|
Michal Hocko |
a10fae |
|
|
Michal Hocko |
a10fae |
---
|
|
Michal Hocko |
a10fae |
ipc/shm.c | 23 ++++++++++++++++++++---
|
|
Michal Hocko |
a10fae |
1 file changed, 20 insertions(+), 3 deletions(-)
|
|
Michal Hocko |
a10fae |
|
|
Michal Hocko |
a10fae |
--- a/ipc/shm.c
|
|
Michal Hocko |
a10fae |
+++ b/ipc/shm.c
|
|
Michal Hocko |
a10fae |
@@ -200,6 +200,12 @@ static int __shm_open(struct vm_area_str
|
|
Michal Hocko |
a10fae |
if (IS_ERR(shp))
|
|
Michal Hocko |
a10fae |
return PTR_ERR(shp);
|
|
Michal Hocko |
a10fae |
|
|
Michal Hocko |
a10fae |
+ if (shp->shm_file != sfd->file) {
|
|
Michal Hocko |
a10fae |
+ /* ID was reused */
|
|
Michal Hocko |
a10fae |
+ shm_unlock(shp);
|
|
Michal Hocko |
a10fae |
+ return -EINVAL;
|
|
Michal Hocko |
a10fae |
+ }
|
|
Michal Hocko |
a10fae |
+
|
|
Michal Hocko |
a10fae |
shp->shm_atim = get_seconds();
|
|
Michal Hocko |
a10fae |
ipc_update_pid(&shp->shm_lprid, task_tgid(current));
|
|
Michal Hocko |
a10fae |
shp->shm_nattch++;
|
|
Michal Hocko |
a10fae |
@@ -430,8 +436,9 @@ static int shm_mmap(struct file *file, s
|
|
Michal Hocko |
a10fae |
int ret;
|
|
Michal Hocko |
a10fae |
|
|
Michal Hocko |
a10fae |
/*
|
|
Michal Hocko |
a10fae |
- * In case of remap_file_pages() emulation, the file can represent
|
|
Michal Hocko |
a10fae |
- * removed IPC ID: propogate shm_lock() error to caller.
|
|
Michal Hocko |
a10fae |
+ * In case of remap_file_pages() emulation, the file can represent an
|
|
Michal Hocko |
a10fae |
+ * IPC ID that was removed, and possibly even reused by another shm
|
|
Michal Hocko |
a10fae |
+ * segment already. Propagate this case as an error to caller.
|
|
Michal Hocko |
a10fae |
*/
|
|
Michal Hocko |
a10fae |
ret = __shm_open(vma);
|
|
Michal Hocko |
a10fae |
if (ret)
|
|
Michal Hocko |
a10fae |
@@ -455,6 +462,7 @@ static int shm_release(struct inode *ino
|
|
Michal Hocko |
a10fae |
struct shm_file_data *sfd = shm_file_data(file);
|
|
Michal Hocko |
a10fae |
|
|
Michal Hocko |
a10fae |
put_ipc_ns(sfd->ns);
|
|
Michal Hocko |
a10fae |
+ fput(sfd->file);
|
|
Michal Hocko |
a10fae |
shm_file_data(file) = NULL;
|
|
Michal Hocko |
a10fae |
kfree(sfd);
|
|
Michal Hocko |
a10fae |
return 0;
|
|
Michal Hocko |
a10fae |
@@ -1219,7 +1227,16 @@ long do_shmat(int shmid, char __user *sh
|
|
Michal Hocko |
a10fae |
file->f_mapping = shp->shm_file->f_mapping;
|
|
Michal Hocko |
a10fae |
sfd->id = shp->shm_perm.id;
|
|
Michal Hocko |
a10fae |
sfd->ns = get_ipc_ns(ns);
|
|
Michal Hocko |
a10fae |
- sfd->file = shp->shm_file;
|
|
Michal Hocko |
a10fae |
+ /*
|
|
Michal Hocko |
a10fae |
+ * We need to take a reference to the real shm file to prevent the
|
|
Michal Hocko |
a10fae |
+ * pointer from becoming stale in cases where the lifetime of the outer
|
|
Michal Hocko |
a10fae |
+ * file extends beyond that of the shm segment. It's not usually
|
|
Michal Hocko |
a10fae |
+ * possible, but it can happen during remap_file_pages() emulation as
|
|
Michal Hocko |
a10fae |
+ * that unmaps the memory, then does ->mmap() via file reference only.
|
|
Michal Hocko |
a10fae |
+ * We'll deny the ->mmap() if the shm segment was since removed, but to
|
|
Michal Hocko |
a10fae |
+ * detect shm ID reuse we need to compare the file pointers.
|
|
Michal Hocko |
a10fae |
+ */
|
|
Michal Hocko |
a10fae |
+ sfd->file = get_file(shp->shm_file);
|
|
Michal Hocko |
a10fae |
sfd->vm_ops = NULL;
|
|
Michal Hocko |
a10fae |
|
|
Michal Hocko |
a10fae |
err = security_mmap_file(file, prot, flags);
|