2012/12/21 Michel Lespinasse <[email protected]>:
> Signed-off-by: Michel Lespinasse <[email protected]>
Hello, this patch introduced the following bug, seen while fuzzing with trinity:
[ 396.825414] BUG: unable to handle kernel NULL pointer dereference
at 0000000000000050
[ 396.826013] IP: [<ffffffff81176efb>] sys_remap_file_pages+0xbb/0x3e0
[ 396.826013] PGD 61e65067 PUD 3fb4067 PMD 0
[ 396.826013] Oops: 0000 [#8] SMP
[ 396.826013] CPU 0
[ 396.826013] Pid: 27553, comm: trinity-child53 Tainted: G D W
3.9.0-rc1+ #108 Bochs Bochs
[ 396.826013] RIP: 0010:[<ffffffff81176efb>] [<ffffffff81176efb>]
sys_remap_file_pages+0xbb/0x3e0
[ 396.826013] RSP: 0018:ffff880071a23f08 EFLAGS: 00010246
[ 396.826013] RAX: 0000000000000000 RBX: ffffffff00000000 RCX: 0000000000000001
[ 396.826013] RDX: 0000000000000000 RSI: ffffffff00000000 RDI: ffff8800679657c0
[ 396.826013] RBP: ffff880071a23f78 R08: 0000000000000002 R09: 0000000000000000
[ 396.826013] R10: 0000000026dad294 R11: 0000000000000000 R12: 0000000000000000
[ 396.826013] R13: ffff880067965870 R14: ffffffffffffffea R15: 0000000000000000
[ 396.826013] FS: 00007f6691a57700(0000) GS:ffff88007f800000(0000)
knlGS:0000000000000000
[ 396.826013] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 396.826013] CR2: 0000000000000050 CR3: 0000000068ab3000 CR4: 00000000000006f0
[ 396.826013] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 396.826013] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 396.826013] Process trinity-child53 (pid: 27553, threadinfo
ffff880071a22000, task ffff88006a360000)
[ 396.826013] Stack:
[ 396.826013] 0000000000000000 ffffffff810f33b6 0000000000000035
0000000000000000
[ 396.826013] 000000000000f000 0000000000000000 0000000026dad294
ffff8800679657c0
[ 396.826013] a80006367e000000 ffffffff00000000 00000000000006c0
00000000000000d8
[ 396.826013] Call Trace:
[ 396.826013] [<ffffffff810f33b6>] ? trace_hardirqs_on_caller+0x16/0x1f0
[ 396.826013] [<ffffffff81faf169>] system_call_fastpath+0x16/0x1b
[ 396.826013] Code: 43 e3 00 48 8b 45 a8 25 00 00 01 00 48 89 45 b8
48 8b 7d c8 48 89 de e8 74 9b 00 00 48 85 c0 49 89 c7 75 1c 49 c7 c6
ea ff ff ff <48> 8b 14 25 50 00 00 00 44 89 f0 e9 7f 02 00 00 0f 1f 44
00 00
[ 396.826013] RIP [<ffffffff81176efb>] sys_remap_file_pages+0xbb/0x3e0
[ 396.826013] RSP <ffff880071a23f08>
[ 396.826013] CR2: 0000000000000050
[ 396.876275] ---[ end trace 0444599b5c1ba02b ]---
> ---
> mm/fremap.c | 22 ++++++----------------
> 1 files changed, 6 insertions(+), 16 deletions(-)
>
> diff --git a/mm/fremap.c b/mm/fremap.c
> index 2db886e31044..b42e32171530 100644
> --- a/mm/fremap.c
> +++ b/mm/fremap.c
> @@ -129,6 +129,7 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size,
> struct vm_area_struct *vma;
> int err = -EINVAL;
> int has_write_lock = 0;
> + vm_flags_t vm_flags;
>
> if (prot)
> return err;
> @@ -228,30 +229,16 @@ get_write_lock:
> /*
> * drop PG_Mlocked flag for over-mapped range
> */
> - vm_flags_t saved_flags = vma->vm_flags;
> if (!has_write_lock)
> goto get_write_lock;
> + vm_flags = vma->vm_flags;
> munlock_vma_pages_range(vma, start, start + size);
> - vma->vm_flags = saved_flags;
> + vma->vm_flags = vm_flags;
> }
>
> mmu_notifier_invalidate_range_start(mm, start, start + size);
> err = vma->vm_ops->remap_pages(vma, start, size, pgoff);
> mmu_notifier_invalidate_range_end(mm, start, start + size);
> - if (!err) {
> - if (vma->vm_flags & VM_LOCKED) {
> - /*
> - * might be mapping previously unmapped range of file
> - */
> - mlock_vma_pages_range(vma, start, start + size);
> - } else if (!(flags & MAP_NONBLOCK)) {
> - if (unlikely(has_write_lock)) {
> - downgrade_write(&mm->mmap_sem);
> - has_write_lock = 0;
> - }
> - make_pages_present(start, start+size);
> - }
> - }
>
> /*
> * We can't clear VM_NONLINEAR because we'd have to do
> @@ -260,10 +247,13 @@ get_write_lock:
> */
>
> out:
> + vm_flags = vma->vm_flags;
When find_vma() fails, vma is NULL here.
> if (likely(!has_write_lock))
> up_read(&mm->mmap_sem);
> else
> up_write(&mm->mmap_sem);
> + if (!err && ((vm_flags & VM_LOCKED) || !(flags & MAP_NONBLOCK)))
> + mm_populate(start, size);
>
> return err;
> }
> --
> 1.7.7.3
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
On Sun, 10 Mar 2013 20:55:21 +0200 Tommi Rantala <[email protected]> wrote:
> 2012/12/21 Michel Lespinasse <[email protected]>:
> > Signed-off-by: Michel Lespinasse <[email protected]>
>
> Hello, this patch introduced the following bug, seen while fuzzing with trinity:
>
> [ 396.825414] BUG: unable to handle kernel NULL pointer dereference
> at 0000000000000050
>
> ...
>
> > + vm_flags = vma->vm_flags;
>
> When find_vma() fails, vma is NULL here.
Yup, thanks.
This, methinks:
From: Andrew Morton <[email protected]>
Subject: mm/fremap.c: fix oops on error path
If find_vma() fails, sys_remap_file_pages() will dereference `vma', which
contains NULL. Fix it by checking the pointer.
(We could alternatively check for err==0, but this seems more direct)
(The vm_flags change is to squish a bogus used-uninitialised warning
without adding extra code).
Reported-by: Tommi Rantala <[email protected]>
Cc: Michel Lespinasse <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
mm/fremap.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff -puN mm/fremap.c~mm-fremapc-fix-oops-on-error-path mm/fremap.c
--- a/mm/fremap.c~mm-fremapc-fix-oops-on-error-path
+++ a/mm/fremap.c
@@ -163,7 +163,8 @@ SYSCALL_DEFINE5(remap_file_pages, unsign
* and that the remapped range is valid and fully within
* the single existing vma.
*/
- if (!vma || !(vma->vm_flags & VM_SHARED))
+ vm_flags = vma->vm_flags;
+ if (!vma || !(vm_flags & VM_SHARED))
goto out;
if (!vma->vm_ops || !vma->vm_ops->remap_pages)
@@ -254,7 +255,8 @@ get_write_lock:
*/
out:
- vm_flags = vma->vm_flags;
+ if (vma)
+ vm_flags = vma->vm_flags;
if (likely(!has_write_lock))
up_read(&mm->mmap_sem);
else
_
(Sorry for the late reply)
On Mon, Mar 11, 2013 at 4:03 PM, Andrew Morton <[email protected]> wrote:
> On Sun, 10 Mar 2013 20:55:21 +0200 Tommi Rantala <[email protected]> wrote:
>
>> 2012/12/21 Michel Lespinasse <[email protected]>:
>> > Signed-off-by: Michel Lespinasse <[email protected]>
>>
>> Hello, this patch introduced the following bug, seen while fuzzing with trinity:
>>
>> [ 396.825414] BUG: unable to handle kernel NULL pointer dereference
>> at 0000000000000050
Good catch...
> From: Andrew Morton <[email protected]>
> Subject: mm/fremap.c: fix oops on error path
>
> If find_vma() fails, sys_remap_file_pages() will dereference `vma', which
> contains NULL. Fix it by checking the pointer.
>
> (We could alternatively check for err==0, but this seems more direct)
>
> (The vm_flags change is to squish a bogus used-uninitialised warning
> without adding extra code).
>
> Reported-by: Tommi Rantala <[email protected]>
> Cc: Michel Lespinasse <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> ---
>
> mm/fremap.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff -puN mm/fremap.c~mm-fremapc-fix-oops-on-error-path mm/fremap.c
> --- a/mm/fremap.c~mm-fremapc-fix-oops-on-error-path
> +++ a/mm/fremap.c
> @@ -163,7 +163,8 @@ SYSCALL_DEFINE5(remap_file_pages, unsign
> * and that the remapped range is valid and fully within
> * the single existing vma.
> */
> - if (!vma || !(vma->vm_flags & VM_SHARED))
> + vm_flags = vma->vm_flags;
> + if (!vma || !(vm_flags & VM_SHARED))
> goto out;
Your commit message indicates the vm_flags load here doesn't generate any code, but this seems very brittle and compiler dependent. If the compiler was to generate an actual load here, the issue with vma == NULL would reappear.
> if (!vma->vm_ops || !vma->vm_ops->remap_pages)
> @@ -254,7 +255,8 @@ get_write_lock:
> */
>
> out:
> - vm_flags = vma->vm_flags;
> + if (vma)
> + vm_flags = vma->vm_flags;
> if (likely(!has_write_lock))
> up_read(&mm->mmap_sem);
> else
Would the following work ? I think it's simpler, and with the compiler
I'm using here it doesn't emit warnings:
diff --git a/mm/fremap.c b/mm/fremap.c
index 0cd4c11488ed..329507e832fb 100644
--- a/mm/fremap.c
+++ b/mm/fremap.c
@@ -254,7 +254,8 @@ get_write_lock:
*/
out:
- vm_flags = vma->vm_flags;
+ if (!err)
+ vm_flags = vma->vm_flags;
if (likely(!has_write_lock))
up_read(&mm->mmap_sem);
else
--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
On Tue, Mar 12, 2013 at 8:24 AM, Michel Lespinasse <[email protected]> wrote:
> (Sorry for the late reply)
>
> On Mon, Mar 11, 2013 at 4:03 PM, Andrew Morton <[email protected]> wrote:
>> On Sun, 10 Mar 2013 20:55:21 +0200 Tommi Rantala <[email protected]> wrote:
>>
>>> 2012/12/21 Michel Lespinasse <[email protected]>:
>>> > Signed-off-by: Michel Lespinasse <[email protected]>
>>>
>>> Hello, this patch introduced the following bug, seen while fuzzing with trinity:
>>>
>>> [ 396.825414] BUG: unable to handle kernel NULL pointer dereference
>>> at 0000000000000050
>
> Good catch...
>
>> From: Andrew Morton <[email protected]>
>> Subject: mm/fremap.c: fix oops on error path
>>
>> If find_vma() fails, sys_remap_file_pages() will dereference `vma', which
>> contains NULL. Fix it by checking the pointer.
>>
>> (We could alternatively check for err==0, but this seems more direct)
>>
>> (The vm_flags change is to squish a bogus used-uninitialised warning
>> without adding extra code).
>>
>> Reported-by: Tommi Rantala <[email protected]>
>> Cc: Michel Lespinasse <[email protected]>
>> Signed-off-by: Andrew Morton <[email protected]>
>> ---
>>
>> mm/fremap.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff -puN mm/fremap.c~mm-fremapc-fix-oops-on-error-path mm/fremap.c
>> --- a/mm/fremap.c~mm-fremapc-fix-oops-on-error-path
>> +++ a/mm/fremap.c
>> @@ -163,7 +163,8 @@ SYSCALL_DEFINE5(remap_file_pages, unsign
>> * and that the remapped range is valid and fully within
>> * the single existing vma.
>> */
>> - if (!vma || !(vma->vm_flags & VM_SHARED))
>> + vm_flags = vma->vm_flags;
>> + if (!vma || !(vm_flags & VM_SHARED))
>> goto out;
>
> Your commit message indicates the vm_flags load here doesn't generate any code, but this seems very brittle and compiler dependent. If the compiler was to generate an actual load here, the issue with vma == NULL would reappear.
>
>> if (!vma->vm_ops || !vma->vm_ops->remap_pages)
>> @@ -254,7 +255,8 @@ get_write_lock:
>> */
>>
>> out:
>> - vm_flags = vma->vm_flags;
>> + if (vma)
>> + vm_flags = vma->vm_flags;
>> if (likely(!has_write_lock))
>> up_read(&mm->mmap_sem);
>> else
>
>
>
> Would the following work ? I think it's simpler, and with the compiler
> I'm using here it doesn't emit warnings:
>
> diff --git a/mm/fremap.c b/mm/fremap.c
> index 0cd4c11488ed..329507e832fb 100644
> --- a/mm/fremap.c
> +++ b/mm/fremap.c
> @@ -254,7 +254,8 @@ get_write_lock:
> */
>
> out:
> - vm_flags = vma->vm_flags;
> + if (!err)
> + vm_flags = vma->vm_flags;
> if (likely(!has_write_lock))
> up_read(&mm->mmap_sem);
> else
>
Is it still necessary to populate mm if bail out due
to a linear mapping encountered?
Hillf
On Mon, Mar 11, 2013 at 9:23 PM, Hillf Danton <[email protected]> wrote:
> Is it still necessary to populate mm if bail out due
> to a linear mapping encountered?
Yes. mmap_region() doesn't do it on its own, and we want the emulated
linear case to behave similarly to the true nonlinear case.
--
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.
On Mon, 11 Mar 2013 17:24:29 -0700 Michel Lespinasse <[email protected]> wrote:
> > --- a/mm/fremap.c~mm-fremapc-fix-oops-on-error-path
> > +++ a/mm/fremap.c
> > @@ -163,7 +163,8 @@ SYSCALL_DEFINE5(remap_file_pages, unsign
> > * and that the remapped range is valid and fully within
> > * the single existing vma.
> > */
> > - if (!vma || !(vma->vm_flags & VM_SHARED))
> > + vm_flags = vma->vm_flags;
> > + if (!vma || !(vm_flags & VM_SHARED))
> > goto out;
>
> Your commit message indicates the vm_flags load here doesn't generate any code, but this seems very brittle and compiler dependent. If the compiler was to generate an actual load here, the issue with vma == NULL would reappear.
I didn't try very hard. I have a surprisingly strong dislike of adding
"= 0" everywhere just to squish warnings.
There are actually quite a lot of places where this function could use
s/vma->vm_flags/vm_flags/ and might save a bit of code as a result.
But the function's pretty straggly and I stopped doing it.
>
> > if (!vma->vm_ops || !vma->vm_ops->remap_pages)
> > @@ -254,7 +255,8 @@ get_write_lock:
> > */
> >
> > out:
> > - vm_flags = vma->vm_flags;
> > + if (vma)
> > + vm_flags = vma->vm_flags;
> > if (likely(!has_write_lock))
> > up_read(&mm->mmap_sem);
> > else
>
>
>
> Would the following work ? I think it's simpler, and with the compiler
> I'm using here it doesn't emit warnings:
>
> diff --git a/mm/fremap.c b/mm/fremap.c
> index 0cd4c11488ed..329507e832fb 100644
> --- a/mm/fremap.c
> +++ b/mm/fremap.c
> @@ -254,7 +254,8 @@ get_write_lock:
> */
>
> out:
> - vm_flags = vma->vm_flags;
> + if (!err)
> + vm_flags = vma->vm_flags;
> if (likely(!has_write_lock))
> up_read(&mm->mmap_sem);
> else
Yes, this will work.
gcc-4.4.4 does generate the warning with this.
Testing `err' was my v1, but it is not obvious that err==0 always
correlates with vma!= NULL. This is true (I checked), and it had
better be true in the future, but it just feels safer and simpler to
test `vma' directly.