linux-kernel - Re: general protection fault in unlink_file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Sun, 13 Sep 2020 09:17:26 +0000
From:   linmiaohe <linmiaohe@...wei.com>
To:     Hillf Danton <hdanton@...a.com>
CC:     "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "syzkaller-bugs@...glegroups.com" <syzkaller-bugs@...glegroups.com>,
        syzbot <syzbot+c5d5a51dcbb558ca0cb5@...kaller.appspotmail.com>
Subject: Re: general protection fault in unlink_file_vma

Hi:
Hillf Danton <hdanton@...a.com> wrote:
> Tue, 08 Sep 2020 17:19:17 -0700
>> syzbot found the following issue on:
>> general protection fault, probably for non-canonical address 
>> 0xe00eeaee0000003b: 0000 [#1] PREEMPT SMP KASAN
>> KASAN: maybe wild-memory-access in range 
>> [0x00777770000001d8-0x00777770000001df]
...
>>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
>Looks like d70cec898324 ("mm: mmap: merge vma after call_mmap() if possible") added an extra fput.
>
>--- a/mm/mmap.c
>+++ b/mm/mmap.c
>@@ -1781,7 +1781,6 @@ unsigned long mmap_region(struct file *f
> 			merge = vma_merge(mm, prev, vma->vm_start, vma->vm_end, vma->vm_flags,
> 				NULL, vma->vm_file, vma->vm_pgoff, NULL, NULL_VM_UFFD_CTX);
> 			if (merge) {
>-				fput(file);
> 				vm_area_free(vma);
> 				vma = merge;
> 				/* Update vm_flags and possible addr to pick up the change. We don't
>

I reviewed the code carefully these days and I found vma_merge() do only fput() the vm_file of the linked vma in remove_next cases.
This gpf is much likely because the ->mmap() callback can change vma->vm_file and fput the original file. But my previous commit
failed to catch this case and always fput() the original file, hence add an extra fput().
The below patch would make the things right:

diff --git a/mm/mmap.c b/mm/mmap.c
index 080f44bcf7a8..80ea11bf12fa 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1816,7 +1816,11 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
                        merge = vma_merge(mm, prev, vma->vm_start, vma->vm_end, vma->vm_flags,
                                NULL, vma->vm_file, vma->vm_pgoff, NULL, NULL_VM_UFFD_CTX);
                        if (merge) {
-                               fput(file);
+                               /* ->mmap() can change vma->vm_file and fput the original file. So
+                                * fput the vma->vm_file here or we would add an extra fput for file
+                                * and cause general protection fault ultimately.
+                                */
+                               fput(vma->vm_file);
                                vm_area_free(vma);
                                vma = merge;
                                /* Update vm_flags and possible addr to pick up the change. We don't

It's very nice of you if you could help test this patch as I can't reproduce it in my product environment. And many thanks
for a possible Tested-by tag.

Thanks again.