KASAN: use-after-free Read in shm_get_unmapped_area

[syzbot]

Hello,

syzkaller hit the following crash on
1418b852174ad50b3cb4738b8801626aefdc0bd9
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/master
compiler: gcc (GCC) 7.1.1 20170620
.config is attached
Raw console output is attached.




netlink: 1 bytes leftover after parsing attributes in process
`syz-executor7'.
==================================================================
BUG: KASAN: use-after-free in shm_get_unmapped_area+0xfd/0x120 ipc/shm.c:477
Read of size 8 at addr ffff8801c684fd28 by task syz-executor5/3616

CPU: 1 PID: 3616 Comm: syz-executor5 Not tainted 4.14.0-rc2-next-20170929+
#32
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:16 [inline]
dump_stack+0x194/0x257 lib/dump_stack.c:52
print_address_description+0x73/0x250 mm/kasan/report.c:252
kasan_report_error mm/kasan/report.c:351 [inline]
kasan_report+0x25b/0x340 mm/kasan/report.c:409
__asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:430
shm_get_unmapped_area+0xfd/0x120 ipc/shm.c:477
get_unmapped_area+0x18d/0x300 mm/mmap.c:2100
do_mmap+0x2aa/0xd50 mm/mmap.c:1364
do_mmap_pgoff include/linux/mm.h:2150 [inline]
SYSC_remap_file_pages mm/mmap.c:2829 [inline]
SyS_remap_file_pages+0x7ce/0xb30 mm/mmap.c:2745
entry_SYSCALL_64_fastpath+0x1f/0xbe
RIP: 0033:0x4520a9
RSP: 002b:00007fb8ac1d5c08 EFLAGS: 00000216 ORIG_RAX: 00000000000000d8
RAX: ffffffffffffffda RBX: 0000000000718210 RCX: 00000000004520a9
RDX: 0000000000000000 RSI: 0000000000001000 RDI: 0000000020002000
RBP: 0000000000000086 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000216 R12: 00000000004b91c7
R13: 00000000ffffffff R14: 0000000000000016 R15: 000000004008aef0

Allocated by task 3594:
save_stack_trace+0x16/0x20 arch/x86/kernel/stacktrace.c:59
save_stack+0x43/0xd0 mm/kasan/kasan.c:447
set_track mm/kasan/kasan.c:459 [inline]
kasan_kmalloc+0xad/0xe0 mm/kasan/kasan.c:551
kasan_slab_alloc+0x12/0x20 mm/kasan/kasan.c:489
kmem_cache_alloc+0x12e/0x760 mm/slab.c:3562
kmem_cache_zalloc include/linux/slab.h:687 [inline]
get_empty_filp+0xfb/0x4f0 fs/file_table.c:123
alloc_file+0x26/0x3a0 fs/file_table.c:164
__shmem_file_setup+0x2fb/0x540 mm/shmem.c:4228
shmem_kernel_file_setup+0x2a/0x40 mm/shmem.c:4254
newseg+0x7cd/0xcd0 ipc/shm.c:588
ipcget_new ipc/util.c:306 [inline]
ipcget+0x8f0/0xfd0 ipc/util.c:635
SYSC_shmget ipc/shm.c:673 [inline]
SyS_shmget+0x158/0x230 ipc/shm.c:657
entry_SYSCALL_64_fastpath+0x1f/0xbe

Freed by task 3580:
save_stack_trace+0x16/0x20 arch/x86/kernel/stacktrace.c:59
save_stack+0x43/0xd0 mm/kasan/kasan.c:447
set_track mm/kasan/kasan.c:459 [inline]
kasan_slab_free+0x71/0xc0 mm/kasan/kasan.c:524
__cache_free mm/slab.c:3504 [inline]
kmem_cache_free+0x77/0x280 mm/slab.c:3764
file_free_rcu+0x5c/0x70 fs/file_table.c:50
__rcu_reclaim kernel/rcu/rcu.h:195 [inline]
rcu_do_batch kernel/rcu/tree.c:2758 [inline]
invoke_rcu_callbacks kernel/rcu/tree.c:3012 [inline]
__rcu_process_callbacks kernel/rcu/tree.c:2979 [inline]
rcu_process_callbacks+0xd74/0x17d0 kernel/rcu/tree.c:2996
__do_softirq+0x29d/0xbb2 kernel/softirq.c:284

The buggy address belongs to the object at ffff8801c684fd00
which belongs to the cache filp of size 488
The buggy address is located 40 bytes inside of
488-byte region [ffff8801c684fd00, ffff8801c684fee8)
The buggy address belongs to the page:
page:ffffea00071a13c0 count:1 mapcount:0 mapping:ffff8801c684f080 index:0x0
flags: 0x200000000000100(slab)
raw: 0200000000000100 ffff8801c684f080 0000000000000000 0000000100000006
raw: ffffea00071bf460 ffffea00071a3e60 ffff8801dae3e300 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
ffff8801c684fc00: fb fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc
ffff8801c684fc80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> ffff8801c684fd00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff8801c684fd80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff8801c684fe00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================


---
This bug is generated by a dumb bot. It may contain errors.
See https://goo.gl/tpsmEJ for details.
Direct all questions to syzk...@googlegroups.com.
Please credit me with: Reported-by: syzbot <syzk...@googlegroups.com>

syzbot will keep track of this bug report.
Once a fix for this bug is committed, please reply to this email with:
#syz fix: exact-commit-title
To mark this as a duplicate of another syzbot report, please reply with:
#syz dup: exact-subject-of-another-report
If it's a one-off invalid bug report, please reply with:
#syz invalid
Note: if the crash happens again, it will cause creation of a new bug
report.
Note: all commands must start from beginning of the line.

[Eric Biggers]

From: Eric Biggers <ebig...@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+d11f321e7f192315...@syzkaller.appspotmail.com
Fixes: c8d78c1823f4 ("mm: replace remap_file_pages() syscall with emulation")
Cc: sta...@vger.kernel.org
Signed-off-by: Eric Biggers <ebig...@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

[Kirill A. Shutemov]

On Mon, Apr 09, 2018 at 04:30:39AM +0000, Eric Biggers wrote:
> From: Eric Biggers <ebig...@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.

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

[Eric Biggers]

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

[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

[Eric Biggers]

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

[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.

[Kirill A. Shutemov]

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....@linux.intel.com>

--
Kirill A. Shutemov

[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 <dbu...@suse.de>

[Eric Biggers]

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

Thanks,

Eric

[Eric Biggers]

From: Eric Biggers <ebig...@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.

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

@@ -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