Re: [PATCH] ipc/shm: fix use-after-free of shm file via remap_file_pages()

["Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>]

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