Skip to content

Commit c478f65

Browse files
ebiggersMichal Hocko
authored and
Michal Hocko
committed
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. Commit 1ac0b6d ("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); 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 torvalds#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 Link: http://lkml.kernel.org/r/[email protected] Reported-by: syzbot+d11f321e7f1923157eac80aa990b446596f46439@syzkaller.appspotmail.com Fixes: c8d78c1 ("mm: replace remap_file_pages() syscall with emulation") Signed-off-by: Eric Biggers <[email protected]> Cc: Davidlohr Bueso <[email protected]> Cc: Manfred Spraul <[email protected]> Cc: "Eric W . Biederman" <[email protected]> Cc: "Kirill A. Shutemov" <[email protected]> Cc: <[email protected]> Signed-off-by: Andrew Morton <[email protected]>
1 parent 240a0fa commit c478f65

File tree

1 file changed

+11
-3
lines changed

1 file changed

+11
-3
lines changed

ipc/shm.c

+11-3
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,12 @@ static int __shm_open(struct vm_area_struct *vma)
203203
if (IS_ERR(shp))
204204
return PTR_ERR(shp);
205205

206+
if (shp->shm_file != sfd->file) {
207+
/* ID was reused */
208+
shm_unlock(shp);
209+
return -EINVAL;
210+
}
211+
206212
shp->shm_atim = ktime_get_real_seconds();
207213
shp->shm_lprid = task_tgid_vnr(current);
208214
shp->shm_nattch++;
@@ -431,8 +437,9 @@ static int shm_mmap(struct file *file, struct vm_area_struct *vma)
431437
int ret;
432438

433439
/*
434-
* In case of remap_file_pages() emulation, the file can represent
435-
* removed IPC ID: propogate shm_lock() error to caller.
440+
* In case of remap_file_pages() emulation, the file can represent an
441+
* IPC ID that was removed, and possibly even reused by another shm
442+
* segment already. Propagate this case as an error to caller.
436443
*/
437444
ret = __shm_open(vma);
438445
if (ret)
@@ -456,6 +463,7 @@ static int shm_release(struct inode *ino, struct file *file)
456463
struct shm_file_data *sfd = shm_file_data(file);
457464

458465
put_ipc_ns(sfd->ns);
466+
fput(sfd->file);
459467
shm_file_data(file) = NULL;
460468
kfree(sfd);
461469
return 0;
@@ -1415,7 +1423,7 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg,
14151423
file->f_mapping = shp->shm_file->f_mapping;
14161424
sfd->id = shp->shm_perm.id;
14171425
sfd->ns = get_ipc_ns(ns);
1418-
sfd->file = shp->shm_file;
1426+
sfd->file = get_file(shp->shm_file);
14191427
sfd->vm_ops = NULL;
14201428

14211429
err = security_mmap_file(file, prot, flags);

0 commit comments

Comments
 (0)