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

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

[Eric Biggers]

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.

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);
-- 
2.17.0

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

[Kirill A. Shutemov]

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

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

[Eric Biggers]

On Mon, Apr 09, 2018 at 12:48:14PM +0300, Kirill A. Shutemov wrote:
> On Mon, Apr 09, 2018 at 04:30:39AM +0000, Eric Biggers wrote:
> > 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.
> 

It's necessary because if we don't hold a reference to sfd->file, then it can be
a stale pointer when we compare it in __shm_open().  In particular, if the new
struct file happened to be allocated at the same address as the old one, then
'sfd->file == shp->shm_file' so the mmap would be allowed.  But, it will be a
different shm segment than was intended.  The caller may not even have
permissions to map it normally, yet it would be done anyway.

In the end it's just broken to have a pointer to something that can be freed out
from under you...

- Eric

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

[Davidlohr Bueso]

On Mon, 09 Apr 2018, Eric Biggers wrote:

>It's necessary because if we don't hold a reference to sfd->file, then it can be
>a stale pointer when we compare it in __shm_open().  In particular, if the new
>struct file happened to be allocated at the same address as the old one, then
>'sfd->file == shp->shm_file' so the mmap would be allowed.  But, it will be a
>different shm segment than was intended.  The caller may not even have
>permissions to map it normally, yet it would be done anyway.
>
>In the end it's just broken to have a pointer to something that can be freed out
>from under you...

So this is actually handled by shm_nattch, serialized by the ipc perm->lock.
shm_destroy() is called when 0, which in turn does the fput(shm_file). Note
that shm_file is given a count of 1 when a new segment is created (deep in
get_empty_filp()). So I don't think the pointer is going anywhere, or am I missing
something?

Thanks,
Davidlohr

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

[Davidlohr Bueso]

On Mon, 09 Apr 2018, Davidlohr Bueso wrote:
>So I don't think the pointer is going anywhere, or am I missing
>something?

Ah, yes, wrong pointer, this is sdf->file -- sorry for the noise.

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

[Eric Biggers]

On Mon, Apr 09, 2018 at 01:12:32PM -0700, Davidlohr Bueso wrote:
> On Mon, 09 Apr 2018, Eric Biggers wrote:
> 
> > It's necessary because if we don't hold a reference to sfd->file, then it can be
> > a stale pointer when we compare it in __shm_open().  In particular, if the new
> > struct file happened to be allocated at the same address as the old one, then
> > 'sfd->file == shp->shm_file' so the mmap would be allowed.  But, it will be a
> > different shm segment than was intended.  The caller may not even have
> > permissions to map it normally, yet it would be done anyway.
> > 
> > In the end it's just broken to have a pointer to something that can be freed out
> > from under you...
> 
> So this is actually handled by shm_nattch, serialized by the ipc perm->lock.
> shm_destroy() is called when 0, which in turn does the fput(shm_file). Note
> that shm_file is given a count of 1 when a new segment is created (deep in
> get_empty_filp()). So I don't think the pointer is going anywhere, or am I missing
> something?
> 
> Thanks,
> Davidlohr

In the remap_file_pages() case, a reference is taken to the ->vm_file, then the
segment is unmapped.  If that brings ->shm_nattch to 0, then the underlying shm
segment and ID can be removed, which (currently) causes the real shm file to be
freed.  But, the outer file still exists and will have ->mmap() called on it.
That's why the outer file needs to hold a reference to the real shm file.

Eric

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

[Kirill A. Shutemov]

On Mon, Apr 09, 2018 at 01:36:35PM -0700, Eric Biggers wrote:
> On Mon, Apr 09, 2018 at 01:12:32PM -0700, Davidlohr Bueso wrote:
> > On Mon, 09 Apr 2018, Eric Biggers wrote:
> > 
> > > It's necessary because if we don't hold a reference to sfd->file, then it can be
> > > a stale pointer when we compare it in __shm_open().  In particular, if the new
> > > struct file happened to be allocated at the same address as the old one, then
> > > 'sfd->file == shp->shm_file' so the mmap would be allowed.  But, it will be a
> > > different shm segment than was intended.  The caller may not even have
> > > permissions to map it normally, yet it would be done anyway.
> > > 
> > > In the end it's just broken to have a pointer to something that can be freed out
> > > from under you...
> > 
> > So this is actually handled by shm_nattch, serialized by the ipc perm->lock.
> > shm_destroy() is called when 0, which in turn does the fput(shm_file). Note
> > that shm_file is given a count of 1 when a new segment is created (deep in
> > get_empty_filp()). So I don't think the pointer is going anywhere, or am I missing
> > something?
> > 
> > Thanks,
> > Davidlohr
> 
> In the remap_file_pages() case, a reference is taken to the ->vm_file, then the
> segment is unmapped.  If that brings ->shm_nattch to 0, then the underlying shm
> segment and ID can be removed, which (currently) causes the real shm file to be
> freed.  But, the outer file still exists and will have ->mmap() called on it.
> That's why the outer file needs to hold a reference to the real shm file.

Okay, fair enough. Logic in SysV IPC implementation is often hard to follow.
Could you include the description in the commit message?

And feel free to use my

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
 Kirill A. Shutemov

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

[Eric Biggers]

On Tue, Apr 10, 2018 at 10:58:22AM +0300, Kirill A. Shutemov wrote:
> On Mon, Apr 09, 2018 at 01:36:35PM -0700, Eric Biggers wrote:
> > On Mon, Apr 09, 2018 at 01:12:32PM -0700, Davidlohr Bueso wrote:
> > > On Mon, 09 Apr 2018, Eric Biggers wrote:
> > > 
> > > > It's necessary because if we don't hold a reference to sfd->file, then it can be
> > > > a stale pointer when we compare it in __shm_open().  In particular, if the new
> > > > struct file happened to be allocated at the same address as the old one, then
> > > > 'sfd->file == shp->shm_file' so the mmap would be allowed.  But, it will be a
> > > > different shm segment than was intended.  The caller may not even have
> > > > permissions to map it normally, yet it would be done anyway.
> > > > 
> > > > In the end it's just broken to have a pointer to something that can be freed out
> > > > from under you...
> > > 
> > > So this is actually handled by shm_nattch, serialized by the ipc perm->lock.
> > > shm_destroy() is called when 0, which in turn does the fput(shm_file). Note
> > > that shm_file is given a count of 1 when a new segment is created (deep in
> > > get_empty_filp()). So I don't think the pointer is going anywhere, or am I missing
> > > something?
> > > 
> > > Thanks,
> > > Davidlohr
> > 
> > In the remap_file_pages() case, a reference is taken to the ->vm_file, then the
> > segment is unmapped.  If that brings ->shm_nattch to 0, then the underlying shm
> > segment and ID can be removed, which (currently) causes the real shm file to be
> > freed.  But, the outer file still exists and will have ->mmap() called on it.
> > That's why the outer file needs to hold a reference to the real shm file.
> 
> Okay, fair enough. Logic in SysV IPC implementation is often hard to follow.
> Could you include the description in the commit message?
> 
> And feel free to use my
> 
> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> 

I'll send v2 to update the commit message and add a comment.

Thanks,

Eric

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

[Eric Biggers]

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.  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);
		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 | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

v2: update commit message and add comment to explain why we need to take a
    reference to the real shm file.

diff --git a/ipc/shm.c b/ipc/shm.c
index acefe44fefef..f06505c68cc9 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,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);
-- 
2.17.0.484.g0c8726318c-goog

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

[Davidlohr Bueso]

On Sun, 08 Apr 2018, Eric Biggers wrote:
>@@ -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;

This probably merits a comment as it is adhoc to remap_file_pages(),
but otherwise:

Acked-by: Davidlohr Bueso <dbueso@suse.de>