Hi:
Hillf Danton <[email protected]> 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.
