mm_release() call in exit_mm() looks dangerous
In kernel/exit.c we have this code :
static void exit_mm(struct task_struct * tsk)
{
struct mm_struct *mm = tsk->mm;
mm_release(tsk, mm);
if (!mm)
return;...
But, mm_release() may dereference it's second argument ('mm'), so shouldn't we be doing the "!mm" test *before* we call mm_release() and not after?
I don't know the mm code well enough to be able to tell if some of the other stuff mm_release does needs to be done always and the mm dereference can't actually happen, but maybe someone else who knows the code better can tell... In any case, what's currently there looks a little shaky..
Signature · 3 lines
Jesper Juhl <[email protected]> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html Plain text mails only, please http://www.expita.com/nomime.html
Quoted 12 lines · Jesper Juhl
In
kernel/exit.cwe have this code :static void
exit_mm(struct task_struct * tsk){ struct mm_struct *mm = tsk->mm; mm_release(tsk, mm); if (!mm) return;...
Quoted 9 lines
But,
mm_release()may dereference it's second argument ('mm'), so shouldn't we be doing the "!mm" test *before* we callmm_release()and not after?
I don't know the mm code well enough to be able to tell if some of the other stuffmm_releasedoes needs to be done always and the mm dereference can't actually happen, but maybe someone else who knows the code better can tell... In any case, what's currently there looks a little shaky..
Yeah, it looks wrong. mm_release() calls deactivate_mm() as its first act, which could well dereference mm (though it often doesn't).
J
Quoted 23 lines · Jeremy Fitzhardinge
Quoted 12 lines · Jesper Juhl
In
kernel/exit.cwe have this code :static void
exit_mm(struct task_struct * tsk){ struct mm_struct *mm = tsk->mm; mm_release(tsk, mm); if (!mm) return;...
Quoted 9 lines
But,
mm_release()may dereference it's second argument ('mm'), so shouldn't we be doing the "!mm" test *before* we callmm_release()and not after?
I don't know the mm code well enough to be able to tell if some of the other stuffmm_releasedoes needs to be done always and the mm dereference can't actually happen, but maybe someone else who knows the code better can tell... In any case, what's currently there looks a little shaky..
Quoted 3 lines
Yeah, it looks wrong.
mm_release()callsdeactivate_mm()as its first act, which could well dereference mm (though it often doesn't).
So, whould simply moving the !mm check up as the first in the function be an appropriate way to deal with this?
Signature · 3 lines
Jesper Juhl <[email protected]> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html Plain text mails only, please http://www.expita.com/nomime.html
PATCH 1/1 kernel/exit.c: Fix use-before-check in exit_mm()
Quoted 24 lines · Jesper Juhl
Quoted 23 lines · Jeremy Fitzhardinge
Quoted 21 lines · Jesper Juhl
In
kernel/exit.cwe have this code :static void
exit_mm(struct task_struct * tsk){ struct mm_struct *mm = tsk->mm; mm_release(tsk, mm); if (!mm) return;...
But,
mm_release()may dereference it's second argument ('mm'), so shouldn't we be doing the "!mm" test *before* we callmm_release()and not after?
I don't know the mm code well enough to be able to tell if some of the other stuffmm_releasedoes needs to be done always and the mm dereference can't actually happen, but maybe someone else who knows the code better can tell... In any case, what's currently there looks a little shaky..
Quoted 5 lines
Quoted 3 lines
Yeah, it looks wrong.
mm_release()callsdeactivate_mm()as its first act, which could well dereference mm (though it often doesn't).So, whould simply moving the !mm check up as the first in the function be an appropriate way to deal with this?
I think yes. Patch below.
Fix use-before-check in kernel/exit.c
---
kernel/exit.c +1 -1
@@ -558,9 +558,9 @@ static void exit_mm(struct task_struct * tsk) {struct mm_struct *mm = tsk->mm; -mm_release (tsk, mm);if (!mm)return ; +mm_release (tsk, mm);/* * Serialize with any possible pending coredump. * We must hold mmap_sem around checking core_waiters