| Message ID | 20200407095107.1988-1-miles.chen@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/gup: fix null pointer dereference detected by coverity | expand |
On Tue, Apr 07, 2020 at 05:51:07PM +0800, Miles Chen wrote: > In fixup_user_fault(), it is possible that unlocked is NULL, > so we should test unlocked before using it. > > For example, in arch/arc/kernel/process.c, NULL is passed > to fixup_user_fault(). > > SYSCALL_DEFINE3(arc_usr_cmpxchg, int *, uaddr, int, expected, int, new) > { > ... > ret = fixup_user_fault(current, current->mm, (unsigned long) uaddr, > FAULT_FLAG_WRITE, NULL); > ... > } > > Fixes: 4a9e1cda2748 ("mm: bring in additional flag for fixup_user_fault to signal unlock") > Signed-off-by: Miles Chen <miles.chen@mediatek.com> Reviewed-by: Ira Weiny <ira.weiny@intel.com> > --- > mm/gup.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/mm/gup.c b/mm/gup.c > index da3e03185144..a68d11dc232d 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1230,7 +1230,8 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm, > if (ret & VM_FAULT_RETRY) { > down_read(&mm->mmap_sem); > if (!(fault_flags & FAULT_FLAG_TRIED)) { > - *unlocked = true; > + if (unlocked) > + *unlocked = true; > fault_flags |= FAULT_FLAG_TRIED; > goto retry; > } > -- > 2.18.0
On Tue, Apr 7, 2020 at 11:49 PM Ira Weiny <ira.weiny@intel.com> wrote: > > On Tue, Apr 07, 2020 at 05:51:07PM +0800, Miles Chen wrote: > > In fixup_user_fault(), it is possible that unlocked is NULL, > > so we should test unlocked before using it. > > > > For example, in arch/arc/kernel/process.c, NULL is passed > > to fixup_user_fault(). > > > > SYSCALL_DEFINE3(arc_usr_cmpxchg, int *, uaddr, int, expected, int, new) > > { > > ... > > ret = fixup_user_fault(current, current->mm, (unsigned long) uaddr, > > FAULT_FLAG_WRITE, NULL); > > ... > > } > > > > Fixes: 4a9e1cda2748 ("mm: bring in additional flag for fixup_user_fault to signal unlock") > > Signed-off-by: Miles Chen <miles.chen@mediatek.com> > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> Acked-by: Souptick Joarder <jrdr.linux@gmail.com> > > > --- > > mm/gup.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/mm/gup.c b/mm/gup.c > > index da3e03185144..a68d11dc232d 100644 > > --- a/mm/gup.c > > +++ b/mm/gup.c > > @@ -1230,7 +1230,8 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm, > > if (ret & VM_FAULT_RETRY) { > > down_read(&mm->mmap_sem); > > if (!(fault_flags & FAULT_FLAG_TRIED)) { > > - *unlocked = true; > > + if (unlocked) > > + *unlocked = true; > > fault_flags |= FAULT_FLAG_TRIED; > > goto retry; > > } > > -- > > 2.18.0 >
On Tue, 7 Apr 2020 17:51:07 +0800 Miles Chen <miles.chen@mediatek.com> wrote: > In fixup_user_fault(), it is possible that unlocked is NULL, > so we should test unlocked before using it. > > For example, in arch/arc/kernel/process.c, NULL is passed > to fixup_user_fault(). > > SYSCALL_DEFINE3(arc_usr_cmpxchg, int *, uaddr, int, expected, int, new) > { > ... > ret = fixup_user_fault(current, current->mm, (unsigned long) uaddr, > FAULT_FLAG_WRITE, NULL); > ... > } (cc Peter) > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1230,7 +1230,8 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm, > if (ret & VM_FAULT_RETRY) { > down_read(&mm->mmap_sem); > if (!(fault_flags & FAULT_FLAG_TRIED)) { > - *unlocked = true; > + if (unlocked) > + *unlocked = true; > fault_flags |= FAULT_FLAG_TRIED; > goto retry; > } Not sure. If the caller passes FAULT_FLAG_ALLOW_RETRY then they must also pass in a valid non-NULL `unlocked'. If the caller passed FAULT_FLAG_ALLOW_RETRY and unlocked==NULL then the resulting oops is an appropriate way of reporting this mistake. I think?
On Tue, 2020-04-14 at 17:08 -0700, Andrew Morton wrote: > On Tue, 7 Apr 2020 17:51:07 +0800 Miles Chen <miles.chen@mediatek.com> wrote: > > > In fixup_user_fault(), it is possible that unlocked is NULL, > > so we should test unlocked before using it. > > > > For example, in arch/arc/kernel/process.c, NULL is passed > > to fixup_user_fault(). > > > > SYSCALL_DEFINE3(arc_usr_cmpxchg, int *, uaddr, int, expected, int, new) > > { > > ... > > ret = fixup_user_fault(current, current->mm, (unsigned long) uaddr, > > FAULT_FLAG_WRITE, NULL); > > ... > > } > > (cc Peter) > > > --- a/mm/gup.c > > +++ b/mm/gup.c > > @@ -1230,7 +1230,8 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm, > > if (ret & VM_FAULT_RETRY) { > > down_read(&mm->mmap_sem); > > if (!(fault_flags & FAULT_FLAG_TRIED)) { > > - *unlocked = true; > > + if (unlocked) > > + *unlocked = true; > > fault_flags |= FAULT_FLAG_TRIED; > > goto retry; > > } > > Not sure. If the caller passes FAULT_FLAG_ALLOW_RETRY then they must > also pass in a valid non-NULL `unlocked'. If the caller passed > FAULT_FLAG_ALLOW_RETRY and unlocked==NULL then the resulting oops is an > appropriate way of reporting this mistake. I think? > Agree. How about put "unlocked==NULL must not be used with FAULT_FLAG_ALLOW_RETRY." in the comment? Make it easier to understand the oops. e.g., --- a/mm/gup.c +++ b/mm/gup.c @@ -1176,7 +1176,8 @@ static bool vma_permits_fault(struct vm_area_struct *vma, * @address: user address * @fault_flags:flags to pass down to handle_mm_fault() * @unlocked: did we unlock the mmap_sem while retrying, maybe NULL if caller - * does not allow retry + * does not allow retry. If NULL, the caller must guarantee + * the fault_flags does not contain FAULT_FLAG_ALLOW_RETRY.
diff --git a/mm/gup.c b/mm/gup.c index da3e03185144..a68d11dc232d 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1230,7 +1230,8 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm, if (ret & VM_FAULT_RETRY) { down_read(&mm->mmap_sem); if (!(fault_flags & FAULT_FLAG_TRIED)) { - *unlocked = true; + if (unlocked) + *unlocked = true; fault_flags |= FAULT_FLAG_TRIED; goto retry; }
In fixup_user_fault(), it is possible that unlocked is NULL, so we should test unlocked before using it. For example, in arch/arc/kernel/process.c, NULL is passed to fixup_user_fault(). SYSCALL_DEFINE3(arc_usr_cmpxchg, int *, uaddr, int, expected, int, new) { ... ret = fixup_user_fault(current, current->mm, (unsigned long) uaddr, FAULT_FLAG_WRITE, NULL); ... } Fixes: 4a9e1cda2748 ("mm: bring in additional flag for fixup_user_fault to signal unlock") Signed-off-by: Miles Chen <miles.chen@mediatek.com> --- mm/gup.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)